Details

Reviewers

Campbell Barton (campbellbarton)
Julian Eisel (Severin)

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

Event Timeline

Vuk Gardašević (lijenstina) created this revision.Nov 15 2017, 10:57 PM

Vuk Gardašević (lijenstina) edited the summary of this revision. (Show Details)Nov 15 2017, 11:14 PM

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.

Vuk Gardašević (lijenstina) edited the summary of this revision. (Show Details)Mar 27 2018, 10:20 PM

Vuk Gardašević (lijenstina) edited the summary of this revision. (Show Details)Mar 28 2018, 1:47 AM

Ray Molenkamp (LazyDodo) mentioned this in T54458: crash in ui_def_but_rna.Mar 29 2018, 2:32 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.

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.

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. :)

Removed a non needed commented out line.

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 10253

source/blender/editors/interface/interface.c

Tentative fix for T53257: Too many color management transforms to display in the menuNeeds ReviewPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 10253

source/blender/editors/interface/interface.c

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

Revision Contents
Changeset List