[opensource-dev] Review Request: STORM-1844: Block and Ignore buttons on llDialog() menus overlap when there is only one button

[Lance Corrimal]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/585/#review1226
-----------------------------------------------------------

Ship it!


Ship It!

- Lance Corrimal


On June 18, 2012, 2:40 a.m., MartinRJ Fayray wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/585/
> -----------------------------------------------------------
> 
> (Updated June 18, 2012, 2:40 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Description
> -------
> 
> 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"
> 
> 
> This addresses bug STORM-1844.
> 
> 
> Diffs
> -----
> 
>   indra/newview/lltoastnotifypanel.cpp 29143d1fc6fa 
> 
> Diff: http://codereview.secondlife.com/r/585/diff/diff
> 
> 
> Testing
> -------
> 
> Testing repository: https://bitbucket.org/MartinRJ/storm-1844
> 
> Looking for feedback!
> 
> 
> Thanks,
> 
> MartinRJ Fayray
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20120618/fb3bae02/attachment-0001.htm