UI: Add Preference to Instant Search in all Menus #112925

Harley Acheson · 2023-09-26T23:02:53+02:00

Harley Acheson commented

2023-09-26 23:02:53 +02:00

Allow "type to search" in all menus. with a user Preference to turn
it off if not wanted.

This PR removes the menu option SEARCH_ON_KEY_PRESS and the SearchOnKeyPress MenuTypeFlag, replacing this with a user preference. U.menu_key_behavior is of type eUserpref_MenuKeyBehavior, which contains USER_MENU_KEY_SEARCH and USER_MENU_KEY_ACCELERATOR - the former as default.

This leaves in the "Type to search..." text shown in the status bar when opening a menu if this option is on.

We normally don't allow searching in menus that have names that end in "_context_menu", but this PR allows searching in context menus if we are doing a "single menu search".

Allow "type to search" in all menus. with a user Preference to turn it off if not wanted. --- This PR removes the menu option `SEARCH_ON_KEY_PRESS` and the `SearchOnKeyPress` `MenuTypeFlag`, replacing this with a user preference. `U.menu_key_behavior` is of type `eUserpref_MenuKeyBehavior`, which contains USER_MENU_KEY_SEARCH and USER_MENU_KEY_ACCELERATOR - the former as default. This leaves in the "Type to search..." text shown in the status bar when opening a menu if this option is on. We normally don't allow searching in menus that have names that end in "_context_menu", but this PR allows searching in context menus if we are doing a "single menu search".

Harley Acheson added this to the User Interface project 2023-09-26 23:03:22 +02:00

Harley Acheson requested review from Hans Goudey 2023-09-26 23:03:28 +02:00

Harley Acheson requested review from Pablo Vazquez 2023-09-26 23:03:34 +02:00

Hans Goudey commented

2023-09-26 23:08:09 +02:00

Could we still exclude context menus for menu search (but not single-menu search)? I think the code should know whether it was called for a single menu or not. I think skipping context menus was only helpful for menu search which includes all the menus.

Could we still exclude context menus for menu search (but **not** single-menu search)? I think the code should know whether it was called for a single menu or not. I think skipping context menus was only helpful for menu search which includes all the menus.

Hans Goudey commented

2023-09-26 23:09:10 +02:00

Also, just wanted to ping @JacquesLucke, since this is affecting feature he added originally. This PR is based on the discussions in the UI meeting today.

Harley Acheson commented

2023-09-26 23:28:51 +02:00

@HooglyBoogly Could we still exclude context menus for menu search (but not single-menu search)?

Great idea! Done.

> @HooglyBoogly Could we still exclude context menus for menu search (but **not** single-menu search)? Great idea! Done.

Hans Goudey approved these changes 2023-09-26 23:51:40 +02:00

Hans Goudey left a comment

Two unused variables:

home/hans/blender-git/blender/source/blender/editors/interface/interface_region_menu_popup.cc:408:19: warning: unused variable ‘mt’ [-Wunused-variable]
  408 |     if (MenuType *mt = UI_but_menutype_get(but)) {
      |                   ^~
[429/1945] Building CXX object source/blender/editors/interface/CMakeFiles/bf_editor_interface.dir/interface_handlers.cc.o
/home/hans/blender-git/blender/source/blender/editors/interface/interface_handlers.cc: In function ‘int ui_handle_menu_letter_press(bContext*, ARegion*, uiPopupBlockHandle*, const wmEvent*, uiBlock*)’:
/home/hans/blender-git/blender/source/blender/editors/interface/interface_handlers.cc:10295:15: warning: unused variable ‘mt’ [-Wunused-variable]
10295 |     MenuType *mt = WM_menutype_find(menu->menu_idname, false);
      |               ^~

Other than that and my simple inline comment, this worked well in testing and the code looks straightforward.

Two unused variables: ``` home/hans/blender-git/blender/source/blender/editors/interface/interface_region_menu_popup.cc:408:19: warning: unused variable ‘mt’ [-Wunused-variable] 408 | if (MenuType *mt = UI_but_menutype_get(but)) { | ^~ [429/1945] Building CXX object source/blender/editors/interface/CMakeFiles/bf_editor_interface.dir/interface_handlers.cc.o /home/hans/blender-git/blender/source/blender/editors/interface/interface_handlers.cc: In function ‘int ui_handle_menu_letter_press(bContext*, ARegion*, uiPopupBlockHandle*, const wmEvent*, uiBlock*)’: /home/hans/blender-git/blender/source/blender/editors/interface/interface_handlers.cc:10295:15: warning: unused variable ‘mt’ [-Wunused-variable] 10295 | MenuType *mt = WM_menutype_find(menu->menu_idname, false); | ^~ ``` Other than that and my simple inline comment, this worked well in testing and the code looks straightforward.

source/blender/makesrna/intern/rna_userdef.cc Outdated

						
				@ -4912,2 +4922,4 @@

				  /* menus */

				  prop = RNA_def_property(srna, "menu_key_behavior", PROP_ENUM, PROP_NONE);

Hans Goudey commented

2023-09-26 23:43:18 +02:00

The enum items array can be defined right here in this function, it doesn't have to be global:

  /* menus */

  static const EnumPropertyItem rna_enum_menu_key_behavior_items[] = {
      {USER_MENU_KEY_SEARCH, "SEARCH", 0, "Type to Search", "Type to immediately start searching"},
      {USER_MENU_KEY_ACCELERATOR,
       "ACCELERATOR",
       0,
       "Accelerator keys",
       "Select item by accelerator"},
      {0, nullptr, 0, nullptr, nullptr},
  };

  prop = RNA_def_property(srna, "menu_key_behavior", PROP_ENUM, PROP_NONE);
  RNA_def_property_enum_sdna(prop, nullptr, "menu_key_behavior");
  RNA_def_property_enum_items(prop, rna_enum_menu_key_behavior_items);
  RNA_def_property_ui_text(
      prop, "Menu Key Behavior", "What happens when you press a key when a menu is open");

This resolved an error for me:

/home/hans/blender-git/blender/source/blender/makesrna/intern/rna_userdef.cc:95:31: error: ‘rna_enum_menu_key_behavior_items’ was declared ‘extern’ and later ‘static’ [-fpermissive]
   95 | static const EnumPropertyItem rna_enum_menu_key_behavior_items[] = {
      |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The enum items array can be defined right here in this function, it doesn't have to be global: ``` /* menus */ static const EnumPropertyItem rna_enum_menu_key_behavior_items[] = { {USER_MENU_KEY_SEARCH, "SEARCH", 0, "Type to Search", "Type to immediately start searching"}, {USER_MENU_KEY_ACCELERATOR, "ACCELERATOR", 0, "Accelerator keys", "Select item by accelerator"}, {0, nullptr, 0, nullptr, nullptr}, }; prop = RNA_def_property(srna, "menu_key_behavior", PROP_ENUM, PROP_NONE); RNA_def_property_enum_sdna(prop, nullptr, "menu_key_behavior"); RNA_def_property_enum_items(prop, rna_enum_menu_key_behavior_items); RNA_def_property_ui_text( prop, "Menu Key Behavior", "What happens when you press a key when a menu is open"); ``` This resolved an error for me: ``` /home/hans/blender-git/blender/source/blender/makesrna/intern/rna_userdef.cc:95:31: error: ‘rna_enum_menu_key_behavior_items’ was declared ‘extern’ and later ‘static’ [-fpermissive] 95 | static const EnumPropertyItem rna_enum_menu_key_behavior_items[] = { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ```

Harley marked this conversation as resolved

Pablo Vazquez commented

2023-09-27 00:02:50 +02:00

Same, can't compile here (macOS):

/source/blender/makesrna/intern/rna_userdef.cc:95:31: error: static declaration of 'rna_enum_menu_key_behavior_items' follows non-static declaration

static const EnumPropertyItem rna_enum_menu_key_behavior_items[] = {
                              ^
/source/blender/makesrna/RNA_enum_items.hh:177:10: note: previous declaration is here
DEF_ENUM(rna_enum_menu_key_behavior_items)

Same, can't compile here (macOS): ``` /source/blender/makesrna/intern/rna_userdef.cc:95:31: error: static declaration of 'rna_enum_menu_key_behavior_items' follows non-static declaration static const EnumPropertyItem rna_enum_menu_key_behavior_items[] = { ^ /source/blender/makesrna/RNA_enum_items.hh:177:10: note: previous declaration is here DEF_ENUM(rna_enum_menu_key_behavior_items) ```

Harley Acheson commented

2023-09-27 00:05:06 +02:00

@blender-bot build

Harley Acheson commented

2023-09-27 00:06:03 +02:00

@pablovazquez - Same, can't compile here (macOS)

Should work now. I'm also testing on the bots to make sure.

> @pablovazquez - Same, can't compile here (macOS) Should work now. I'm also testing on the bots to make sure.

🎉 1

Julian Eisel commented

2023-09-27 16:25:41 +02:00

What is the reason for this preference? That seems quite radical (while not actually daring to commit to it, hence making it optional), and IMO the wrong approach to this.

What we discussed earlier and what I've reconfirmed with Pablo is that it mostly depends on the type of menu if accelerator keys make sense or not. For large menus, being able to search is very useful. For small menus not so much. For menus with a fixed set of items accelerator keys are useful (because they don't change), for dynamic menus where items change a lot they don't (because they change and don't work well for muscle memory). (Context menus are an example of the latter, accelerator keys don't make sense there. But they are also often not big so search may not be that useful either. This could go either way, although I'd vote for search personally)

To be clear, as far as I understand this patch, it adds a preferences option for this to either use accelerator keys, or search everywhere. Nothing in between (like what was agreed on and committed earlier). This honestly feels like week design, and we just give the user a choice instead of coming up with something proper.

What is the reason for this preference? That seems quite radical (while not actually daring to commit to it, hence making it optional), and IMO the wrong approach to this. What we discussed earlier and what I've reconfirmed with Pablo is that it mostly depends on the type of menu if accelerator keys make sense or not. For large menus, being able to search is very useful. For small menus not so much. For menus with a fixed set of items accelerator keys are useful (because they don't change), for dynamic menus where items change a lot they don't (because they change and don't work well for muscle memory). _(Context menus are an example of the latter, accelerator keys don't make sense there. But they are also often not big so search may not be that useful either. This could go either way, although I'd vote for search personally)_ To be clear, as far as I understand this patch, it adds a preferences option for this to either use accelerator keys, or search everywhere. Nothing in between (like what was agreed on and committed earlier). This honestly feels like week design, and we just give the user a choice instead of coming up with something proper.

Julian Eisel commented

2023-09-27 16:28:59 +02:00

To be clear, I can't find a rationale for this design solution neither here nor in the latest meeting notes. To me they read: "this is how it should probably behave; so let's do something totally different".

To be clear, I can't find a rationale for this design solution neither here nor in the latest [meeting notes](https://devtalk.blender.org/t/2023-09-26-user-interface-meeting/31112). To me they read: "this is how it should probably behave; so let's do something totally different".

Julian Eisel reviewed 2023-09-27 16:44:04 +02:00

scripts/startup/bl_ui/space_userpref.py Outdated

						
				@ -320,3 +322,4 @@

				class USERPREF_PT_interface_menus_mouse_over(InterfacePanel, CenterAlignMixIn, Panel):

				    bl_label = "Open on Mouse Over"

				    bl_parent_id = "USERPREF_PT_interface_menus"

				    bl_options = {'DEFAULT_CLOSED'}

Julian Eisel commented

2023-09-27 16:13:45 +02:00

Why is this changed?

Harley marked this conversation as resolved

scripts/startup/bl_ui/space_userpref.py Outdated

						
				@ -342,6 +345,7 @@ class USERPREF_PT_interface_menus_mouse_over(InterfacePanel, CenterAlignMixIn, P

				class USERPREF_PT_interface_menus_pie(InterfacePanel, CenterAlignMixIn, Panel):

				    bl_label = "Pie Menus"

				    bl_parent_id = "USERPREF_PT_interface_menus"

				    bl_options = {'DEFAULT_CLOSED'}

Julian Eisel commented

2023-09-27 16:13:49 +02:00

Why is this changed?

Harley marked this conversation as resolved

source/blender/editors/interface/interface_template_search_menu.cc Outdated

						
				@ -463,3 +465,3 @@

				    };

				    for (int i = 0; i < ARRAY_SIZE(idname_array); i++) {

				      MenuType *mt = WM_menutype_find(idname_array[i], false);

				      MenuType *mt = (idname_array[i]) ? WM_menutype_find(idname_array[i], false) : nullptr;

Julian Eisel commented

2023-09-27 16:10:13 +02:00

Don't hide this logic inside a variable initialization. Make it explicit (above this line):

if (!idname_array[i]) {
  continue;
}

Don't hide this logic inside a variable initialization. Make it explicit (above this line): ``` if (!idname_array[i]) { continue; } ```

Harley marked this conversation as resolved

source/blender/editors/interface/interface_template_search_menu.cc Outdated

						
				@ -472,2 +474,2 @@

				  {

				    /* Exclude context menus because:

				  if (!single_menu_idname) {

				    /* Exclude context menus (when not single menu search) because:

Julian Eisel commented

2023-09-27 16:08:30 +02:00

"Single menu search" is an invented term that doesn't really add much. Just say "when not searching in a single menu only".

Harley marked this conversation as resolved

Harley Acheson commented

2023-09-27 16:50:10 +02:00

@JulianEisel - while not actually daring to commit to it, hence making it optional

I'm not certain what you mean by this. This PR makes "type to search" the default, with the preference being used to turn it off if unwanted.

To be clear, as far as I understand this patch, it adds a preferences option for this to either use accelerator keys, or search everywhere. Nothing in between (like what was agreed on and committed earlier). This honestly feels like week design, and we just give the user a choice instead of coming up with something proper.

This PR is just an implementation of what the UI Module discussed and agreed upon in the meeting yesterday attended by Dalai Felinto, Hans Goudey, Leon Schittek, Pablo Vazquez, Thomas Dinges, and me. We found that having anything "in between" is inconsistent and confusing.

For large menus, being able to search is very useful. For small menus not so much

It is a bit weird to have different behavior for large and small menus, especially since they could be side-by-side. For example, In current master go to the 3DView object mode menu. Open the "Select" menu and press "a" and you will select all objects. Now open the "Add" menu, press "a" and it will start a search. This inconsistency is difficult to convey to users. It was felt that going "all in" would make all dropdown and context menus behave in the same manner all the time without any need for a "type to search..." hint anywhere.

But at the end of the day this PR is just what was discussed and wanted. This can now be compiled and tested to confirm that this a solution. If too " radical" it can be rejected.

This honestly feels like week design, and we just give the user a choice instead of coming up with something proper.

This isn't my design but what we agreed to in the UI Module meeting.

> @JulianEisel - while not actually daring to commit to it, hence making it optional I'm not certain what you mean by this. This PR makes "type to search" the default, with the preference being used to turn it off if unwanted. > To be clear, as far as I understand this patch, it adds a preferences option for this to either use accelerator keys, or search everywhere. Nothing in between (like what was agreed on and committed earlier). This honestly feels like week design, and we just give the user a choice instead of coming up with something proper. This PR is just an implementation of what the UI Module discussed and agreed upon in the meeting yesterday attended by Dalai Felinto, Hans Goudey, Leon Schittek, Pablo Vazquez, Thomas Dinges, and me. We found that having anything "in between" is inconsistent and confusing. > For large menus, being able to search is very useful. For small menus not so much It is a bit weird to have different behavior for large and small menus, especially since they could be side-by-side. For example, In current master go to the 3DView object mode menu. Open the "Select" menu and press "a" and you will select all objects. Now open the "Add" menu, press "a" and it will start a search. This inconsistency is difficult to convey to users. It was felt that going "all in" would make all dropdown and context menus behave in the same manner all the time without any need for a "type to search..." hint anywhere. But at the end of the day this PR is just what was discussed and wanted. This can now be compiled and tested to confirm that this a solution. If too " radical" it can be rejected. > This honestly feels like week design, and we just give the user a choice instead of coming up with something proper. This isn't my design but what we agreed to in the UI Module meeting.

👍 2

Harley Acheson commented

2023-09-27 17:51:07 +02:00

@JulianEisel - Why is this changed?

I had changed the parent "Menus" panel to be open by default in order to better show this new option. Having its children open by default too added a lot of exposed content in the initial view of Preferences.

I assumed that your comment was not rhetorical and so undid those changes. This PR now keeps the "Menus" panel closed by default with its children open inside of it.

> @JulianEisel - Why is this changed? I had changed the parent "Menus" panel to be open by default in order to better show this new option. Having its children open by default too added a lot of exposed content in the initial view of Preferences. I assumed that your comment was not rhetorical and so undid those changes. This PR now keeps the "Menus" panel closed by default with its children open inside of it.

Harley Acheson force-pushed SearchMenus from 995ed60f33 to 4a01c270d2

2023-10-03 19:38:36 +02:00

Compare

Harley Acheson changed title from ~~UI: Add Preference to Instant Search in all Menus~~ to UI: Add Preference to Instant Search in all Menus

2023-10-03 19:38:56 +02:00

Harley changed target branch from main to blender-v4.0-release

2023-10-03 19:38:58 +02:00

Harley Acheson added 1 commit 2023-10-03 20:36:19 +02:00

b4a2460a46 use pup->popup rather than pup->title

Harley Acheson added 1 commit 2023-10-03 21:06:53 +02:00

08a43c19d8 Line added while testing - does nothing.

Harley Acheson referenced this pull request