Maniphest T39234

popup menus behave poorly when they have not enough width for all their columns
Closed, Resolved
Actions

Assigned To
Bastien Montagne (mont29)
Authored By
Alexandros Alexandrou (alex1)
Mar 17 2014, 8:37 PM
Tags
  • User Interface
  • BF Blender
Subscribers
Alexandros Alexandrou (alex1)
Bastien Montagne (mont29)
Brecht Van Lommel (brecht)
Eneko Castresana Vara (ecv)

Description

System Information
Ubuntu 12.04

Blender Version
Broken: 2.70 RC 2

Short description of error
When "Undo History" popup is filled, some items are not shown because of insufficient screen space. I don't think it is a bug, but just an inconsistency.

Probable solution
In case items need more space than screen can provide them, a slider or an arrow could be added, so remaining items can be seen and selected.

Exact steps for others to reproduce the error

  1. Set "Undo Steps" in "User Preferences" to 64.
  2. Make 64 steps, of which at least one has a long name (so the problem is visible).
  3. Go to "Undo History" and you will see the following.

Revisions and Commits

rB Blender
rB09e5aa515648 Fix T39234: popup menus behave poorly when they have not enough width for all…
rBAC Blender Add-ons Contrib
rBAC09e5aa515648 Fix T39234: popup menus behave poorly when they have not enough width for all…

Event Timeline

Alexandros Alexandrou (alex1) created this task.Mar 17 2014, 8:37 PM
Alexandros Alexandrou (alex1) raised the priority of this task from to 90.
Alexandros Alexandrou (alex1) updated the task description.
Alexandros Alexandrou (alex1) added a project: User Interface.
Alexandros Alexandrou (alex1) edited a custom field.
Alexandros Alexandrou (alex1) added a subscriber: Alexandros Alexandrou (alex1).
Bastien Montagne (mont29) added a project: BF Blender.Mar 18 2014, 3:32 PM
Bastien Montagne (mont29) added a subscriber: Bastien Montagne (mont29).
Bastien Montagne (mont29) lowered the priority of this task from 90 to 30.Mar 18 2014, 3:41 PM

Can’t reproduce such issue error here, I just get up/down arrows…

What is your screen definition? looks *very* small (I tried here with a Blender window less than 800*600 and still could show the four columns, even with a quite long op name).

Alexandros Alexandrou (alex1) added a comment.EditedMar 18 2014, 7:23 PM

My screen resolution is 1280x800.

Sure, up/down arrows appear on menus with height exceeding Blender window height. But such arrows do not appear on "Undo History" popup, which was what I had pointed out. In fact, some items can't be accessed and an arrow or a slider is needed there.

Bastien Montagne (mont29) raised the priority of this task from 30 to Normal.Mar 19 2014, 8:28 PM

Ok, could reproduce it by setting a much bigger DPI size… We might enhance behavior of that kind of popup in two ways, imho:

  • Do not set same width for all columns!
  • Reduce number of columns when there is not enough width.

Will have a look at what we can do here.

Bastien Montagne (mont29) renamed this task from When "Undo History" popup is filled, some items are hidden to popup menus behave poorly when they have not enough width for all their columns.Mar 19 2014, 8:28 PM
Bastien Montagne (mont29) claimed this task.
Bastien Montagne (mont29) added a subscriber: Brecht Van Lommel (brecht).Apr 14 2014, 8:17 PM

Following patch "fixes" the issue by:

  • Not having constant width for all columns, but adapt each to its content's width;
  • Adapting undo' menu height to undo list length (so that we never have more than three columns).

It is still possible to get issues in extreme cases (small screen, high DPI size, long op names), but this should now be rare corner cases.

I also fixed a minor glitch with undo menu (first column had one item less than the others…).

@Brecht Van Lommel (brecht) would need your green light here before I commit, since column width change will also affect menus like modifiers or node sockets ones (imho it’s better now, wastes less space, but…). :)

P37 Fix T39234
1diff --git a/source/blender/editors/interface/interface.c b/source/blender/editors/interface/interface.c
2index 7e6e00b..5622288 100644
3--- a/source/blender/editors/interface/interface.c
4+++ b/source/blender/editors/interface/interface.c
5@@ -207,43 +207,41 @@ void ui_block_translate(uiBlock *block, int x, int y)
6 static void ui_text_bounds_block(uiBlock *block, float offset)
7 {
8 uiStyle *style = UI_GetStyle();
9- uiBut *bt;
10- int i = 0, j, x1addval = offset, nextcol;
11- int lastcol = 0, col = 0;
12-
13+ uiBut *bt, *init_col_bt, *col_bt;
14+ int i = 0, j, x1addval = offset;
15+
16 uiStyleFontSet(&style->widget);
17-
18- for (bt = block->buttons.first; bt; bt = bt->next) {
19+
20+ for (init_col_bt = bt = block->buttons.first; bt; bt = bt->next) {
21 if (!ELEM(bt->type, SEPR, SEPRLINE)) {
22 j = BLF_width(style->widget.uifont_id, bt->drawstr, sizeof(bt->drawstr));
23
24- if (j > i) i = j;
25+ if (j > i)
26+ i = j;
27 }
28
29- if (bt->next && bt->rect.xmin < bt->next->rect.xmin)
30- lastcol++;
31- }
32+ if (bt->next && bt->rect.xmin < bt->next->rect.xmin) {
33+ /* End of this column, and it’s not the last one. */
34+ for (col_bt = init_col_bt; col_bt->prev != bt; col_bt = col_bt->next) {
35+ col_bt->rect.xmin = x1addval;
36+ col_bt->rect.xmax = x1addval + i + block->bounds;
37
38- /* cope with multi collumns */
39- bt = block->buttons.first;
40- while (bt) {
41- nextcol = (bt->next && bt->rect.xmin < bt->next->rect.xmin);
42-
43- bt->rect.xmin = x1addval;
44- bt->rect.xmax = bt->rect.xmin + i + block->bounds;
45-
46- if (col == lastcol) {
47- bt->rect.xmax = max_ff(bt->rect.xmax, offset + block->minbounds);
48- }
49+ ui_check_but(col_bt); /* clips text again */
50+ }
51
52- ui_check_but(bt); /* clips text again */
53-
54- if (nextcol) {
55+ /* And we prepare next column. */
56 x1addval += i + block->bounds;
57- col++;
58+ i = 0;
59+ init_col_bt = col_bt;
60 }
61-
62- bt = bt->next;
63+ }
64+
65+ /* Last column. */
66+ for (col_bt = init_col_bt; col_bt; col_bt = col_bt->next) {
67+ col_bt->rect.xmin = x1addval;
68+ col_bt->rect.xmax = max_ff(x1addval + i + block->bounds, offset + block->minbounds);
69+
70+ ui_check_but(col_bt); /* clips text again */
71 }
72 }
73
74diff --git a/source/blender/editors/util/undo.c b/source/blender/editors/util/undo.c
75index 434f1e3..733b45f 100644
76--- a/source/blender/editors/util/undo.c
77+++ b/source/blender/editors/util/undo.c
78@@ -533,14 +533,20 @@ static int undo_history_invoke(bContext *C, wmOperator *op, const wmEvent *UNUSE
79 uiLayout *layout = uiPupMenuLayout(pup);
80 uiLayout *split = uiLayoutSplit(layout, 0.0f, false);
81 uiLayout *column = NULL;
82+ const int col_size = 20 + totitem / 12;
83 int i, c;
84+ bool add_col = true;
85
86- for (c = 0, i = totitem - 1; i >= 0; i--, c++) {
87- if ( (c % 20) == 0)
88+ for (c = 0, i = totitem; i--;) {
89+ if (add_col && !(c % col_size)) {
90 column = uiLayoutColumn(split, false);
91- if (item[i].identifier)
92+ add_col = false;
93+ }
94+ if (item[i].identifier) {
95 uiItemIntO(column, item[i].name, item[i].icon, op->type->idname, "item", item[i].value);
96-
97+ ++c;
98+ add_col = true;
99+ }
100 }
101
102 MEM_freeN(item);

Brecht Van Lommel (brecht) added a comment.Apr 15 2014, 3:32 PM

This patch doesn't apply here, it has spaces instead of tabs and in a way that makes it difficult to search and replace them.

The code looks ok to me though, just not sure how the result looks.

Bastien Montagne (mont29) added a comment.Apr 15 2014, 4:39 PM

@Brecht Van Lommel (brecht) ack, sorry, copy/paste directly from terminal is not a good idea, after all… :/

This one should be OK:

P38 Fix T39234
1diff --git a/source/blender/editors/interface/interface.c b/source/blender/editors/interface/interface.c
2index 7e6e00b..5622288 100644
3--- a/source/blender/editors/interface/interface.c
4+++ b/source/blender/editors/interface/interface.c
5@@ -207,43 +207,41 @@ void ui_block_translate(uiBlock *block, int x, int y)
6 static void ui_text_bounds_block(uiBlock *block, float offset)
7 {
8 uiStyle *style = UI_GetStyle();
9- uiBut *bt;
10- int i = 0, j, x1addval = offset, nextcol;
11- int lastcol = 0, col = 0;
12-
13+ uiBut *bt, *init_col_bt, *col_bt;
14+ int i = 0, j, x1addval = offset;
15+
16 uiStyleFontSet(&style->widget);
17-
18- for (bt = block->buttons.first; bt; bt = bt->next) {
19+
20+ for (init_col_bt = bt = block->buttons.first; bt; bt = bt->next) {
21 if (!ELEM(bt->type, SEPR, SEPRLINE)) {
22 j = BLF_width(style->widget.uifont_id, bt->drawstr, sizeof(bt->drawstr));
23
24- if (j > i) i = j;
25+ if (j > i)
26+ i = j;
27 }
28
29- if (bt->next && bt->rect.xmin < bt->next->rect.xmin)
30- lastcol++;
31- }
32+ if (bt->next && bt->rect.xmin < bt->next->rect.xmin) {
33+ /* End of this column, and it’s not the last one. */
34+ for (col_bt = init_col_bt; col_bt->prev != bt; col_bt = col_bt->next) {
35+ col_bt->rect.xmin = x1addval;
36+ col_bt->rect.xmax = x1addval + i + block->bounds;
37
38- /* cope with multi collumns */
39- bt = block->buttons.first;
40- while (bt) {
41- nextcol = (bt->next && bt->rect.xmin < bt->next->rect.xmin);
42-
43- bt->rect.xmin = x1addval;
44- bt->rect.xmax = bt->rect.xmin + i + block->bounds;
45-
46- if (col == lastcol) {
47- bt->rect.xmax = max_ff(bt->rect.xmax, offset + block->minbounds);
48- }
49+ ui_check_but(col_bt); /* clips text again */
50+ }
51
52- ui_check_but(bt); /* clips text again */
53-
54- if (nextcol) {
55+ /* And we prepare next column. */
56 x1addval += i + block->bounds;
57- col++;
58+ i = 0;
59+ init_col_bt = col_bt;
60 }
61-
62- bt = bt->next;
63+ }
64+
65+ /* Last column. */
66+ for (col_bt = init_col_bt; col_bt; col_bt = col_bt->next) {
67+ col_bt->rect.xmin = x1addval;
68+ col_bt->rect.xmax = max_ff(x1addval + i + block->bounds, offset + block->minbounds);
69+
70+ ui_check_but(col_bt); /* clips text again */
71 }
72 }
73
74diff --git a/source/blender/editors/util/undo.c b/source/blender/editors/util/undo.c
75index 434f1e3..733b45f 100644
76--- a/source/blender/editors/util/undo.c
77+++ b/source/blender/editors/util/undo.c
78@@ -533,14 +533,20 @@ static int undo_history_invoke(bContext *C, wmOperator *op, const wmEvent *UNUSE
79 uiLayout *layout = uiPupMenuLayout(pup);
80 uiLayout *split = uiLayoutSplit(layout, 0.0f, false);
81 uiLayout *column = NULL;
82+ const int col_size = 20 + totitem / 12;
83 int i, c;
84+ bool add_col = true;
85
86- for (c = 0, i = totitem - 1; i >= 0; i--, c++) {
87- if ( (c % 20) == 0)
88+ for (c = 0, i = totitem; i--;) {
89+ if (add_col && !(c % col_size)) {
90 column = uiLayoutColumn(split, false);
91- if (item[i].identifier)
92+ add_col = false;
93+ }
94+ if (item[i].identifier) {
95 uiItemIntO(column, item[i].name, item[i].icon, op->type->idname, "item", item[i].value);
96-
97+ ++c;
98+ add_col = true;
99+ }
100 }
101
102 MEM_freeN(item);

Brecht Van Lommel (brecht) added a comment.Apr 15 2014, 4:44 PM

Thanks, looks good to me.

Bastien Montagne (mont29) edited this Maniphest Task.Apr 15 2014, 4:57 PM
Bastien Montagne (mont29) changed the task status from Unknown Status to Resolved.Apr 15 2014, 4:57 PM

Closed by commit rB09e5aa515648.

Bastien Montagne (mont29) edited this Maniphest Task.May 12 2014, 4:48 PM
Eneko Castresana Vara (ecv) changed the task status from Resolved to Unknown Status.EditedJun 22 2016, 8:01 PM
Eneko Castresana Vara (ecv) added a subscriber: Eneko Castresana Vara (ecv).

Hello, this is still broken in 2.77a

I tried to reproduce it on purpose a number of times and I couldn't get it. However after working for a while it will eventually mess up.

My setup: Windows 7, two monitors both at 1024x768

Feel free to ask for any other specifics or actions.

Cheers,
Eneko

Bastien Montagne (mont29) changed the task status from Unknown Status to Resolved.Jun 22 2016, 9:38 PM

Please do not resurrect mummified reports like that… If you believe this is still valid, then open a new report and optionally add a link/reference to this one in it (typing T39234 would be enough).

Further more, as explained in closing commit, we cannot tackle all possible corner cases, Blender is not exactly designed to be used on a smartphone screen e.g.