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

Tue Jun 9 16:30:41 UTC 2015

On 06/09/2015 11:15 AM, Jiri Vanek wrote:
> 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

When I was writing changelog, i realized - this class implements DialogResult, is in package 
dialogresults. Are you sure you wont also suffix to most of the classes in this packages - suffix 
DialogResult ?

If I was "does not metter' this morning, I'm against right now ;(

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