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

[Andrew Azores]

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.

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