[rfc][icedtea-web] rememberable dialogues general solution - impl part 1 - safe types returned from getUserResponse

[Jiri Vanek]

On 06/08/2015 10:11 PM, Andrew Azores wrote:
> On 08/06/15 10:32 AM, Jiri Vanek wrote:
>> On 06/08/2015 04:04 PM, Andrew Azores wrote:
>>> Hi,
>>>
>>> On 08/06/15 06:18 AM, Jiri Vanek wrote:
>>>> On 06/05/2015 05:36 PM, Andrew Azores wrote:
>>>>> Hi! Generally liking where this patch is going.
>>>>>
>>>>> Jiri Vanek <jvanek at ...> writes:
>>>>>
>>>>>>
>>>>>> hi!
>>>>>>
>>>>>> So this is first part.
>>>>>> Tho most nasty and error prone.
>>>>>>
>>>>>> It is changing type of securityMEssage.response from Object to regular
>>>>> interface, and is providing
>>>>>> useful implementations of this interface.
>>>>>>
>>>>>> The legacy object was integer at first, and was used to preselect button,
>>>>> and tro return id of
>>>>>> pressed button. LAter was missuesd and returned yes/no/always/never even
>>>>> later sanbox was added and
>>>>>> last drop was complex result about shortcuts.
>>>>>
>>>>> I much prefer your new approach, obviously much better than ints and Object[]s.
>>>>>
>>>>>>
>>>>>> In this patch, there are few wrappers for types of yAnNS, which contains
>>>>> also remember logic. This
>>>>>> will be fixed in another changeset, and those wrappers will be gone.
>>>>>
>>>>> Does this mean the Yes, No, YesNo, YesNoCancel, etc. classes? And "Garbage"?
>>>>
>>>> Garabge for sure:)
>>>>
>>>> But not so YEs, No, YesNo - they are what dfialog can return, and what is returning. What you
>>>> would like to replace them with?
>>>> If dialog returns more htne just yes/nos/sandbox/any_button then they can be rused in same way
>>>> as AccessWarningPaneComplexReturn do.
>>>>
>>>> Thoughts?
>>>
>>> I don't know, I don't have anything in particular in mind that this might be replaced with. I
>>> understand what these types signify but it just seems like a cumbersome way to have to define
>>> them, but I'm not sure how else it might be done better. Could you perhaps rename all the
>>> Yes/YesNo/YesNoCancel classes and add "Result" or something similar to their names? "new Yes()"
>>> is a very odd looking snippet :)
>>
>> Nope. They all are implementing DialogResult. Thats better then naming convention, isnt it?
>
> Is there a reason you can't do both? "YesNo" still looks like "YesNo" in the source, even if it
> implements DialogResult.
>
> static YesNoDialogResult promptUser()

Well AccessWarningPaneComplexReturn have also "return" in name. so I guess yes.

if you insists (and you seems to be) then why not - I will change them to *DialogResutl as you wish, 
and also AccessWarningPaneComplexReturn will change to AccessWarningDialogResult

I will do it as another changest, otherwise I wil ened to adapt whole stack of five not exactly 
smallest patches. ok?
>
> is a method signature that tells me it's going to present a dialog asking the user what they want to
> do, and we expect the user to choose from buttons labeled Yes or No, and we get back some kind of
> structure that encapsulates the user's choice. Then it also wouldn't be surprising if this structure
> also contained details like "remember this decision".
>
> static YesNo promptUser()
>
> is a method signature that looks like we're going to ask the user a yes/no question somehow - maybe
> with a dialog box, maybe stdin, maybe carrier pigeon - and we've forgotten Java comes with booleans.
>
>>>
>>> It also seems to me like there's no sense to individual instances of these... "Yes a = new Yes()"
>>> and "Yes b = new Yes()" are just two different instances of stateless things that should always
>>> compare as equal, right? The other classes eg YesNo have factory methods for obtaining instances
>>> anyway, so why not standardize each of these classes on using factory methods and have each
>>> factory method return a reference to the same object every time, whenever possible (ie just not
>>> for the readValue case)? In other words, define singletons for each value of each return type. I
>>> think this might make the intention of these types a little clearer, and also might help avoid
>>> gotchas like trying to compare the returns of two
>>> dialogs.
>>
>> Well.. no. You can not guarantee for any possible DialogResult, that it will behave like that. The
>> part where it canbe guaranteed -  is hte enum - YES, NO...SANDBOX... So it is "singleton as you
>> are suggesting"
>
> Can you give me an example of a DialogResult that might not behave this way? I'm struggling to think \

AccessWarningPaneComplexReturn :)

> of what kind of dialog result value might be stateful, mutable, etc. Are they not just a way of
It can be made immutable, but imho it is not necessary.
> encoding which button (and associated options eg "remember") was selected out of a set of presented

Remember is not associated option. It is something hidden inside dialog mehchanism. (See tech 
preview diagram. Maybe it leaked in Impl somewhere in test preview, but it hsould not, and is fixed 
in cleaned extracted patch)

> options? Anyway, it's not very important to use singletons here, it just seemed to me that doing so
> might just make semantic sense. But maybe there will be too many combinations of result types and
> associated options and enumerating all of the values of the types will become cumbersome - Yes, No,
> Sandbox, Cancel, Yes and Remember, No and Remember, Sandbox and Remember...
>
>> The classes around, have a bit different  - especially to guarantee that nothing else willl be
>> passed in.
>
> I'm not clear on what this means... ?

Generally we can have only those Primitves enum and no wrapping types.  But I wonted to establish 
more logic around it
  1) the type is reposnisble for its writing/readin (not that soething else is writing it)
  2) since begging it is clear what is what dialog returns
  3) the wrapper validates its methods. So dialog which should return yesno will not suddenly 
unexpectedly  recieve/return cancel.
    - I would like to have something a bit better here then I actually have, but have not found 
something suitable :(
  4) support of more complex returns (And keep them traceable via interface in code)
>
>>
>> But yo are deffinitley right with missing equals. I already spotted it at friday, when I was
>> writing unittests for it. I will add equals+hash codes. They are  trivbial anyway all except
>> AccessWarningPaneComplexReturn(which is still simpel enough)
>
> This is the more important aspect of what doing things as singletons would address, so this is good
> enough IMO.
>
>>>
>>> If you didn't have to define each of these type as extending a common base type then it would be
>>> much nicer to just make each of these into an enum, but unfortunately that's not possible. I
>>> don't think that the lazy init matters here and thread safety doesn't seem like a concern either,
>>> though, so private static final instances with factory methods to return references to them still
>>> seems alright to me.
>>
>> I was thinking about singletons inside YesNo.no()/yes() and similar. I abandoned them, because
>> you can read/write those to/from string. So it seemed better to me just return new instance
>> instead of switch, which will determine instance.
>> again, this is bringing troubles in case of more compelx types.
>>
>>>
>>>>>
>>>>>>
>>>>>> Approach of this patch i slightly different, less verbose, with a lot of
>>>>> shared code and different
>>>>>> typesafety. I hope it is generally better. If somebody found tech-review's
>>>>> solution better, or
>>>>>> generally have better ideas, pelase let me know.
>>>>>>
  J.
>>>>>>
>>>>>> Attachment (makeTypesOfSecurityDialogMessageReturnValueSafe.patch):
>>>>> text/x-patch, 84 KiB
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Andrew Azores
>>>>>
>>>>
>>>
>>>
>>
>
>