/hg/icedtea-web: IcedTea-Web Splashscreen allows zero or one hyp...

[Lukasz Dracz]

Hello,

> On 10/06/2014 05:10 PM, Lukasz Dracz wrote:
> > Hello,
> >
> > Sorry, I thought at the time the change/difference was small enough but I
> > agree even the smallest changes should get reviewed. The reasoning behind
> > the change is that I thought -*headless  was not a correct change, since
> > really only headless and -headless are accepted as options, anything with
> > more dashes is not accepted such as --headless, ---headless, -----headless
> > etc. It did not make sense to have a user put ---headless, then have the
> > icedtea-web splash screen not appear only to get a message back that
> > ---headless is not an accepted option. That is why I changed what was
> > agreed upon to what is actually supported, which is only headless or
> > -headless.
> > Once again I am sorry, I did not mean to violate the review process. Do you
> > want to support -*headless ? So anything from headless to --*--headless I
> > can make the required changes if that is what is wanted in option parser.
> > Should the change be reverted ?
> >
> 
> In case that your parser supports only -headless or headles, tehn your chnage
> will be ok. I also
> like your ^ and [] enhancement. For that I will do nothing more then ping you
> adn verify stuff.
> 
> But I would swear, that we agreed  on -* support.

Ah yes because I thought it did actually work like that with headless or -----headless being the same at the time, then when I looked into it more, I was thinking just adapting the splash screen to how it was implemented with just headless or -headless. My question is do you want it to support --*--headless ? I'm fine with that as it does add leniency and flexibility to how our users input options but on the other hand I don't expect any one to really to put ----headless for ex. The idea behind allowing -headless or headless was that it wouldn't matter if the user remembers if it had a dash or not. 

> 
> However in code I now can see the
> 
>   private boolean stringEqualsOption(String input, OptionsDefinitions.OPTIONS
>   opt) {
>          String option = opt.option.replaceAll("^-","").split("=")[0];
>          input = input.replaceAll("^-","").split("=")[0];
>          if (input.equals(option)) {
>              return true;
>          }
>          return false;
>      }
> 
> 
> Which really accept only -{0,1}
> 
> And the method is not tested;)
> 
> 
> So with that, you are cleared for this well intentioned  change, but please,
> try to avoid it.
> especially when original review is gone;)

Alright, I will double check with the list next time :)
 
> Also please [rfc] attached patch, which is fixing both those issues.

Do you want me to review it ? or to post it on the list as a separate thread ?

I'll review it right now, It looks good :) 
The only thing I'm wondering is maybe instead of sanitize call the method removeAllDashes / removeDashes / sanitizeDashes to be more descriptive of what it does.

Other than that it looks good to push if you want to go into the direction of supporting -*-headless :) +1

> Sorry for the "attack", but at least it clarified the  -/--/---// hyphens
> issue

Yes its good that the hyphens issue is being clarified ! I don't mind "attacks" as they help me learn from my mistakes and makes sure I do my best ;)
 
> J.
> 
> ps: * but please, really try to avoid such last-moment changes *

okay :)

Thank you,
Lukasz Dracz

> >
> > ----- Original Message -----
> >> From: "Jiri Vanek" <jvanek at redhat.com>
> >> To: ldracz at icedtea.classpath.org, distro-pkg-dev at openjdk.java.net
> >> Sent: Monday, October 6, 2014 4:30:33 AM
> >> Subject: Re: /hg/icedtea-web: IcedTea-Web Splashscreen allows zero or one
> >> 	hyp...
> >>
> >> Shouldnt it be zero to infinity hyphens?
> >>> +++ b/launcher/launchers.in	Fri Oct 03 14:20:40 2014 -0400
> >>> @@ -53,7 +53,7 @@
> >>>        *)
> >>>          ARGS[$j]="$1"
> >>>          j=$((j+1))
> >>> -      if [ "$1" = "-headless" ] ; then
> >>> +      if [[ "$1" =~ ^[-]{0,1}headless ]] ; then
> >>
> >>    =>  +      if [[ "$1" =~ ^[-]*headless ]] ; then
> >>
> >> I rember it like this....
> >>
> >> ?
> >>>            SPLASH="false"
> >>>          fi
> >>>          ;;
> >>>
> >>
> >>
> >> diff -r 6d62f68fb037 launcher/launchers.in
> >> --- a/launcher/launchers.in	Mon Sep 22 17:10:07 2014 +0200
> >> +++ b/launcher/launchers.in	Thu Sep 25 17:26:49 2014 -0400
> >> @@ -53,7 +53,7 @@
> >>        *)
> >>          ARGS[$j]="$1"
> >>          j=$((j+1))
> >> -      if [ "$1" = "-headless" ] ; then
> >> +      if [[ "$1" =~ -*headless ]] ; then
> >>            SPLASH="false"
> >>          fi
> >>          ;;
> >>
> >>
> >> is your original chnageset. I have not found discussion about this chnage,
> >> and unless I overlooked it, it is violating the review process.
> >>
> >>
> >> ????
> >> J.
> >>
> 
>