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!

Diff revision 1 (Latest)

  1. indra/newview/lltoastnotifypanel.cpp: Loading...
indra/newview/lltoastnotifypanel.cpp
Revision 29143d1fc6fa New Change
... 326 lines hidden [Expand]
void LLToastNotifyPanel::updateButtonsLayout(const std::vector<index_button_pair_t>& buttons, S32 h_pad)
327
			bottom_offset += (BTN_HEIGHT + VPAD);
327
			bottom_offset += (BTN_HEIGHT + VPAD);
328
		}
328
		}
329
		//we arrange buttons from bottom to top for backward support of old script
329
		//we arrange buttons from bottom to top for backward support of old script
330
		btn_rect.setOriginAndSize(left, bottom_offset, btn_rect.getWidth(),	btn_rect.getHeight());
330
		btn_rect.setOriginAndSize(left, bottom_offset, btn_rect.getWidth(),	btn_rect.getHeight());
331
		btn->setRect(btn_rect);
331
		btn->setRect(btn_rect);
332
		left = btn_rect.mLeft + btn_rect.getWidth() + h_pad;
332
		left = btn_rect.mLeft + btn_rect.getWidth() + h_pad;
333
		mControlPanel->addChild(btn, -1);
333
		mControlPanel->addChild(btn, -1);
334
	}
334
	}
335

   
335

   
336
	U32 ignore_btn_width = 0;
336
	U32 ignore_btn_width = 0;

   
337
	U32 mute_btn_pad = 0;
337
	if (mIsScriptDialog && ignore_btn != NULL)
338
	if (mIsScriptDialog && ignore_btn != NULL)
338
	{
339
	{
339
		LLRect ignore_btn_rect(ignore_btn->getRect());
340
		LLRect ignore_btn_rect(ignore_btn->getRect());
340
		S32 buttons_per_row = max_width / BUTTON_WIDTH; //assume that h_pad far less than BUTTON_WIDTH
341
		S32 ignore_btn_left = max_width - ignore_btn_rect.getWidth();
341
		S32 ignore_btn_left = buttons_per_row * BUTTON_WIDTH + (buttons_per_row	- 1) * h_pad - ignore_btn_rect.getWidth();

   
342
		if (ignore_btn_left + ignore_btn_rect.getWidth() > max_width)// make sure that the ignore button is in panel

   
343
		{

   
344
			ignore_btn_left = max_width - ignore_btn_rect.getWidth() - 2 * HPAD;

   
345
		}

   
346
		ignore_btn_rect.setOriginAndSize(ignore_btn_left, BOTTOM_PAD,// always move ignore button at the bottom
342
		ignore_btn_rect.setOriginAndSize(ignore_btn_left, BOTTOM_PAD,// always move ignore button at the bottom
347
				ignore_btn_rect.getWidth(), ignore_btn_rect.getHeight());
343
				ignore_btn_rect.getWidth(), ignore_btn_rect.getHeight());
348
		ignore_btn->setRect(ignore_btn_rect);
344
		ignore_btn->setRect(ignore_btn_rect);
349
		ignore_btn_width = ignore_btn_rect.getWidth();
345
		ignore_btn_width = ignore_btn_rect.getWidth();
350
		mControlPanel->addChild(ignore_btn, -1);
346
		mControlPanel->addChild(ignore_btn, -1);

   
347
		mute_btn_pad = 4 * HPAD; //only use a 4 * HPAD padding if an ignore button exists
351
	}
348
	}
352

   
349

   
353
	if (mIsScriptDialog && mute_btn != NULL)
350
	if (mIsScriptDialog && mute_btn != NULL)
354
	{
351
	{
355
		LLRect mute_btn_rect(mute_btn->getRect());
352
		LLRect mute_btn_rect(mute_btn->getRect());
356
		S32 buttons_per_row = max_width / BUTTON_WIDTH; //assume that h_pad far less than BUTTON_WIDTH

   
357
		// Place mute (Block) button to the left of the ignore button.
353
		// Place mute (Block) button to the left of the ignore button.
358
		S32 mute_btn_left = buttons_per_row * BUTTON_WIDTH + (buttons_per_row	- 1) * h_pad - mute_btn_rect.getWidth() - ignore_btn_width - (h_pad / 2);
354
		S32 mute_btn_left = max_width - mute_btn_rect.getWidth() - ignore_btn_width - mute_btn_pad;
359
		if (mute_btn_left + mute_btn_rect.getWidth() > max_width) // make sure that the mute button is in panel

   
360
		{

   
361
			mute_btn_left = max_width - mute_btn_rect.getWidth() - 2 * HPAD;

   
362
		}

   
363
		mute_btn_rect.setOriginAndSize(mute_btn_left, BOTTOM_PAD,// always move mute button at the bottom
355
		mute_btn_rect.setOriginAndSize(mute_btn_left, BOTTOM_PAD,// always move mute button at the bottom
364
				mute_btn_rect.getWidth(), mute_btn_rect.getHeight());
356
				mute_btn_rect.getWidth(), mute_btn_rect.getHeight());
365
		mute_btn->setRect(mute_btn_rect);
357
		mute_btn->setRect(mute_btn_rect);
366
		mControlPanel->addChild(mute_btn);
358
		mControlPanel->addChild(mute_btn);
367
	}
359
	}
368
}
360
}
369

   
361

   
370
void LLToastNotifyPanel::adjustPanelForScriptNotice(S32 button_panel_width, S32 button_panel_height)
362
void LLToastNotifyPanel::adjustPanelForScriptNotice(S32 button_panel_width, S32 button_panel_height)
371
{
363
{
372
	//adjust layout
364
	//adjust layout
... 230 lines hidden [Expand]
  1. indra/newview/lltoastnotifypanel.cpp: Loading...