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

[Andrew Azores]

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

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 of what kind of dialog result value might 
be stateful, mutable, etc. Are they not just a way of encoding which 
button (and associated options eg "remember") was selected out of a set 
of presented 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... ?

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


-- 
Thanks,

Andrew Azores