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

[Vojtech Szocs]

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

> 
> 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);
    }

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