[top posting] opened a bugzilla in order to track this issue: Bug 1021584 - message in the 'remove' confirmation dialog cannot be overridden in some cases [https://bugzilla.redhat.com/show_bug.cgi?id=1021584]
many thanks, everyone, for your input - I highly appreciate it! ---- Regards, Einav ----- Original Message ----- > From: "Vojtech Szocs" <vsz...@redhat.com> > To: "Einav Cohen" <eco...@redhat.com> > Cc: "engine-devel" <engine-devel@ovirt.org> > Sent: Monday, October 21, 2013 9:39:04 AM > Subject: Re: [Engine-devel] ui code: possible problem in 'remove' > confirmation dialog > > > ----- Original Message ----- > > From: "Einav Cohen" <eco...@redhat.com> > > To: "Vojtech Szocs" <vsz...@redhat.com> > > Cc: "Daniel Erez" <de...@redhat.com>, "engine-devel" > > <engine-devel@ovirt.org> > > Sent: Monday, October 21, 2013 3:28:56 PM > > Subject: Re: [Engine-devel] ui code: possible problem in 'remove' > > confirmation dialog > > > > > For me, the question is if we want to give models the ability to specify > > > full > > > remove confirm msg (i.e. "Are you sure...") or just the item part of it > > > (i.e. "Virtual Machine(s)"). > > > > > > >From localization perspective, full remove confirm msg seems more > > > >flexible > > > >but will add more msg keys, however as you wrote we'd need some more > > > >changes in UiCommon models anyway. > > > > 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 Einav, now it's clear to me. I think we can go with Daniel's > suggestion, in general we need to inspect/update all setMessage calls on > ConfirmationModel instances. > > > > > thanks. > > > > > ----- Original Message ----- > > > From: "Vojtech Szocs" <vsz...@redhat.com> > > > Sent: Monday, October 21, 2013 8:57:16 AM > > > > > > > > > ----- Original Message ----- > > > > From: "Daniel Erez" <de...@redhat.com> > > > > To: "Vojtech Szocs" <vsz...@redhat.com> > > > > Cc: "Einav Cohen" <eco...@redhat.com>, "Gilad Chaplik" > > > > <gchap...@redhat.com>, "Tomas Jelinek" <tjeli...@redhat.com>, > > > > "Lior Vernia" <lver...@redhat.com>, "engine-devel" > > > > <engine-devel@ovirt.org> > > > > Sent: Monday, October 21, 2013 2:12:22 PM > > > > Subject: Re: ui code: possible problem in 'remove' confirmation dialog > > > > > > > > > > > > > > > > ----- Original Message ----- > > > > > From: "Vojtech Szocs" <vsz...@redhat.com> > > > > > To: "Daniel Erez" <de...@redhat.com> > > > > > Cc: "Einav Cohen" <eco...@redhat.com>, "Gilad Chaplik" > > > > > <gchap...@redhat.com>, "Tomas Jelinek" <tjeli...@redhat.com>, > > > > > "Lior Vernia" <lver...@redhat.com>, "engine-devel" > > > > > <engine-devel@ovirt.org> > > > > > Sent: Monday, October 21, 2013 12:55:05 PM > > > > > Subject: Re: ui code: possible problem in 'remove' confirmation > > > > > dialog > > > > > > > > > > > > > > > > > > > > ----- Original Message ----- > > > > > > From: "Daniel Erez" <de...@redhat.com> > > > > > > To: "Einav Cohen" <eco...@redhat.com> > > > > > > Cc: "Gilad Chaplik" <gchap...@redhat.com>, "Tomas Jelinek" > > > > > > <tjeli...@redhat.com>, "Vojtech Szocs" > > > > > > <vsz...@redhat.com>, "Lior Vernia" <lver...@redhat.com>, > > > > > > "engine-devel" > > > > > > <engine-devel@ovirt.org> > > > > > > Sent: Friday, October 18, 2013 12:06:02 PM > > > > > > Subject: Re: ui code: possible problem in 'remove' confirmation > > > > > > dialog > > > > > > > > > > > > > > > > > > > > > > > > ----- Original Message ----- > > > > > > > From: "Einav Cohen" <eco...@redhat.com> > > > > > > > To: "Daniel Erez" <de...@redhat.com>, "Gilad Chaplik" > > > > > > > <gchap...@redhat.com>, "Tomas Jelinek" <tjeli...@redhat.com>, > > > > > > > "Vojtech Szocs" <vsz...@redhat.com>, "Lior Vernia" > > > > > > > <lver...@redhat.com> > > > > > > > Cc: "engine-devel" <engine-devel@ovirt.org> > > > > > > > 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. > > > > > > For me, the question is if we want to give models the ability to specify > > > full > > > remove confirm msg (i.e. "Are you sure...") or just the item part of it > > > (i.e. "Virtual Machine(s)"). > > > > > > >From localization perspective, full remove confirm msg seems more > > > >flexible > > > >but will add more msg keys, however as you wrote we'd need some more > > > >changes in UiCommon models 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" <eco...@redhat.com> > > > > > > > > To: "Daniel Erez" <de...@redhat.com>, "Gilad Chaplik" > > > > > > > > <gchap...@redhat.com>, "Tomas Jelinek" <tjeli...@redhat.com>, > > > > > > > > "Vojtech Szocs" <vsz...@redhat.com>, "Lior Vernia" > > > > > > > > <lver...@redhat.com> > > > > > > > > Cc: "engine-devel" <engine-devel@ovirt.org> > > > > > > > > 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@ovirt.org > > > http://lists.ovirt.org/mailman/listinfo/engine-devel > > > > > > > > > > > > _______________________________________________ > Engine-devel mailing list > Engine-devel@ovirt.org > http://lists.ovirt.org/mailman/listinfo/engine-devel > > > _______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel