[Engine-devel] ui code: possible problem in 'remove' confirmation dialog
Einav Cohen
ecohen at redhat.com
Mon Oct 21 15:03:08 UTC 2013
[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" <vszocs at redhat.com>
> To: "Einav Cohen" <ecohen at redhat.com>
> Cc: "engine-devel" <engine-devel at 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" <ecohen at redhat.com>
> > To: "Vojtech Szocs" <vszocs at redhat.com>
> > Cc: "Daniel Erez" <derez at redhat.com>, "engine-devel"
> > <engine-devel at 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" <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
> > >
> > >
> > >
> >
> _______________________________________________
> Engine-devel mailing list
> Engine-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/engine-devel
>
>
>
More information about the Devel
mailing list