[rfc][icedtea-web] PolicyEditor

[Andrew Azores]

On 02/20/2014 11:08 AM, Jiri Vanek wrote:
> On 02/19/2014 11:19 PM, Andrew Azores wrote:
>> On 02/13/2014 05:14 PM, Andrew Azores wrote:
>>> On 02/13/2014 08:55 AM, Jiri Vanek wrote:
>>>
>
>
> Hmm, I would be much more happy if those changes were add on top of 
> previous commited one.
>
>>>>
>>>> Hi!
>>>>
>>>> I have no general issues with this.Considering it was done n best 
>>>> mind;)
>>>> After reading it, I'm against making it separate project. I would 
>>>> much ratehr keep it in itew..
>>>> and suggestion .. add new luncher? next to itw-settings javaws.. 
>>>> SimplePolicyEditor?
>>>
>>> I didn't get around to that with this patch, but I'll work on that 
>>> next. Either a new launcher
>>> beside itweb-settings and javaws, or a standalone launcher as a 
>>> separate project... ;) I really do
>>> think this idea has some merit. Maybe the editor seems small and 
>>> simple enough now to stay within
>>> ITW, but I think it's already got enough utility to be a replacement 
>>> for PolicyTool for a lot of
>>> users. Most of whom probably are using IcedTea-Web anyway, but not 
>>> necessarily.
>>>
>>
>> Launcher added as well. I think I did it right? Please carefully 
>> check that I didn't miss any steps
>> or make any errors though.
>>
>>>> Onemore think I noted After I click the send - yo are allowing the 
>>>> app to open multiple simple
>>>> editor windows in time. This may be dangerous. - maybe some check 
>>>> "underlying file have changed,
>>>> reload?)
>>>> (but yah, advaced tool do the same )I'm for -
>>>>  - have it as modal dialogue in itw-settings
>>>>  - have it on frame in case of seaprate application (see suggestion 
>>>> in previous email)
>>>>  -  otherwse do as you wish ::)
>>>
>>> Hmm. I don't think I like the idea of making it modal in this 
>>> situation (just seems like it is not
>>> necessary, and unnecessary modality is really irritating to deal 
>>> with as a user IMO), but "file
>>> has changed, reload/ignore/quit" does sound good. I will look into 
>>> that next. Besides, even if we
>>> make it modal, that doesn't actually protect us against concurrent 
>>> modification very much. Perhaps
>>> itweb-settings control panel can be made to handle the PolicyEditor 
>>> in the same way that
>>> PolicyEditor handles CustomPolicyViewer - if there is no instance, 
>>> create one. If there is an
>>> instance, focus it rather than creating another.
>>
>> I chose this approach after all. The PolicyPanel will bring the 
>> existing PolicyEditor to front,
>> rather than making a new one. I think it works quite well.
>>
>> I wanted to implement a "underlying file has changed, 
>> reload/ignore/quit" type of check, but it
>
> What? everything you need is to check timestamp(faster) or 
> md5sum(better) before accessing the file...

Ah, so it would let you know if the file changed when you go to save 
again, basically. I was thinking of having it produce this dialog even 
when just "idling". I guess doing it only at file access time should be 
good enough. I'll do that as a separate changeset later on.

>
>> seems like this was only made into a standard library feature in Java 
>> 7, and I didn't see anything
>> in our own utilities to help with it. I'd rather avoid adding a 
>> worker thread to poll the file or
>> something ugly like that as well, but I suppose it would be doable. 
>> Apache Commons VFS can provide
>> this, but I don't know if we want to pull in that dependency just for 
>> this.
>>
>>>
>>> - "parsing"/"deserializing" is not the most robust. I haven't tested 
>>> it with random garbage
>>> malformed text or with comments in the middle of a block, etc. These 
>>> hardening improvements will
>>> come a bit later on, right now I want to nail down the 
>>> functionality/visuals. I may end up
>>> refactoring the models even further, so hardening it now might be a 
>>> waste of effort
>
> Please heavilly unittest it together with known to fail cases....
> (may come later! :) especially if more work is planned

More work is probably planned, yes, so heavier testing will also come later.

>>
>> Comments mostly work, but if you have a line like this:
>>
>> /* comment */ permission somepermission ... ;
>>
>> then the whole thing is considered a comment. I think this is okay 
>> for a small, "lightweight" editor
>> like this, but maybe eventually it will gain a proper parser (rather 
>> than the simple line-by-line
>> "parsing" it does now) to be able to handle this. Double-slash type 
>> comments are handled a bit more
>> nicely. Similarly, it doesn't properly handle multiple permissions on 
>> one line:
>>
>> permission permissionA ...; permission permissionB ...;
>>
>> and for now this will just have to be "good enough" I think. Anyway, 
>> this would only happen if
>> someone edited their policy file by hand and did this.
>>
>>>
>>> I'm planning to add plenty of unit testing to the models introduced 
>>> with this patch, but I'm out
>>> of time today and am taking a short day tomorrow, so I'm not sure if 
>>> I'll get those done before
>>> the end of the week, and I wanted to get this out for at least 
>>> visuals feedback first.
>>>
>>> Thanks,
>>>
>>
>> - More unit tests were added, for the new models
>> - Fixed some bugs with file loading and UI updating. Still, sometimes 
>> the list view looks strange
>> after a load, as if the topmost list element is mostly above the list 
>> and can't be scrolled into
>> view, and I'm not sure what to do about that. Adding a new codebase 
>> forces the list to update and it
>> fixes itself at least. The topmost element is always "All Applets" 
>> anyway so that's not too bad.
>> - FileUtils.getFileLock() usage was also added where the policy file 
>> is loaded and saved.
>> - Launching policyeditor from the command line with exactly one arg 
>> is interpreted as being meant as
>> a file path, since it's a little annoying to use the -file flag when 
>> this is also the only flag the
>> program even accepts other than -help. The -file flag remains in use 
>> simply because policytool has
>> it and I'd like to keep that option compatible.
>>
>> Still OK for head?
>
> As I told, I would like to see original pushed, andd this added on top 
> of it, however yes.
>
> Also the lunchers looks ok!
>
>  Thanks,
>
> J.

Great, going to push shortly then.

Thanks for review!

-- 
Andrew A