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

[Jiri Vanek]

On 09/11/2014 04:01 PM, 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?
>
>>
>>> +    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().
>
> 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?
>
>
I'm happy with that. If Andrew have some objections, Follow his advises.

Green from me :)

J.