[rfc][icedtea-web] Move Translation Responsibility from JNLPRuntime to Translator

[Jie Kang]

----- Original Message -----
> On 09/11/2014 10:01 AM, Jie Kang wrote:
> >
> >
> > ----- Original Message -----
> >> On 09/10/2014 02:31 PM, Jie Kang wrote:
> >>>
> >>> Hello,
> >>>
> >>>
> >>> Good points. I have made Translator an enum with a single instance where
> >>> the public static String R still remains and added basic unit tests for
> >>> Translator.
> >>>
> >>>
> >>
> >> Overall I definitely like the idea behind this patch. Just a few minor
> >> implementation details I have some questions about.
> >>
> >>>       public static String R(String message) {
> >>> -        return JNLPRuntime.getMessage(message);
> >>> +        return getInstance().getMessage(message);
> >>>       }
> >>
> >> Can this be defined in terms of Translator.R(String, Object...) instead?
> >> Not a big deal but it seems natural to me.
> >
> > Hello,
> >
> >
> > Thanks for the review!
> >
> > I am not sure if I understood correctly but for this R(String message) do
> > you mean something like:
> >    return R(message, new Object[0])
> >
> > I prefer the other way since it's more efficient and makes more sense to me
> > hahah. Though I understand what you mean by being more natural. It's just
> > on closer inspection it seems really weird to make this call when you can
> > just use getInstance().getMessage(message);
> >
> > Is it okay if I keep it original?
> 
> Sure, this definitely is not a blocker on push. I think "efficiency"
> here is a pretty weak reason though ;) it's just one extra method call,
> not very heavy. The other benefit is that if down the line somebody
> wants to add some extra validation or something, for example, then it
> only needs to be added in one of the methods, rather than both overloads.
> 
> It's also a little bit confusing looking because since R(String,
> Object...) is varargs, it's actually valid to call it simply as, for
> example, R("TEST"). In this case, your varargs array just ends up being
> the empty array. So what you've done by defining both methods is
> essentially add a constraint for the varargs that it has to be of at
> least size 1, then branching into different calls to getMessage, one of
> which is actually defined in terms of the other in failure cases
> anyway... the call web is just a bit tangled is what I'm trying to say.
> But it's fine as-is regardless.

Hello,

Just wanted to say thanks for this explanation! I decided to go with what you suggested instead. Since the varargs case accepts 0 args, we can technically remove the function R(String message) and just have R(String message, Object ... params), no? Do you think it'd be good to remove R(String message) in this patch?


Regards,

> 
> >
> >>
> >>> +    public static void loadResourceBundle(ResourceBundle bundle) {
> >>> +        getInstance().resources = bundle;
> >>> +    }
> >>
> >> Why is this still static? I think this makes sense as a method belonging
> >> to the singleton instance, and I only see one call site (TranslatorTest)
> >> that would need to be refactored. I would also rather see a
> >> (package-private?) setter method rather than direct field assignment
> >> here, but that's not a huge deal.
> >
> > I chose to make it static since I wanted the class to be accessed in a
> > consistent manner. getInstance() was private and R() was public static so
> > it made sense to me to make loadResourceBundle() also static in order to
> > preserve the privacy of getInstance().
> >
> 
> Well, getInstance() may have been private, but INSTANCE is not anyway ;)
> 
> > Also I preferred keeping it public in order to possibly allow for
> > itweb-settings to have an option to set the preferred locale or something
> > like that (in the future). Now that I think about it, this isn't
> > necessary.
> >
> >
> >>
> >>> +        } catch (Exception ex) {
> >>> +            throw new IllegalStateException("Missing resource bundle in
> >>> netx.jar:net/sourceforge/jnlp/resource/Messages.properties");
> >>> +        }
> >>
> >> Is this path correct even for non-default (English) locales? Eg for
> >> Czech shouldn't it be attempting (and failing) to load
> >> net/sourceforge/jnlp/resource/Messages_cs.properties? I'm not sure how
> >> easy it is to get the name of the ResourceBundle that the runtime would
> >> attempt to load however, it may not be worth the effort - this is a
> >> pretty close approximation anyway even for other locales I suppose.
> >
> > In terms of resource bundle failing to load, it attempts to load whatever
> > the default locale is and failing that, attempts to load the base copy,
> > only if the base does not exist will it fail and throw the exception.
> > Getting the default locale is pretty easy so I've changed the message to
> > be more appropriate.
> >
> > How does it look?
> 
> Ah okay, this makes perfect sense. I do like the new message better.
> 
> >
> >
> > Thanks,
> >
> >>
> >> Thanks,
> >> --
> >> Andrew Azores
> >>
> >
> 
> +1 okay to push from me.
> 
> Thanks,
> --
> Andrew Azores
> 

-- 

Jie Kang