[rfc][icedtea-web] PolicyEditor

[Jiri Vanek]

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

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