----------------------------------------------------------- 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 > >
_______________________________________________ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/OpenSource-Dev Please read the policies before posting to keep unmoderated posting privileges
