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

Vojtech Szocs vszocs at redhat.com
Mon Oct 21 12:57:16 UTC 2013 

----- 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
> > > > > 
> > > > 
> > > 
> > 
>

More information about the Engine-devel mailing list