[Engine-devel] ui code: possible problem in 'remove' confirmation dialog
Einav Cohen
ecohen at redhat.com
Mon Oct 21 13:28:56 UTC 2013
> 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.
> ----- Original Message -----
> From: "Vojtech Szocs" <vszocs at redhat.com>
> Sent: Monday, October 21, 2013 8:57:16 AM
>
>
> ----- Original Message -----
> > From: "Daniel Erez" <derez at redhat.com>
> > To: "Vojtech Szocs" <vszocs at redhat.com>
> > Cc: "Einav Cohen" <ecohen at redhat.com>, "Gilad Chaplik"
> > <gchaplik at redhat.com>, "Tomas Jelinek" <tjelinek at redhat.com>,
> > "Lior Vernia" <lvernia at redhat.com>, "engine-devel" <engine-devel at 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" <vszocs at redhat.com>
> > > To: "Daniel Erez" <derez at redhat.com>
> > > Cc: "Einav Cohen" <ecohen at redhat.com>, "Gilad Chaplik"
> > > <gchaplik at redhat.com>, "Tomas Jelinek" <tjelinek at redhat.com>,
> > > "Lior Vernia" <lvernia at redhat.com>, "engine-devel"
> > > <engine-devel at 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" <derez at redhat.com>
> > > > To: "Einav Cohen" <ecohen at redhat.com>
> > > > Cc: "Gilad Chaplik" <gchaplik at redhat.com>, "Tomas Jelinek"
> > > > <tjelinek at redhat.com>, "Vojtech Szocs"
> > > > <vszocs at redhat.com>, "Lior Vernia" <lvernia at redhat.com>, "engine-devel"
> > > > <engine-devel at 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" <ecohen at redhat.com>
> > > > > To: "Daniel Erez" <derez at redhat.com>, "Gilad Chaplik"
> > > > > <gchaplik at redhat.com>, "Tomas Jelinek" <tjelinek at redhat.com>,
> > > > > "Vojtech Szocs" <vszocs at redhat.com>, "Lior Vernia"
> > > > > <lvernia at redhat.com>
> > > > > Cc: "engine-devel" <engine-devel at 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" <ecohen at redhat.com>
> > > > > > To: "Daniel Erez" <derez at redhat.com>, "Gilad Chaplik"
> > > > > > <gchaplik at redhat.com>, "Tomas Jelinek" <tjelinek at redhat.com>,
> > > > > > "Vojtech Szocs" <vszocs at redhat.com>, "Lior Vernia"
> > > > > > <lvernia at redhat.com>
> > > > > > Cc: "engine-devel" <engine-devel at 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 at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/engine-devel
>
>
>
More information about the Engine-devel
mailing list