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

we *must* be able to specify full remove confirm msg only - we should not use message "particles", as they cause translation-hell (sigular vs. plural, etc.) - we had multiple problems with that in the past - that's why we changed the message here from "are you sure ... <object-type>?" to the more generic "are you sure ... items?" [http://gerrit.ovirt.org/#/c/14621] thanks. > ----- Original Message ----- > From: "Vojtech Szocs" <vszocs(a)redhat.com&gt; > Sent: Monday, October 21, 2013 8:57:16 AM > > > ----- Original Message ----- > > From: "Daniel Erez" <derez(a)redhat.com&gt; > > To: "Vojtech Szocs" <vszocs(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 2:12:22 PM > > Subject: Re: ui code: possible problem in 'remove' confirmation dialog > > > > > > > > ----- Original Message ----- > > > 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)") > > > > Isn't it a cleanup that should be done either way? > > (I assume this message isn't currently used on any context...). > > Yes, I guess some sort of cleanup will be required anyway. > > > > But indeed, it will require making sure that no one is > > currently overriding the setMessage() by mistake to > > avoid such possible regressions. > > > > > > > > > > > > > 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); > > > } > > > > What about default value (when message is null)? > > The setMessage code I've mentioned above was based on assumption that > UiCommon models such as VmListModel will always follow convention like > removeConfirmDialogModel.setMessage("item(s)") .. but maybe such assumption > is too strong. > > In general, you're right, there should be some default value handling > included (I forgot about this, sorry). > > > Do you mean we should add another abstract class in > > the hierarchy that would default it to > > "Are you sure you want to remove the following items?". > > [btw, we might have some localization issues with this > > pattern as we've encountered with 'no items to display'; > > so just need to check it out]. > > OK, in that case we can follow Daniel's suggestion (setMessage code that just > delegates to super.setMessage unless message == null for which there'd be > some default msg), along with updating relevant UiCommon models (i.e. > strings used when calling setMessage). > > > > > > > > > > > > > > > > > > > > [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 > > > > > > > > > > > > > > > > > > > > > _______________________________________________ > Engine-devel mailing list > Engine-devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/engine-devel > > >