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

[Daniel Erez]

----- Original Message -----
> From: "Einav Cohen" <ecohen at redhat.com>
> To: "Daniel Erez" <derez at redhat.com>, "Greg Sheremeta" <gshereme at redhat.com>
> Cc: "engine-devel" <engine-devel at ovirt.org>
> Sent: Friday, October 18, 2013 4:44:25 PM
> Subject: Re: [Engine-devel] ui code: possible problem in 'remove'	confirmation dialog
> 
> > ----- Original Message -----
> > From: "Daniel Erez" <derez at redhat.com>
> > Sent: Friday, October 18, 2013 6:06:02 AM
> > 
> > 
> > 
> > ----- 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,
> > I guess it should probably be as simple as:
> > 
> >     public void setMessage(String message) {
> >         super.setMessage(message != null ? message :
> >         constants.removeConfirmationPopupMessage());
> >     }
> 
> the above would mean that in order to utilize the default message,
> we would need to explicitly call to 'setMessage()' with 'null' as
> a parameter, doesn't it?

IINM, setMessage is called explicitly with model's getMessage
upon popup revealing. So we don't need to call setMessage with
null again.

> 
> maybe we should do the following instead:
> 
>      public void setMessage(String message) {
>          super.setMessage(message);
>      }
> 
>      public String getMessage() {
>          return super.getMessage() != null ? super.getMessage() :
>          constants.removeConfirmationPopupMessage();
>      }
> 
> [of course, the above means that if someone would explicitly sets
> the message to 'null', the dialog would end up with the default
> message; but I think it is acceptable. so:
> - if you want the default message: don't set message / set it to 'null'

Just don't set a message, no need to set it to null.

> - if you want an empty message: set message to an empty string
> - if you want a message other than default: set it to whatever you want]
> 
> ?

+1

> 
> > 
> > Just need to make sure that no one is currently overriding
> > it by mistake to avoid possible regressions.
> > 
> > > 
> > > [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
> > 
> > 
> > 
>