Request for review on German localizations for icedtea-web

[Jiri Vanek]

You have reviewd old version of patch. Most of your comment are invalid for version 2:((
see:
Subject: Re: Request for review on German localizations for icedtea-web
Date: Fri, 01 Mar 2013 11:59:22 +0100
From: Jiri Vanek <jvanek at redhat.com>
To: Jacob Wisor <gitne at excite.co.jp>
CC: distro-pkg-dev at openjdk.java.net

And especially attachement must be fixedWinPAthValidator_2.diff


Sorry for doubled work ;((
J.

On 03/22/2013 04:00 PM, Adam Domurad wrote:
> On 02/22/2013 06:47 AM, Jiri Vanek wrote:
>> On 02/22/2013 03:04 AM, Jacob Wisor wrote:
>>> Hello there!
>>>
>>>>> Jacob, Alexander - in your emails you have come to conclusion, that there
>>>>> are errors - both lexical
>>>>> and semantic in original English properties.  If you will have spare time,
>>>>> do you think - as
>>>>> separate patch, you can fix them?
>>>>>
>>>> *** OK, I can try to have look, but generally sometimes the source is quite
>>>> confusing and the translator can only guess, what is the correct meaning.
>>>> Rather the developers themselves should see our comments about unclear
>>>> parts and rephrase them so that the meaning is clear...
>>> I can post some proposals for the messages in question. It should not be really that much, but a few of them are utterly confusing indeed.
>>>    
>>> Btw, as I already wrote to Alexandr, I was able to compile at least NetX on Windows today, so I can see the German and Polish translations in the context of the running application. This helps a lot.
>>> Having done that, I have also found a minor bug (I did not check whether it has been already reported) in regards to one message on Windows. I have already commented on it. It is the message that refers to absolute paths that must begin with a /. Though absolute paths on Windows do not begin with a / (it is a forbidden character anyway), icedtea-web assumes the (semi-) correct default value. The default value is an absolute Windows path beginning with a drive letter or device. Icedtea-Web still complains about it that it does not begin with a / and prints an error message about the value. It should be easy to fix.
>>>
>>> Anyway, I will post an updated localization patch ASAP.
>>>
>>> Regards,
>>> Jacob
>>>
>> Wou!
>> So there will be also Polish trnaslation?
>> /me amazed :)
>>
>> And I'm happy netx is working on windows !o)
>>
>> I'm attaching patch to window paths. If you will be able to test, it would be nice (As there is no
>> win machine around :( ) I will send [rfc] for it as separate email later.
>>
>> Looking forward for transaltions!
>>
>>
>>
>> J.
> 
> Note i have not tested this on Windows, just doing a review of the code.
> 
>> diff -r ad2e15f15a75
>> netx/net/sourceforge/jnlp/config/BasicValueValidators.java
>> --- a/netx/net/sourceforge/jnlp/config/BasicValueValidators.java Thu
>> Feb 21 15:26:38 2013 -0500
>> +++ b/netx/net/sourceforge/jnlp/config/BasicValueValidators.java Fri
>> Feb 22 11:44:32 2013 +0100
>> @@ -82,12 +82,35 @@
>> }
>> };
>>
>> +
>> + //decomposed for testing
>> + public static boolean canBeWindows(String s) {
>> + //is just win bullet proof enough? SOme winfows mark themselves as
>> win32 or similar
>> + //but I can imagine linux distribution with some Name containing win:(
>> + //so I guess windows should be good enough as I have not seen
>> not-windows idf since XP
> 
> s/SOme/some/, s/winfows/Windows/, s/Name/name/
> idf -> ???
> 
> Anyway, I haven't see any reference that os.name can return 'win32', do
> you have one ?
> [nit] I'd be in favour of a shorter note here, it's a bit of a ramble :-)
> 
>> + return s.toLowerCase().contains("windows");
>> + }
>> +
>> + /**
>> + * guess the OS of user, if legal, or windows
>> + * @return
>> + */
>> + public static boolean canOsBeWindows() {
>> + return canBeWindows(System.getProperty("os.name"));
> 
>  From what I read on the internet,
> System.getProperty("os.name").startsWith("Windows") is the accepted way
> to do this. (I don't think we need to worry about this being inadequate
> until we have proof it doesn't handle some version of windows).
> [nit] I'd just call it isWindows() or isOsWindows() or similar.
> 
>> + }
>> +
>> /**
>> * Checks if a value is a valid file path (not a valid file!). The actual
>> * file may or may not exist
>> */
>> - private static class FilePathValidator implements ValueValidator {
>> + //package private foor testing purposes
> 
> s/foor/for/
> 
>> + static class FilePathValidator implements ValueValidator {
>>
>> + //not static, we want test!
>> + public boolean isWindows() {
>> + return canOsBeWindows();
>> + }
> 
> [nit] s/test/to test/
> [nit] This can be protected, and called 'handleWindowsPaths', which
> makes more sense as a non-static
> 
>> +
>> @Override
>> public void validate(Object value) throws IllegalArgumentException {
>> if (value == null) {
>> @@ -101,8 +124,16 @@
>> }
>>
>> String possibleFile = (String) possibleValue;
>> - if (!(possibleFile.startsWith("/"))) {
>> - throw new IllegalArgumentException();
>> +
>> + if (isWindows()) {
>> + //still maybe to strict, but should be ok for now
> 
> s/to/too/
> 
>> + if (!possibleFile.matches("([a-zA-Z]:)?(\\\\[a-zA-Z0-9_.-]+)+\\\\?")) {
> 
> This doesn't seem to consider / to be a valid character, unless I'm
> wrong. I rather be more lenient, and just check for the drive letter
> with :/ or :\ and then anything after it.
>> + throw new IllegalArgumentException();
>> + }
>> + } else {
>> + if (!(possibleFile.startsWith("/"))) {
>> + throw new IllegalArgumentException();
>> + }
>> }
>>
>> }
>> diff -r ad2e15f15a75
>> netx/net/sourceforge/jnlp/resources/Messages.properties
>> --- a/netx/net/sourceforge/jnlp/resources/Messages.properties Thu Feb
>> 21 15:26:38 2013 -0500
>> +++ b/netx/net/sourceforge/jnlp/resources/Messages.properties Fri Feb
>> 22 11:44:32 2013 +0100
>> @@ -283,7 +283,7 @@
>> # Value Validator messages. Messages should follow "Possible values ..."
>> VVPossibleValues=Possible values {0}
>> VVPossibleBooleanValues=are {0} or {1}
>> -VVPossibleFileValues=include the absolute location of a file - it
>> must begin with a /
>> +VVPossibleFileValues=include the absolute location of a file - it
>> must begin with a / on linux, or with DRIVE_LETTER:\ on windows
>> VVPossibleRangedIntegerValues=are in range {0} to {1} (inclusive)
>> VVPossibleUrlValues=include any valid url (eg
>> http://icedtea.classpath.org/hg/)
>>
>> diff -r ad2e15f15a75
>> tests/netx/unit/net/sourceforge/jnlp/config/BasicValueValidatorsTests.java
>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>> +++
>> b/tests/netx/unit/net/sourceforge/jnlp/config/BasicValueValidatorsTests.java
>> Fri Feb 22 11:44:32 2013 +0100
>> @@ -0,0 +1,133 @@
>> +/* DeploymentConfigurationTest.java
>> + Copyright (C) 2012 Red Hat, Inc.
>> +
>> + This file is part of IcedTea.
>> +
>> + IcedTea is free software; you can redistribute it and/or
>> + modify it under the terms of the GNU General Public License as
>> published by
>> + the Free Software Foundation, version 2.
>> +
>> + IcedTea is distributed in the hope that it will be useful,
>> + but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + General Public License for more details.
>> +
>> + You should have received a copy of the GNU General Public License
>> + along with IcedTea; see the file COPYING. If not, write to
>> + the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
>> Boston, MA
>> + 02110-1301 USA.
>> +
>> + Linking this library statically or dynamically with other modules is
>> + making a combined work based on this library. Thus, the terms and
>> + conditions of the GNU General Public License cover the whole
>> + combination.
>> +
>> + As a special exception, the copyright holders of this library give you
>> + permission to link this library with independent modules to produce an
>> + executable, regardless of the license terms of these independent
>> + modules, and to copy and distribute the resulting executable under
>> + terms of your choice, provided that you also meet, for each linked
>> + independent module, the terms and conditions of the license of that
>> + module. An independent module is a module which is not derived from
>> + or based on this library. If you modify this library, you may extend
>> + this exception to your version of the library, but you are not
>> + obligated to do so. If you do not wish to do so, delete this
>> + exception statement from your version.
>> + */
>> +package net.sourceforge.jnlp.config;
>> +
>> +import org.junit.Assert;
>> +import org.junit.Test;
>> +
>> +public class BasicValueValidatorsTests {
>> +
>> + private static class WinFilePathValidator extends
>> BasicValueValidators.FilePathValidator {
>> +
>> + @Override
>> + public boolean isWindows() {
>> + return true;
>> + }
>> + }
>> +
>> + private static class LinuxFilePathValidator extends
>> BasicValueValidators.FilePathValidator {
>> +
>> + @Override
>> + public boolean isWindows() {
>> + return false;
>> + }
>> + }
>> + private static final BasicValueValidators.FilePathValidator lv = new
>> LinuxFilePathValidator();
>> + private static final BasicValueValidators.FilePathValidator wv = new
>> WinFilePathValidator();
>> + private final String neverLegal = "aaa/bb/cc";
>> + private final String winLegal = "C:\\aaa\\bb\\cc";
> 
> I'd test window paths with / in them, as well.
> 
>> + private final String linuxLegal = "/aaa/bb/cc";
>> +
>> + @Test
>> + public void testWindowsDetction() {
>> + Assert.assertTrue(BasicValueValidators.canBeWindows("blah windows
>> blah"));
>> + Assert.assertTrue(BasicValueValidators.canBeWindows("blah Windows
>> blah"));
>> + Assert.assertTrue(BasicValueValidators.canBeWindows(" WINDOWS 7"));
>> + Assert.assertFalse(BasicValueValidators.canBeWindows("blah windy
>> miracle blah"));
>> + Assert.assertFalse(BasicValueValidators.canBeWindows("blah wind blah"));
>> + Assert.assertTrue(BasicValueValidators.canBeWindows("windows"));
>> + Assert.assertFalse(BasicValueValidators.canBeWindows("linux"));
>> + Assert.assertFalse(BasicValueValidators.canBeWindows("blah mac blah"));
>> + Assert.assertFalse(BasicValueValidators.canBeWindows("blah solaris
>> blah"));
>> + }
>> +
>> + @Test
>> + public void testLinuxAbsoluteFilePathValidator() {
>> + Exception ex = null;
>> + try {
>> + lv.validate(linuxLegal);
>> + } catch (Exception eex) {
>> + ex = eex;
>> + }
>> + Assert.assertTrue(ex == null);
>> +
>> + ex = null;
>> + try {
>> + lv.validate(neverLegal);
>> + } catch (Exception eex) {
>> + ex = eex;
>> + }
>> + Assert.assertTrue(ex instanceof IllegalArgumentException);
>> +
>> +
>> + ex = null;
>> + try {
>> + lv.validate(winLegal);
>> + } catch (Exception eex) {
>> + ex = eex;
>> + }
>> + Assert.assertTrue(ex instanceof IllegalArgumentException);
>> + }
>> +
>> + @Test
>> + public void testWindowsAbsoluteFilePathValidator() {
>> + Exception ex = null;
>> + try {
>> + wv.validate(winLegal);
>> + } catch (Exception eex) {
>> + ex = eex;
>> + }
>> + Assert.assertTrue(ex == null);
>> +
>> + ex = null;
>> + try {
>> + wv.validate(neverLegal);
>> + } catch (Exception eex) {
>> + ex = eex;
>> + }
>> + Assert.assertTrue(ex instanceof IllegalArgumentException);
>> +
>> +
>> + ex = null;
>> + try {
>> + wv.validate(linuxLegal);
>> + } catch (Exception eex) {
>> + ex = eex;
>> + }
>> + Assert.assertTrue(ex instanceof IllegalArgumentException);
>> + }
>> +}
> 
> Happy hacking,
> -Adam
> 
>