[rfc][icedtea-web] Option Parser
Jiri Vanek
jvanek at redhat.com
Wed Sep 10 16:08:28 UTC 2014
On 09/09/2014 10:17 PM, Lukasz Dracz wrote:
> Hello,
>
> I have added support for -jnlp and -basedir. In OptionsDefinitions I added JNLP to runtime options jawaws list and BASEDIR directly to the javaws list.
>
> I also got rid of the boolean in the constructor and the call to findMainArg from the constructor. I made findMainArg public since I found that really only want this called if -jnlp or -basedir is not found when parsed for. Added a getMainArgs() to return multiple arguments it finds as Main Args.
>
> The not valid args are dealt with in Boot's getJNLPFile. It determines whether it has more than 1 main arg or if it has additional main arg along with a JNLP and BASEDIR value, and then outputs that these main args are not valid arguments. Then it checks for JNLP, BASEDIR and a single main arg and returns a value based on what it finds.
>
> Thank you,
> Lukasz Dracz
As we agreed, drop basedri, and update NEWS
Something like "dropped support for long unmaintained -basedir argument"
And also "maybe returned support for -jnlp argument " :)
Much more nits later
>
>
> optionParser-13.patch
>
>
> diff -r b0690979967f netx/net/sourceforge/jnlp/OptionsDefinitions.java
> --- a/netx/net/sourceforge/jnlp/OptionsDefinitions.java Tue Sep 09 14:51:16 2014 +0200
> +++ b/netx/net/sourceforge/jnlp/OptionsDefinitions.java Tue Sep 09 15:52:35 2014 -0400
> @@ -71,6 +71,8 @@
> NOHEADERS("-Xignoreheaders", "BXignoreheaders"),
> OFFLINE("-Xoffline", "BXoffline"),
> TRUSTNONE("-Xtrustnone","BOTrustnone"),
> + JNLP("-jnlp","BOJnlp", NumberOfArguments.ONE),
> + BASEDIR("-basedir", "BOBasedir",NumberOfArguments.ONE),
> //itweb settings
> NODASHHELP("help", "IBOHelp"),
> LIST("list", "IBOList"),
> @@ -200,7 +202,8 @@
> OPTIONS.NOFORK,
> OPTIONS.NOHEADERS,
> OPTIONS.OFFLINE,
> - OPTIONS.TRUSTNONE});
> + OPTIONS.TRUSTNONE,
> + OPTIONS.JNLP});
> }
>
> public static List<OPTIONS> getJavaWsOptions() {
> @@ -210,6 +213,7 @@
> //trustall is not returned by getJavaWsRuntimeOptions
> //or getJavaWsControlOptions, as it is not desitred in documentation
> l.add(OPTIONS.TRUSTALL);
> + l.add(OPTIONS.BASEDIR);
> return l;
> }
>
> diff -r b0690979967f netx/net/sourceforge/jnlp/ParserSettings.java
> --- a/netx/net/sourceforge/jnlp/ParserSettings.java Tue Sep 09 14:51:16 2014 +0200
> +++ b/netx/net/sourceforge/jnlp/ParserSettings.java Tue Sep 09 15:52:35 2014 -0400
> @@ -37,9 +37,6 @@
>
> package net.sourceforge.jnlp;
>
> -import java.util.Arrays;
> -import java.util.List;
> -
> /**
> * Contains settings to be used by the Parser while parsing JNLP files.
> *
> @@ -99,12 +96,8 @@
> * at boot on the command line. These settings are also stored so they
> * can be retrieved at a later time.
> */
> - 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;
> }
Looking to the code, I really do not see an reason for this to do exists anymore (even withyour
explanation).
This is just creating new object. You can create it and set is reguraly
If you decide to have helepr method here, then the bestargument for it is optionParser, and this
helping method will read it on its own.
eeing this:
ParserSettings defaultSettings, noArgs;
defaultSettings = new ParserSettings();
- noArgs = ParserSettings.setGlobalParserSettingsFromArgs(new String[0]);
+ noArgs = ParserSettings.setGlobalParserSettingsFromArgs(false, true);
later, makes me think that combination of
setGlobalParserSettingsFromArgs(boolean strict, boolean xml) {
and
setGlobalParserSettingsFromArgs(OptionParser)) {
setGlobalParserSettingsFromArgs(getParsed1, getPArsed2...) {
}
Is the pointer...
>
> diff -r b0690979967f netx/net/sourceforge/jnlp/resources/Messages.properties
> --- a/netx/net/sourceforge/jnlp/resources/Messages.properties Tue Sep 09 14:51:16 2014 +0200
> +++ b/netx/net/sourceforge/jnlp/resources/Messages.properties Tue Sep 09 15:52:35 2014 -0400
> @@ -238,6 +238,7 @@
> BOUsage=[-run-options] jnlp file
> BOUsage2=[-control-options]
> BOJnlp = Location of JNLP file to launch (url or file).
> +BOBasedir = Location of JNLP file to launch (url or file).
> BOArg = Adds an application argument before launching.
> BOParam = Adds an applet parameter before launching.
> BOProperty = Sets a system property before launching.
> diff -r b0690979967f netx/net/sourceforge/jnlp/resources/Messages_cs.properties
> --- a/netx/net/sourceforge/jnlp/resources/Messages_cs.properties Tue Sep 09 14:51:16 2014 +0200
> +++ b/netx/net/sourceforge/jnlp/resources/Messages_cs.properties Tue Sep 09 15:52:35 2014 -0400
> @@ -234,6 +234,7 @@
> BOUsage=javaws [-volby-spu\u0161t\u011bn\u00ed] <soubor jnlp>
> BOUsage2=javaws [-volby-ovl\u00e1d\u00e1n\u00ed]
> BOJnlp= Um\u00edst\u011bn\u00ed souboru JNLP ke spu\u0161t\u011bn\u00ed (URL nebo soubor)
> +BOBasedir= Um\u00edst\u011bn\u00ed souboru JNLP ke spu\u0161t\u011bn\u00ed (URL nebo soubor)
> BOArg= P\u0159id\u00e1 p\u0159ed spu\u0161t\u011bn\u00edm parametr aplikace.
Dont forget to drop also those lines and thirs mutations
...
> */
> private static String getJNLPFile() {
> + if (optionParser.getMainArgs().length > 1 || optionParser.mainArgExists()
> + && (optionParser.hasOption(OptionsDefinitions.OPTIONS.JNLP)
> + || optionParser.hasOption(OptionsDefinitions.OPTIONS.BASEDIR))) {
> + for (String s : optionParser.getMainArgs()) {
> + OutputController.getLogger().printOutLn("The argument '"+ s +"' is not a valid expected argument");
use getLogger.log() for all outputs. There is actually no exception...
> + }
> + } else if (optionParser.hasOption(OptionsDefinitions.OPTIONS.JNLP)) {
> + return optionParser.getValue(OptionsDefinitions.OPTIONS.JNLP);
I would encapsulate this in OptionParser
something like optionParser.validateMainArg() or similar.
There is still question how to handle them.
But right now, the best way is to die once some unrecognized arg is found. Its not celar for me
where this happens in code. AFAIK it does not.
So validateMainArg should throw lunchError with invalid options found.
> + } else if (optionParser.hasOption(OptionsDefinitions.OPTIONS.BASEDIR)) {
> + return optionParser.getValue(OptionsDefinitions.OPTIONS.BASEDIR);
> + } else if (optionParser.mainArgExists()) {
> + return optionParser.getMainArg();
> + }
>
> - if (args.length == 0) {
> - handleMessage();
> - JNLPRuntime.exit(0);
> - } else if (args.length == 1) {
> - return args[args.length - 1];
> - } else {
> - String lastArg = args[args.length - 1];
> - String secondLastArg = args[args.length - 2];
> -
> - if (doubleArgs.indexOf(secondLastArg) == -1) {
> - return lastArg;
> - } else {
> - handleMessage();
> - JNLPRuntime.exit(0);
> - }
> - }
> + handleMessage();
> + JNLPRuntime.exit(0);
> return null;
> }
...
> +
> + private final String[] args;
> + private final Map<OptionsDefinitions.OPTIONS, List<String>> parsedOptions;
You have list here, and it is correct. Do not vaste time converting it to array. Rather fix the
users of those methods.
> + private final List<OptionsDefinitions.OPTIONS> possibleOptions;
> + private final String MAINARG = "mainArg";
> + //null represents all values that are parsed that don't have a corresponding option which are potential main args
> + private final OptionsDefinitions.OPTIONS mainArg = null;
> + private List<String> result;
> +
> +
> + public OptionParser(String[] args, List<OptionsDefinitions.OPTIONS> options) {
> + this.args = Arrays.copyOf(args, args.length);
> + this.possibleOptions = options;
> +
> + result = new ArrayList<String>();
> + parsedOptions = new HashMap<>();
> + result = parseContents(args, result);
> +
> + }
> +
> + private List<String> parseContents(final String[] args, List<String> result) {
> + String lastOption = "";
> + int i = 0;
> + while (i < args.length) {
> + if (isOption(args[i])) {
> + result.clear();
> + if (args[i].contains("=")) {
> + result.add(args[i].split("=")[1]);
> + lastOption = args[i].split("=")[0];
> + parsedOptions.put(getOption(lastOption), new ArrayList<String>(result));
> + } else {
> + lastOption = args[i];
> + if (parsedOptions.keySet().contains(getOption(lastOption))) {
> + if (getOption(lastOption).hasOneOrMoreArguments()) {
> + result = new ArrayList<>(parsedOptions.get(getOption(lastOption)));
> + }
> + }
> + if (getOption(lastOption).hasNoArguments()) {
> + parsedOptions.put(getOption(lastOption), null);
> + lastOption = MAINARG;
> + }
> + }
> + } else {
> + result.add(args[i]);
> + parsedOptions.put(getOption(lastOption), new ArrayList<String>(result));
> + if (getOption(lastOption) != null) {
> + if (getOption(lastOption).hasOneArgument()) {
> + lastOption = MAINARG;
> + result.clear();
> + }
> + }
> + }
> + i++;
> + }
> + return result;
> + }
> +
> + public void findMainArg() {
> + int i = args.length - 1;
> + if (!(parsedOptions.keySet().contains(mainArg))) {
> + while (i >= 0) {
> + if (!args[i].startsWith("-")) {
> + for (OptionsDefinitions.OPTIONS op : parsedOptions.keySet()) {
> + if (!(parsedOptions.get(op) == null)) {
> + if (parsedOptions.get(op).contains(args[i])) {
> + result.clear();
> + result.add(args[i]);
> + parsedOptions.get(op).remove(parsedOptions.get(op).indexOf(args[i]));
> + break;
> + }
> + }
> + }
> + parsedOptions.put(mainArg, result);
> + break;
> + }
> + i--;
> + }
> + }
> + }
> +
> + private boolean isOption(String s) {
> + for(OptionsDefinitions.OPTIONS opt : possibleOptions){
> + if (stringEqualsOption(s, opt)) {
> + return true;
> + }
> + }
> + return false;
> + }
> +
> + private OptionsDefinitions.OPTIONS getOption(String s) {
> + for(OptionsDefinitions.OPTIONS opt : possibleOptions){
> + if (stringEqualsOption(s, opt)) {
> + return opt;
> + }
> + }
> + return mainArg;
> + }
> +
> + private boolean stringEqualsOption(String s, OptionsDefinitions.OPTIONS opt) {
> + String option = opt.option.replace("-","").split("=")[0];
This is actually still wrong. You shoudl remove only leading dashes.
So you may change your algorithm to some while (s.starts) or use repalceAll with "^-*" regex
> + s = s.replace("-","").split("=")[0];
This is s surprising but not guaranted that if split argument mathces nothing then whole original is
returens. But I guess we can live with that.
Anyway this method needs test.
> + public boolean mainArgExists() {
> + if (parsedOptions.keySet().contains(mainArg)) {
> + return true;
> + }
> + return false;
> + }
> +
> + public String getMainArg() {
> + if (mainArgExists()) {
> + return parsedOptions.get(mainArg).toArray()[0].toString();
> + }
> + return "";
> + }
> +
> + public String[] getMainArgs() {
> + if (mainArgExists()) {
> + return parsedOptions.get(mainArg).toArray(new String[0]);
here and lower
> + }
> + return new String[] { };
> + }
> +
> + public String getValue(OptionsDefinitions.OPTIONS option) {
> + if (parsedOptions.get(option) != null) {
> + return parsedOptions.get(option).toString().substring(1, parsedOptions.get(option).toString().length() - 1);
> + }
> + return "";
> + }
> +
> + public String[] getValues(OptionsDefinitions.OPTIONS option) {
> + if (parsedOptions.get(option) != null) {
> + return parsedOptions.get(option).toArray(new String[0]);
ere is lower
and here - do not connvert to array. Wotking with list is much more comfortable. Keep returning
list, and fix the users of those calls rather.
> + }
> + return new String[]{};
> + }
More information about the distro-pkg-dev
mailing list