[RFC][icedtea-web] Fix PR955: regression: SweetHome3D fails to run

Jiri Vanek jvanek at redhat.com
Wed Aug 8 05:11:46 PDT 2012


On 08/07/2012 08:20 PM, Danesh Dadachanji wrote:
>
>

I'm ok for head and in some time also for 1.3

> On 02/08/12 09:57 AM, Jiri Vanek wrote:
>> On 08/01/2012 05:13 PM, Danesh Dadachanji wrote:
>>> Hello,
>>>
>>> Here is a fix for PR955. [1]
>>>
>>> This was quite a tricky patch to figure out. Proprietary webstart handles locales..
>>> interestingly.. I've made ours use a fallback
>>> mechanism to find any information in the information tag. For each item it searches for (e.g. the
>>> title), it will try to find an
>>> information tag which has the closest possible locale that looks like your JVM's locale. Please
>>> see the unit tests for examples.
>>>
>>> I have changed a few other things too with how we setup locales, the variant was not working
>>> properly. It was assumed to be 2
>>> characters long which is definitely not the case. My locale is en_CA.utf8 <-- variant has 4 chars.
>>>
>>> My changes to the localeMatches method cover both the implementation I needed (being able to
>>> match multiple a locale by a given
>>> precision - the enum), as well as making it more easily understood. I hope the switch cases do
>>> this, it was a bit of a brain teaser
>>> figuring out those if statements and double negatives. :/
>>>
>>> All other changes are in the ChangeLog below:
>>> +2012-08-01 Danesh Dadachanji <ddadacha at redhat.com>
>>> +
>>> + Fix PR955: regression: SweetHome3D fails to run
>>> + * NEWS: Added entry for PR955
>>> + * netx/net/sourceforge/jnlp/JNLPFile.java: New enum Match that represents
>>> + the level of precision to use when matching locales.
>>> + (localMatches): Renamed to localeMatches, added matchLevel paramater
>>> + and updated conditionals to handle the level of precision specified by it.
>>> + (getVendor): New method that returns an information's vendor text.
>>> + (getInformation): Added override methods for getTitle and getVendor
>>> + that are used by the anonymous class to filter by locale. All three
>>> + methods now go through all levels of precision to search for the best
>>> + fitted locale.
>>> + (getResources), (getResourcesDescs): Updated to check if any level of
>>> + precision matches when searching for locales.
>>> + (parse): Added call to checkForTitleVendor.
>>> + * netx/net/sourceforge/jnlp/Parser.java
>>> + (checkForTitleVendor): New method to check for availability of localized
>>> + title and vendor from the information tags. Throws ParseException.
>>> + (getInfo): Replace loop with foreach loop.
>>> + (getInformationDesc): Remove check for present title and vendor.
>>> + (getLocale): Variant returned can now use everything after the eigth
>>> + element of the locale's string.
>>> + * netx/net/sourceforge/jnlp/resources/Messages.properties:
>>> + Update missing title and vendor messages to mention localization.
>>> + *
>>> tests/reproducers/simple/InformationTitleVendorParser/testcases/InformationTitleVendorParserTest.java:
>>>
>>> + Update output string as per new changes to Messages internationalizations.
>>> + * tests/netx/unit/net/sourceforge/jnlp/JNLPFileTest.java:
>>> + New unit test that checks the localesMatches method in JNLPFile.
>>> + * tests/netx/unit/net/sourceforge/jnlp/MockJNLPFile.java:
>>> + New class used to create a mock JNLPFile object.
>>> + * tests/netx/unit/net/sourceforge/jnlp/ParserTest.java:
>>> + New unit test that checks that the return of getTitle and getVendor
>>> + have localized information.
>>>
>>> I would like to push this to HEAD and backport it to 1.3. The regression isn't present in 1.2 and
>>> before. Any thoughts?
>>>
>>> Regards,
>>> Danesh
>>> [1] http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=955
>>
>> Hi! Nice work, I have just few really minor stuff for patch itself and several more for tests.
>>
>> in JNLPFile there are iner classes ( you have touched them) InformationDesc and ResourcesDesc
>> which are overriding methods but are
>> missing annotations. Can you please add those notations? ( I have counted 5 of them)
>
> Ah yes, good catch, done.
>
>> You are throwing ParseException  if venodr/title is not found, and later in tests you are testing
>> result against internationalize-able
>> text. What do you think about more modular  exception chain?
>> eg ParseException -> RequiredElementException --> (MissingTitleException + MissingVendorException)
>> and then in tests check just for
>> those exceptions without real text?
>
> Amazing idea, this also makes the unit tests cleaner! I added a third, MissingInformationException
> is needed as well.
>
>> In Message.properties you are writing
>> "The title/vendor section has not been defined for your locale in the JNLP file." which looks
>> wrong to me or I understand badly your
>> code. Your code will fallback to the not-localised  if no locale (ab_CD.efg -> ab_CD -> ab ->
>> defualt) or not?
>
> You are correct, I added that change before I fully understood the fallbacks, changing it now.
>
>> This should be a bit reflecting in error message. eg "The title/vendor section has not been
>> defined for your locale nor is existing its
>> default value in yourJNLP file."
>> You can also save duplicate entries in properties by something like :
>>
>> PVendor=vendor
>> PTitle=title
>> PNoVendorTitleElement=The {1} section has not been defined for your locale in the JNLP file.
>
> Neat, I did not know about this. I think it's {0} though for param1.

sorry, sure.
>
>>
>> and
>>
>> throw new SomeBetterThenParseException(R("PNoVendorTitleElement",R("PTitle"/"PVendor")));
>
> Done.
>
>>
>> but as you wish....
>>
>>
>>
>> Tests:
>> MockJNLPFile is in tests directory but is not a test. So test runner will fail on it. This is not
>> what is wanted. To prevent this yo
>> can make this class as inner or move to test-extensions. because this file is already shared I'm
>> for second approach. lets move it eg
>> to test-extensions/net.sourceforge.jnlp.mocks
>
> Great idea, done.
>
>> Unittests will probably also deserve tests with new Locale("X") and new Locale("x","y") (you are
>> now just testing new Locale("x","y","z");
>
> Okay, I duplicated all the tests to use Locale("EN") and to use Locale("EN", "CA"). The tests are
> the almost identical, I had to swap a few expected failures around. (e.g. Locale("EN", "CA") should
> fail if the info tag had "en_CA.utf8"). Apart from this, they're pretty much the same tests copy
> pasted. =)
>
>> Reproducer test - reproducer is missing:(( I tought that you will include two or three (both
>> failing and passing) with the Locales
>> hack. I have posted repreoducer (without hack!) some time ago, it is not in. Feel free to ruse.
>> But imho reproducer is still worthy.
>>
>>
>
> I thought the unit tests + the general info/title/vendor reproducer would be enough so I didn't try
> to write one earlier. I gave writing one a shot but did not get very far. I'd like to finish off the
> tests but I just don't have enough cycles left to finish them off. Sorry! Please feel free to take
> over the rest of the patch.
>
> I've attached the latest patch here, hopefully you or someone else can pick it up. Thanks very much
> for all the reviews!
>
> Regards,
> Danesh




More information about the distro-pkg-dev mailing list