Request for review on German localizations for icedtea-web

Fri Mar 22 08:00:23 PDT 2013

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