[rfc][icedtea-web] Option Parser

[Lukasz Dracz]

Hello,
 
> I'm quite happy with the patch.
> 
> Still few nits (really nits) and two questions
> 
>   Q1 - your patch tolerates more then one unknown parameter. This should be
>   handled differrently.
>   Q2 - By accident I found
> BOJnlp      = Location of JNLP file to launch (url or file).
>    in message.properties
> 
> Now it is not supported.  is it worthy of support?
 
Yeah I can support it, its just specifying the main arg in this case with an option. Also there was support before for -basedir which did exactly what -jnlp did. Should we support -basedir too ? I thought it was simpler to just expect the user to input the File as a main arg, but I can add support for having a -jnlp or -basedir option before the File if you want.
 
> A2 - I dont know ;) I'm in temptation to return this param. If not, then it
> shold be removed from
> properties.
> 
> A1 depends on A2 :)
>   As it is now, if more then one main paramis found, then ITW should die. And
>   print out that last
> parameter is expected to be jnlp file, and all those unknown params were
> found. (in error_all new
> LunchException)
>   If we agree that Q2 answer is yes, then except above reaction there should
>   be second one.
>    If -jnlp is s found, then  no unknown param is tolerated, and you can
>    simply die. Anyway your
> parser ust live also with no -jnlp as is now.

Yeah I am going to work on detecting unknown params. 

> >
> >
> > diff -r b0690979967f netx/net/sourceforge/jnlp/ParserSettings.java
> > --- a/netx/net/sourceforge/jnlp/ParserSettings.java	Tue Sep 09 14:51:16
> > 2014 +0200
> 
> 
> > -    public static ParserSettings setGlobalParserSettingsFromArgs(String[]
> > cmdArgs) {
> > -        List<String> argList = Arrays.asList(cmdArgs);
> > -        boolean strict = argList.contains("-strict");
> > -        boolean malformedXmlAllowed = !argList.contains("-xml");
> > -
> > -        globalParserSettings = new ParserSettings(strict, true,
> > malformedXmlAllowed);
> > +    public static ParserSettings setGlobalParserSettingsFromArgs(boolean
> > strict, boolean xml) {
> > +        globalParserSettings = new ParserSettings(strict, true, !xml);
> >           return globalParserSettings;
> 
> Get rid of this factory method compeltely.
> 
> you can simply
> ParserSettings settings =  new
> ParserSettings(optionParser.hasOption(OptionsDefinitions.OPTIONS.STRICT),optionParser.hasOption(OptionsDefinitions.OPTIONS.XML));
> 
> in smilar way as you have it now, or not?

The problem with that is, there is a

private static ParserSettings globalParserSettings = new ParserSettings();

with a getGlobalParserSettings() and setGlobalParserSettings(), so I assumed these methods that reference the static ParserSettings might have been used elsewhere so I thought it was a good idea to set the globalParserSettings as it was done before. The getGlobalParserSettings is used in the PluginBridge and NetxPanel.

> 
> >       }
> >
> > diff -r b0690979967f netx/net/sourceforge/jnlp/runtime/Boot.java
> > --- a/netx/net/sourceforge/jnlp/runtime/Boot.java	Tue Sep 09 14:51:16 2014
> > +0200
> 
> ...SNIP...
> 
> > +    }
> > +
> > +    private boolean stringEqualsOption(String s,
> > OptionsDefinitions.OPTIONS opt) {
> 
> Ouch this can be done much nicer...
> Just strip all leading "-" and all behind "="we are done.
> then you have only one simple s.equals[IgnoreCase?]
> 
> > +        if (s.equals(opt.option) || s.equals("-".concat(opt.option)) ||
> > s.split("=")[0].equals(opt.option)
> > +                || s.split("=")[0].equals("-".concat(opt.option)) ||
> > "-".concat(s).equals(opt.option) ||
> > "-".concat(s).equals("-".concat(opt.option))
> > +                || "-".concat(s).split("=")[0].equals(opt.option) ||
> > "-".concat(s).split("=")[0].equals("-".concat(opt.option))) {
> > +            return true;
> > +        }

Yes, I will work on implementing this in a better way.

> Except above, its a msut that iweb-settings and policyeditor uses this option
> parser to.
> 
> You may do it as different changeset but you have to. Mybe evennow is goo
> dtime to check if your
> parser will suit.

Yeah I will begin to look and see how itweb-settings and policyeditor handle parsing and thinking about how OptionParser will be used by them.

Thank you,
Lukasz Dracz