Re: [Engine-devel] ui code: possible problem in 'remove' confirmation dialog

From: "Vojtech Szocs" <vszocs(a)redhat.com&gt; To: "Daniel Erez" <derez(a)redhat.com&gt; Cc: "Einav Cohen" <ecohen(a)redhat.com&gt;, "Gilad Chaplik" <gchaplik(a)redhat.com&gt;, "Tomas Jelinek" <tjelinek(a)redhat.com&gt;, "Lior Vernia" <lvernia(a)redhat.com&gt;, "engine-devel" <engine-devel(a)ovirt.org&gt; Sent: Monday, October 21, 2013 12:55:05 PM Subject: Re: ui code: possible problem in 'remove' confirmation dialog ----- Original Message ----- > From: "Daniel Erez" <derez(a)redhat.com&gt; > To: "Einav Cohen" <ecohen(a)redhat.com&gt; > Cc: "Gilad Chaplik" <gchaplik(a)redhat.com&gt;, "Tomas Jelinek" > <tjelinek(a)redhat.com&gt;, "Vojtech Szocs" > <vszocs(a)redhat.com&gt;, "Lior Vernia" <lvernia(a)redhat.com&gt;, "engine-devel" > <engine-devel(a)ovirt.org&gt; > Sent: Friday, October 18, 2013 12:06:02 PM > Subject: Re: ui code: possible problem in 'remove' confirmation dialog > > > > ----- Original Message ----- > > From: "Einav Cohen" <ecohen(a)redhat.com&gt; > > To: "Daniel Erez" <derez(a)redhat.com&gt;, "Gilad Chaplik" > > <gchaplik(a)redhat.com&gt;, "Tomas Jelinek" <tjelinek(a)redhat.com&gt;, > > "Vojtech Szocs" <vszocs(a)redhat.com&gt;, "Lior Vernia" <lvernia(a)redhat.com&gt; > > Cc: "engine-devel" <engine-devel(a)ovirt.org&gt; > > Sent: Thursday, October 17, 2013 4:24:56 PM > > Subject: Re: ui code: possible problem in 'remove' confirmation dialog > > > > [apologies - previous e-mail sent prematurely by mistake] > > > > Hi, > > > > Looking at the current code: It seems that we cannot set the message > > within > > a > > 'remove' confirmation dialog if its HashName starts with "remove_" - it > > is > > being set "statically" to the "Are you sure you want to remove the > > following > > items?" [constants.removeConfirmationPopupMessage] message [1] > > > > I don't have a major problem with relying on the HashName for setting a > > *default* message [2] - but I think that not allowing to override this > > message if the developer chooses to do so is an incorrect behavior. > > > > I would like to change the behavior so that the user would be able to > > override the message displayed in that dialog, even if its hash-name is > > set to 'remove_'. > > > > thoughts? objections? > > > > ---- > > Thanks, > > Einav > > > > > > [1] From RemoveConfirmationPopupView.java, line 86: > > > > public void setMessage(String message) { > > if (getHashName() != null && > > getHashName().startsWith("remove_")) > > { > > //$NON-NLS-1$ > > super.setMessage(constants.removeConfirmationPopupMessage()); > > } else { > > super.setMessage(message); > > } > > } > > Not sure why we prevent override in this case, IIRC the current RemoveConfirmationPopupView.setMessage implementation reflects the assumption that all remove confirm dialogs [hashName=remove_*] have the same message, i.e. "Are you sure you want to remove the following items?". However, looking at VmListModel#remove (VM main tab / Remove button) it first sets [hashName=remove_virtual_machine] and then sets message like "Virtual Machine(s)". In practice, RemoveConfirmationPopupView.setMessage will still make the dialog message generic, i.e. "Are you sure ... following items?". > I guess it should probably be as simple as: > > public void setMessage(String message) { > super.setMessage(message != null ? message : > constants.removeConfirmationPopupMessage()); > } For that, we'd have to analyze and modify existing UiCommon models to call setMessage with appropriate value, i.e. VmListModel#remove currently yields RemoveConfirmationPopupView.setMessage("Virtual Machine(s)")

> > Just need to make sure that no one is currently overriding > it by mistake to avoid possible regressions. I think the general pattern used in UiCommon models when dealing with remove confirm dialogs is following: - set wnd hashName to "remove_<entity>" i.e. "remove_virtual_machine" - set wnd message to "<specific item(s)>" i.e. "Virtual Machine(s)" Alternative to Daniel's suggestion, following above mentioned pattern: if (message != null) { // expecting message to be "<specific item(s)>" // should yield "Are you sure you want to remove <specific item(s)>?" super.setMessage(messages.removeConfirmationPopupMessage(message)); } else { super.setMessage(message); }

> > > > > [2] in fact, I don't mind that this would be the default message for > > this dialog, even if its hash-name is not set to something that starts > > with 'remove_'. > > > > ----- Original Message ----- > > > From: "Einav Cohen" <ecohen(a)redhat.com&gt; > > > To: "Daniel Erez" <derez(a)redhat.com&gt;, "Gilad Chaplik" > > > <gchaplik(a)redhat.com&gt;, "Tomas Jelinek" <tjelinek(a)redhat.com&gt;, > > > "Vojtech Szocs" <vszocs(a)redhat.com&gt;, "Lior Vernia" <lvernia(a)redhat.com&gt; > > > Cc: "engine-devel" <engine-devel(a)ovirt.org&gt; > > > Sent: Thursday, October 17, 2013 9:20:07 AM > > > Subject: ui code: possible problem in 'remove' confirmation dialog > > > > > > Hi, > > > > > > Looking at the current code: It seems that we cannot set the message > > > within > > > a > > > 'remove' confirmation > > > dialog if its HashName starts with "remove_" - it is being set > > > "statically[1] > > > I don't have a major problem with relying on the HashName for setting a > > > *default* message (in fact, > > > I don't have a problem > > > > > > [1] From RemoveConfirmationPopupView.java, line 86: > > > > > > public void setMessage(String message) { > > > if (getHashName() != null && > > > getHashName().startsWith("remove_")) > > > { > > > //$NON-NLS-1$ > > > super.setMessage(constants.removeConfirmationPopupMessage()); > > > } else { > > > super.setMessage(message); > > > } > > > } > > > > > > ---- > > > Regards, > > > Einav Cohen Baum > > > RHEV-M Engineering - UX Team Manager > > > Red Hat, Inc. > > > 314 Littleton Road > > > Westford, MA 01886 > > > T [internal]: (81) 31046 > > > T [external]: (+1) 978 589 1046 > > > IRC: ecohen @ > > > - RHAT [internal]: #rhev-dev #boston #westford #tlv > > > - OFTC [external]: #ovirt > > > > > >