Page MenuHome

Property Search: Single tab
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Wed, Sep 9, 8:01 PM.

Details

Summary

This adds a search bar to the properties editor. The full search for every tab
isn't includede in this patch, but the interaction with panels, searching
behavior, internal UI, region level, and DNA changes are included here.

The block-level search works by iterating over the block's buttons and
checking whether they match the search. If they do, they are tagged with
a flag, and the block's panel is tagged too.

For every update (text edit), the panel's expansion is set to whether the
panel has a result or not.

There is some complications to this that you might no initially think of:

  1. Closed panel's subpanels have to be searched too. This adds some complexity and special cases to the area-level panel layout code.
  2. Maybe more if I think of things to add here

There might be some methods of simplifying some of the logic, particularly
around choosing whether to highlight panel headers. Also note that automatic
subpanel expansion isn't working right now. I'll look into this, but I want to post
all the patches first.

Future Improvements
Here are some improvements possible in the future that won't be part of this patch:

  1. Use the new fuzzy search in BLI
  2. Reseting panels to their expansion before the search started if you esc out of the text box
  3. Open parent panels if their subpanels have a match but they don't. This requires adding a reference to parent panels for subpanels.

Diff Detail

Repository
rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Wed, Sep 9, 8:01 PM
Hans Goudey (HooglyBoogly) created this revision.
Campbell Barton (campbellbarton) added inline comments.
source/blender/editors/interface/interface.c
3499–3505

This is a bit odd having properties space being checked for in UI_block_begin,

Why not pass the search string in to ED_region_panels_layout_ex ?

Side-note, contexts[], category_override, search_string could be wrapped into a struct if the arguments are getting a bit much.

source/blender/editors/interface/interface_intern.h
525

Comment who owns the data, is it allocated etc.

Also, shouldn't this be const? If it can be written to, it should have a size variable to avoid buffer overrun.

source/blender/editors/interface/interface_layout.c
5190–5198

These should use translated strings too. See rna_translate_ui_text.

5210

Should use RNA_property_enum_items_gettexted, same for other descriptions.

Campbell Barton (campbellbarton) requested changes to this revision.Thu, Sep 10, 12:54 PM
This revision now requires changes to proceed.Thu, Sep 10, 12:54 PM
Julian Eisel (Severin) added inline comments.
source/blender/editors/interface/interface_intern.h
84

"later used to deactivate it" - I find that a bit confusing. Isn't it more like "later used to determine which items to keep enabled, and which to disable"?

source/blender/editors/interface/interface_layout.c
5219

No need for void * cast.

5784

Would avoid the double negated logic below and use search_enabled.

5796–5798

This doesn't seem like something that should be in a UI_block_layout_resolve() function, it should be done by the caller I guess. Given that UI_block_layout_resolve() is called in quite some places, I'd suggest adding a wrapper, say UI_block_layout_end() that resolves the layout and does the search post-processing (disable buttons).

source/blender/makesdna/DNA_space_types.h
169

Number seems a bit arbitrary, maybe use UI_MAX_NAME_STR, or something else that is defined.
I'd also suggest putting this into a runtime only struct that is NULLed on file read. I don't see too great use in keeping the search string when loading files, not worth the memory added to .blends.

Julian Eisel (Severin) requested changes to this revision.Thu, Sep 10, 12:55 PM
Julian Eisel (Severin) edited the summary of this revision. (Show Details)Thu, Sep 10, 2:18 PM
Hans Goudey (HooglyBoogly) marked 7 inline comments as done.
  • Merge branch 'property-search-add-theme-color' into property-search-single-tab
  • Property Search: Add comment to field, make const
  • Property Search: Refactor setting block search filter
  • Property Search: Clarify comment
  • Property Search: Move search string to a runtime struct
  • Property Search: Refactor block search into new function
  • Property Search: Remove redundant button hiding code
  • Property Search: Use RNA_property_enum_items_gettexted
source/blender/editors/interface/interface.c
3499–3505

Good point, that's not great. I passed it to ed_panel_draw from ED_region_panels_layout_ex which can get it from the area with an existing helper function. That's nice because it stays generalized.

source/blender/editors/interface/interface_intern.h
84

Ah right, the meaning of this flag was reversed before and I didn't update the comment.

source/blender/editors/interface/interface_layout.c
5190–5198

I'm a bit confused here, I thought these functions accounted for that. For example:

const char *RNA_property_ui_name(const PropertyRNA *prop)
{
  return CTX_IFACE_(prop->translation_context, rna_ensure_property_name(prop));
}

Would this not do the translation?

5219

It seems we do need a cast here to avoid a warning freeing the const items_array. That's how it's done everywhere else at least. But I changed it to a (EnumPropertyItem *) cast, that's more clear about the reasoning.

5796–5798

I refactored this area a bit. Now you run UI_block_apply_search_filter() right before UI_block_layout_resolve(), and the two are completely separate processes. Should make more sense now.

source/blender/makesdna/DNA_space_types.h
169

I thought it might be nice to keep it stored because I figured people might want to keep it on more permanently in some cases, but I don't really care much, so I moved it to a new runtime struct.

Note that the size definition is just in a comment because this is in DNA.

  • Property Search: Set expansion for subpanels based on results
  • Property Search: Clear region search filter active flag on read
  • Property Search: Fix tab buttons graying out after refactor

I don't know about the translation thing, so I'll leave that up to Campbell to check.
Otherwise fine!

source/blender/editors/interface/interface_layout.c
5170

When I first read this it was unclear to me why it's needed. Maybe good to clarify with a comment.

source/blender/makesdna/DNA_space_types.h
170

I'd suggest making this a pointer. We don't typically do that but in this case I think it makes sense, since we don't want the string buffer to be written to files. (The fact that we still write the runtime structs to files generally bothers me.)
You'd have to ensure it's allocated, which you could just to in buttons_init().

Hans Goudey (HooglyBoogly) marked an inline comment as done.
  • Merge branch 'property-search-add-theme-color' into property-search-single-tab
  • Property Search: Code to use new button group structs
  • Property Search: Fix memory leak
  • Property Search: Add explanatory comment
  • Property Search: Use pointer for properties space runtime struct

Should

  • Fix for previous commit

I tested searching translated UI messages and it works fine, so every previous comment
should be addressed.

Campbell Barton (campbellbarton) added inline comments.
source/blender/editors/interface/interface_layout.c
5190–5198

Yes, this does.

This revision is now accepted and ready to land.Tue, Sep 15, 2:40 AM
source/blender/editors/interface/interface_layout.c
5160

Better add a comment on why this is disabled.

source/blender/makesrna/intern/rna_space.c
1823

Use rna_area_from_space(ptr) here, avoid context.

Julian Eisel (Severin) requested changes to this revision.Tue, Sep 15, 11:48 AM
Julian Eisel (Severin) added inline comments.
source/blender/makesdna/DNA_space_types.h
132

Since this is now a pointer, you can and should avoid writing this to SDNA with the two #'s:

#
#
typedef struct Foo {
  ...
} Foo;
This revision now requires changes to proceed.Tue, Sep 15, 11:48 AM
source/blender/editors/space_buttons/space_buttons.c
117–125

You have to NULL-check if the runtime data already exists. The init() callback is called multiple times. E.g. on every area size change, window size change or window move. That should also be the source of the memory leak in D8859.

Hans Goudey (HooglyBoogly) marked 8 inline comments as done.
  • Property Search: Apply fixes from all tab code
  • Property Search: Don't write runtime struct to SDNA
  • Property Search: Use proper spacing for header
  • Property Search: Only initialize runtime struct if NULL
  • Property Search: Don't use context for property update function
  • Property Search: Update comments and function name

There's an issue with clearing the search string in buttons_init(), but I already let Hans know and he'll address it before committing (updating the patches is a hassle I guess). So accepting this already.

This revision is now accepted and ready to land.Tue, Sep 15, 4:53 PM
This revision was automatically updated to reflect the committed changes.