Tentative fix for T53257: Too many color management transforms to display in the menu
Needs ReviewPublic

Authored by Vuk Gardašević (lijenstina) on Nov 15 2017, 10:57 PM.

Details

Summary

This is a tentative workaround for T53257.

Calls the prop directly using search with bl_rna access if there is more than 30 items to display.

Preferably this should be solved, if possible, on a widget level > changing the call if there are too many items to display.
The current UI behavior is not capable of dealing with complexity. The Aces 1.03 has over 300 of entries which will get cut of the screen.
For instance all EnumProperty entries, as the generator script from the mentioned task will do the same.

Note : the original code revision dealt only with a python workaround. The current implementation in C takes account of the window size, makes an estimation of the menu entries that are text depended and adjusts the number of columns accordingly.

Diff Detail

Think it would be better to change this in our UI code, so we have a limit where large lists are displayed with a search box.

Otherwise we need workarounds in each area that might happen to show a large list.

Campbell Barton (campbellbarton) requested changes to this revision.Dec 6 2017, 3:20 PM
This revision now requires changes to proceed.Dec 6 2017, 3:20 PM

I agree, however is that something that could be done from the python side or it needs changes in the C code?

This is a change that will take in account the screen width and try to adjust the column number accordingly.
Sorry for some empty lines changes.

Vuk Gardašević (lijenstina) retitled this revision from Temporary fix for T53257: Too many color management transforms to display in the menu to Tentative fix for T53257: Too many color management transforms to display in the menu.Mar 27 2018, 9:09 PM

Some little style tweaks. Added a comment about the text length.

Julian Eisel (Severin) requested changes to this revision.Mar 29 2018, 3:09 AM

Was checking if we can do this more generically, e.g. in ui_block_bounds_calc_text. Doesn't seem to be possible in a nice way though, so think this approach is fine.

There is one issue still though:

I get that just by scaling the window horizontally a bit.
Other than that, I only got some small code-style suggestions, nothing big really.

source/blender/editors/interface/interface.c
3321

You can put the MENU_PADDING define into interface_handlers.c.

3324

Term width is a bit vague, why not use screen_margin? Or actually why not use UI_SCREEN_MARGIN directly?

3345

Would make this a local var to limit scope.
Also, things like UI_UNIT_X * 1.75f seem to serve some purpose, but hard to tell which and where the value comes from. Could you make that more clear?

3367

(txtwidth + UI_UNIT_X) seems to be the column width? Would suggest adding a var for this if so. Would also suggest to use names like column_count instead of just columns, just to be clear.

3377

Prefer while ((rows * columns) < totitems). Again, just to be clear and make reading more fluent.

3410

That is in fact an additional change. Don't mind it personally but should probably committed separately.

This revision now requires changes to proceed.Mar 29 2018, 3:09 AM

Addressed the points from the review
Added an UI scale factor to check for the column width, in cases when scalling the UI leads to cutting off the first column

Concerning the separator line, if it is deemed outside of scope, it can be merged later.

Vuk Gardašević (lijenstina) marked 5 inline comments as done.Apr 1 2018, 4:08 AM

Will update the patch soon.
The issue with the current one is that it doesn't account for the size of the property name. Since it is called first if it is longer than the column width, it can throw off the calculation of the columns.

Vuk Gardašević (lijenstina) planned changes to this revision.Apr 1 2018, 4:09 AM

Updated the calculations of the column width:

  • Takes in account the property name length.
  • Add menu padding to the strings length, since it makes the calculation easier
  • The UI scale factor is not needed

Currently the menu size will fail only if the first column entries / property name are wider than the window size since it has to have one column and the text is not abbreviated for space.
However that is outside the scope of this patch.

Julian Eisel (Severin) requested changes to this revision.Apr 4 2018, 5:54 PM

Almost there! Functionality wise this seems to work just fine now. Just have a little more nit picking ;)

source/blender/editors/interface/interface.c
3351–3354

Generally, if you have to copy logic/behavior from one place to another you should consider calling a function instead. In this case I don't see why you shouldn't just call ui_text_icon_width().

Or actually... most of ui_def_but_rna__menu() should be in interface_layout.c, there are similar functions there anyway (like ui_item_enum_expand()).
So I'd suggest to move most of ui_def_but_rna__menu() into interface_layout.c. Call it something like ui_layout_enum_menu() and forward declare it in interface_intern.h. In ui_def_but_rna_menu() we would only do the callback specific setup (say, the current first 3 lines) and call the new ui_layout_enum_menu() from there.
Of course you'd also call ui_text_icon_width() then instead of duplicating its code.

3431–3432

Would still suggest to commit this separately.

This revision now requires changes to proceed.Apr 4 2018, 5:54 PM

Update:

  • move the function to the interface_layout, add the header call.
  • Split the ui_text_icon_width into the call that is used in the function. Since using the original function doesn't work as it passes the layout which is not yet defined and doesn't produce valid results. Using block->curlayout doesn't either. So ui_icon_text_width_calc is introduced.
  • Removed the separator change.

Hope that I understood properly the review notes. :)

Vuk Gardašević (lijenstina) marked an inline comment as done.

Removed a non needed commented out line.