All submissions to this site are governed by the Second Life Viewer Contribution Agreement. By submitting patches and other information using this site, you acknowledge that you have read, understood, and agreed to those terms.

Review Board 1.6.11

Welcome to the Second Life Viewer Code Review tool.
See the documentation on our wiki for how to use this site.

STORM-1844: Block and Ignore buttons on llDialog() menus overlap when there is only one button

Review Request #585 - Created June 18, 2012 and submitted

MartinRJ Fayray Reviewers
3.3.2 viewer
STORM-1844
None viewer-release
Removed unnecessary code, replaced unapplicable use of h_pad with a default padding of 4 * HPAD (because I don't see why the padding should depend on the number of ALL buttons including scripted, block and ignore button, and why the block and ignore button should move at all, it acts strange - which can be seen best in case of only one button where h_pad behaves quite weird, and I don't see why one would try approximating max_width with h_pad, it doesn't need to test for an overlap - I should not even get into that if-block ever. It looks to me that during design of the ignore/block buttons someone was planning to put the ignore button into the same - last - row as the dialog button when there was enough space).
I believe in the actual version of viewer-release block and ignore would probably always overlap if you change the default button size to something greater than 100.

line 356 says in a comment: //assume that h_pad far less than BUTTON_WIDTH
which does NOT work when there is only one button.

ln 341 or the copy-pasted code in ln 358 can in many cases cause that the ignore button ALWAYS leaves the notification-window, because h_pad is only useful to calculate the padding between the scripted dialog buttons, it doesn't really apply to the block/ignore buttons, I believe.
So we can remove ln 342-345 and the copypasted lines 359-362 when we just prevent that the case ever occurs.
When we enter the if-block in ln 342, the if-block in ln 359 should automatically get executed also (when the ignore-button is moved because it otherwise would overlap with the window frame, then the block-button must be moved as well) and move by the size/width of the ignore button - but right now there is no case at all where the (wrong) if-block from ln 359-362 gets executed - in the current viewer.
Ln. 359 SHOULD BE - (if we would leave the current behaviour) - like: if (mute_btn_left + mute_btn_rect.getWidth() > max_width - ignore_btn_width - "padding-between-ignore-and-block") - 
and ln. 361 SHOULD BE like: mute_btn_left = max_width - mute_btn_rect.getWidth() - 2 * HPAD - ignore_btn_width - "padding-between-ignore-and-block";


An example for the current misbehaviour (that the button gets out of the visible area of the notification window frame) would be if we would extend the window size to 360 (max_width = 360), then buttons-per-row would become '4', and ignore_btn_left would become increased by 90 - (assuming that h_pad doesn't change to much), then the ignore-button would in all cases (1 dialog-button, two dialog buttons, n- dialog buttons) leave the area of the notifications window to the right, and the block-button in some cases too.

Another example would be if we changed the default size of the button to 107 (current default is 90). Then the block- and ignore-button would probably always overlap (the block and ignore-buttons would be on the same spot, I believe).


It seems that it was presumed that 2*h_pad + 3 buttons = maximum width. The old code can lead to many little bugs if the maximum width changes at some future point.

buttons_per_row * BUTTON_WIDTH + (buttons_per_row	- 1) * h_pad - was an approximation to max_width, which I have replaced with "max_width"
Testing repository: https://bitbucket.org/MartinRJ/storm-1844

Looking for feedback!
Review request changed
Updated (June 18, 2012, 11:02 a.m.)
  • changed from pending to submitted