Page MenuHome

Jumping from first to last item for UI_BTYPE_SEARCH_MENU.
Needs ReviewPublic

Authored by matc (matc) on Feb 20 2019, 12:18 PM.


Brecht Van Lommel (brecht)
Group Reviewers
Quick Hacks

Implements T51087 (Improve keyboard usage for spacebar search menu).

Enables scrolling over the beginning or end of search results. Additionally supports PageUp and PageDown keys.

Previously a first iteration was used to determine total amount of items and the position of a preselected item, before a second iteration would display the relevant slice of items. Now the logic to the determine the relevant slice is inside the add item function and can be applied during the first iteration.

Overall the behaviour is otherwise the same as in 2.79 and on master, with the exception of the operator search menu (F3). It is the only implementation without underlying data, that still keeps track of the previous selection. Because the filter string does not have to match exactly the selection, it is possible to keep track of the filter string and the selection as distinct values. This means the filter string can stay the same even after selecting different items multiple times.

Diff Detail

rB Blender

Event Timeline

matc (matc) created this revision.Feb 20 2019, 12:18 PM

Since I've worked in this area before, I decided to take a look.
I tested the code and it works great! No slowness or freezes.

The patch is too big for a quick review. Are all of these changes really necessary?
If the patch has too much that is outside the initial proposal, it is good to split it and propose the changes later.

Here are some points I identified:

4357 ↗(On Diff #13765)

It is not common to use BLI_assert at the beginning of a function.
There is an UNUSED...some_thing macro for this.

255 ↗(On Diff #13765)
267 ↗(On Diff #13765)

Is it really necessary to use malloc here?
Using malloc inside the loop does not seem to be an efficient solution, even more being freed soon after.
And in this case we still have a limited number of characters (MAX_ID_FULL_NAME).

277 ↗(On Diff #13765)

It seems like you can avoid allocating here too.

I removed all non-essential changes outside of interface_region_search.c.

One side effect is that the operator search menu does not actually select the last used operator. Instead the first that matches the filter is selected. This is in most cases the same one. I will reapply the fix for this on top of this diff.

All other removed changes were related to performance.

This diff now looks messed up. Can I fix this?

The diff I used to update the revision was made against the previous state and probably should have been against master. Will it fix things if I now upload a diff against master or will it only mess up more?

Update patch against master.

*EDIT* applied both patches and updated this diff, however seems like something might have gone wrong? - getting build errors.

@matc (matc), could you update this diff from what you have locally?

Diff against master.

Selecting the proper operator once the search menu is opened again.

The "Refresh" operator can be used to test this. It is one of those operators that don't show up as first item when searching for their exact name.

Fixed warnings and errors when building on linux.

Fixed object constraint target

Removed two warnings.

Diff against current master

Diff against current master.

Changed to use operator idname instead of a pointer to keep track of previously used operator.

matc (matc) edited the summary of this revision. (Show Details)Aug 7 2019, 6:14 PM
matc (matc) updated this revision to Diff 16916.

Rewrote the patch. More contained and better readable.

Navigating into unlinked state is possible from first and last item.

Some test cases:
Viewlayer property (top right): pointer lookup, not unlinkable
Modifier properties like Target Object and Vertex Group: string lookup, unlinkable
Search menu (F3): string lookup (that fails, because the name is cut off), not unlinkable