6:03 p.m.
[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(a)redhat.com>
To: "Einav Cohen" <ecohen(a)redhat.com>
Cc: "engine-devel" <engine-devel(a)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(a)redhat.com>
> To: "Vojtech Szocs" <vszocs(a)redhat.com>
> Cc: "Daniel Erez" <derez(a)redhat.com>, "engine-devel"
> <engine-devel(a)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(a)redhat.com>
> > Sent: Monday, October 21, 2013 8:57:16 AM
> >
> >
> > ----- Original Message -----
> > > From: "Daniel Erez" <derez(a)redhat.com>
> > > To: "Vojtech Szocs" <vszocs(a)redhat.com>
> > > Cc: "Einav Cohen" <ecohen(a)redhat.com>, "Gilad
Chaplik"
> > > <gchaplik(a)redhat.com>, "Tomas Jelinek"
<tjelinek(a)redhat.com>,
> > > "Lior Vernia" <lvernia(a)redhat.com>,
"engine-devel"
> > > <engine-devel(a)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(a)redhat.com>
> > > > To: "Daniel Erez" <derez(a)redhat.com>
> > > > Cc: "Einav Cohen" <ecohen(a)redhat.com>, "Gilad
Chaplik"
> > > > <gchaplik(a)redhat.com>, "Tomas Jelinek"
<tjelinek(a)redhat.com>,
> > > > "Lior Vernia" <lvernia(a)redhat.com>,
"engine-devel"
> > > > <engine-devel(a)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(a)redhat.com>
> > > > > To: "Einav Cohen" <ecohen(a)redhat.com>
> > > > > Cc: "Gilad Chaplik" <gchaplik(a)redhat.com>,
"Tomas Jelinek"
> > > > > <tjelinek(a)redhat.com>, "Vojtech Szocs"
> > > > > <vszocs(a)redhat.com>, "Lior Vernia"
<lvernia(a)redhat.com>,
> > > > > "engine-devel"
> > > > > <engine-devel(a)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(a)redhat.com>
> > > > > > To: "Daniel Erez" <derez(a)redhat.com>,
"Gilad Chaplik"
> > > > > > <gchaplik(a)redhat.com>, "Tomas Jelinek"
<tjelinek(a)redhat.com>,
> > > > > > "Vojtech Szocs" <vszocs(a)redhat.com>,
"Lior Vernia"
> > > > > > <lvernia(a)redhat.com>
> > > > > > Cc: "engine-devel"
<engine-devel(a)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(a)redhat.com>
> > > > > > > To: "Daniel Erez" <derez(a)redhat.com>,
"Gilad Chaplik"
> > > > > > > <gchaplik(a)redhat.com>, "Tomas Jelinek"
<tjelinek(a)redhat.com>,
> > > > > > > "Vojtech Szocs" <vszocs(a)redhat.com>,
"Lior Vernia"
> > > > > > > <lvernia(a)redhat.com>
> > > > > > > Cc: "engine-devel"
<engine-devel(a)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(a)ovirt.org
> > http://lists.ovirt.org/mailman/listinfo/engine-devel
> >
> >
> >
>
_______________________________________________
Engine-devel mailing list
Engine-devel(a)ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-devel