Implement xml:lang support in ConnectionConfiguration and AbstractXMPPConnection #325

ge0rg · 2019-07-26T13:11:07Z

This patch makes it possible to change the stream-level language as part
of the connection configuration, to allow a properly implemented
entities to provide i18n'ed response messages.

This code does not cover XMPPBOSHConnection!

X-Signed-Off-By: Georg Lukas georg@op-co.de

GitCop · 2019-07-26T13:11:09Z

There were the following issues with your Pull Request

Commit: ffd3432
Your subject line is longer than 72 characters

Guidelines are available at https://github.com/igniterealtime/Smack/wiki/Guidelines-for-Smack-Developers-and-Contributors

This message was auto-generated by https://gitcop.com

Flowdalic · 2019-07-27T10:27:15Z

smack-core/src/main/java/org/jivesoftware/smack/ConnectionConfiguration.java

@@ -118,6 +118,8 @@
    private final String password;
    private final Resourcepart resource;

+    private final String language;


This should be of type java.util.Locale.

Isn't a locale more than just the language? Are we going to have issues of ambiguity here?

Isn't a locale more than just the language?

"A locale" yes, it describes how your date is formatted, your preferred input method and other things.

But think of java.util.Locale as a wrapper around BCP 47, which is what xml:lang also uses.

Are we going to have issues of ambiguity here?

I don't see any.

Flowdalic · 2019-07-27T10:28:54Z

smack-core/src/main/java/org/jivesoftware/smack/ConnectionConfiguration.java

+         * @param language the language to use.
+         * @return a reference to this builder.
+         * @see <a href="https://tools.ietf.org/html/rfc6120#section-4.7.4">RFC 6120 § 4.7.4</a>
+         */


I suggest to additionally add a @see to linking to "XML 1.0 § 2.12" https://www.w3.org/TR/xml/#sec-lang-tag

GitCop · 2019-07-29T10:31:07Z

There were the following issues with your Pull Request

Commit: ffd3432
Your subject line is longer than 72 characters

Guidelines are available at https://github.com/igniterealtime/Smack/wiki/Guidelines-for-Smack-Developers-and-Contributors

This message was auto-generated by https://gitcop.com

ge0rg · 2019-07-29T18:51:29Z

So I've updated the code, just to realize that Locale.toLanguageTag() didn't exist in Android prior to r21. No idea how it even compiled with targetSdkLevel 17. I suppose the least ugly workaround would be to use Locale.toString.replace("_", "-"). Other ideas?

Flowdalic · 2019-07-29T19:39:48Z

No idea how it even compiled with targetSdkLevel 17.

What did you compile against an Android API 17 runtime jar? Smack, besides smack-android and smack-android-extensions is compiled with the Java SE runtime jar set bootstrapClasspath.

But do not worry, Smack also uses AnimalSniffer to ensure Android compatibility, which got you covered here and is the reason travis fails for the PR in the current state.

I suppose the least ugly workaround would be to use Locale.toString.replace("_", "-"). Other ideas?

Works for me. We can always switch to a better approach if we come up with one. Please also add the following comment:

// TODO: Change to Locale.toLanguageTag() once Smack's minimum Android API level is 21 or higher

Thanks

GitCop · 2019-07-30T13:35:48Z

There were the following issues with your Pull Request

Commit: ffd3432
Your subject line is longer than 72 characters

Guidelines are available at https://github.com/igniterealtime/Smack/wiki/Guidelines-for-Smack-Developers-and-Contributors

This message was auto-generated by https://gitcop.com

Flowdalic · 2019-07-30T13:39:48Z

smack-core/src/main/java/org/jivesoftware/smack/ConnectionConfiguration.java

@@ -577,7 +578,7 @@ public boolean isEnabledSaslMechanism(String saslMechanism) {
        private CharSequence username;
        private String password;
        private Resourcepart resource;
-        private String language = "en";
+        private Locale language = Locale.ENGLISH;


Is there any reason to not use Locale.getDefault() to initialize the value?

I didn't dare to change the default, which was "en". If you feel that using the default locale is good, and it's not going to break anything, I can patch it in.

I'm pretty sure, that there may be some tests that might break on non-english systems.
There was one such case pretty recently, but fixing those tests in the first place is probably a good idea :D

What @vanitasvitae said :)

The "nice" thing about that is that now we are leaking the environment of the user into the test setup, and from my experience, it will only break on locales that are not used by the maintainers in the first place, like Turkish (capital "i"), or Arabic (number parsing / formatting will bite you!). Feel free to rework the test setup to iterate through all existing locales, but I'm keeping this status-quo as is in this PR.

Flowdalic · 2019-07-30T20:49:59Z

smack-core/src/main/java/org/jivesoftware/smack/AbstractXMPPConnection.java

@@ -2012,7 +2012,7 @@ protected void sendStreamOpen() throws NotConnectedException, InterruptedExcepti
            from = XmppStringUtils.completeJidFrom(localpart, to);
        }
        String id = getStreamId();
-        sendNonza(new StreamOpen(to, from, id));
+        sendNonza(new StreamOpen(to, from, id, config.getLanguage().toString().replace("_", "-"), StreamOpen.StreamContentNamespace.client));


This could cause an NPE if language is null. I suggest to create a ConnectionConfiguration.getLanguageTag() method which returns null if language is null. This is also the method the TODO comment should go to, which I am still missing.

Oh, right. The TODO comment! Sorry about that! Is there a particular reason why getLanguageTag should return null and not the default "en" value?

Sorry about that! Is there a particular reason why getLanguageTag should return null

It is not getLanguageTag() that I am worried about returning null, it is getLanguage(). Currently it is possible to set the language to null in the configuration, and that is desirable given that xml:lang in stream open elements is optional. But the code should take that into consideration and not throw NPEs in that, arguably rare, case.

Understood. The questions that I'm looking at, are:

is it reasonable to allow the user to supply a null Locale?

what is the expected outcome in that case? "en", "" or no xml:lang tag at all?

is it reasonable to allow the user to supply a null Locale?

I think so, especially for integration tests and the like. The point is, the standard is designed with an non-existing xml:lang in the stream open element in mind, and a library implementing the standard should allow for it. Of course, users deliberately providing a null value here, go down the "you should know what you are doing" road. And that is why Smack defaults to a non-empty xml:lang.

what is the expected outcome in that case? "en", "" or no xml:lang tag at all?

No xml:lang at all, otherwise the user would have provided Locale.ENGLISH for "en" or a Locale which returns the empty string as language tag for "".

The latter case is especially interesting: values where the empty string is allowed often cause pain, as, for example, their semantic is usually not as clearly defined as people believe to be or cause different behavior by different implementations. I really would like to be able to investigate how server implementations behave if a stream open with xml:lang="" is send (AFAIKT it is not explicitly forbidden by the standard). That said, given the current design we are aiming for, it is not possible to configure Smack to do so. That is because

jshell> var l = new java.util.Locale(""); l ==> jshell> l.getLanguage() $3 ==> "" jshell> l.toLanguageTag() $4 ==> "und"

so once we use toLangaugeTag() which is the goal, using a Locale constructed with the empty string as sole argument, will cause toLanguageTag() to return "und" (which I assume stands for 'undefined'). Hence the stream open will carry xml:lang="und".

That means we should probably not introduce a method ConnectionConfiguration.getLanguageTag(), but ConnectionConfiguration.getXmlLang(), which

returns null if language is null

returns "" if language.getLanguage() returns the empty string

returns language.toLanguageTag() otherwise

GitCop · 2019-07-31T10:23:55Z

There were the following issues with your Pull Request

Commit: ffd3432
Your subject line is longer than 72 characters

Guidelines are available at https://github.com/igniterealtime/Smack/wiki/Guidelines-for-Smack-Developers-and-Contributors

This message was auto-generated by https://gitcop.com

Flowdalic · 2019-07-31T10:57:16Z

smack-core/src/main/java/org/jivesoftware/smack/ConnectionConfiguration.java

+     * @return the stream language to use when connecting to the server.
+     */
+    public String getXmlLang() {
+        // TODO: use language.getLanguageTag() once we are on Java1.8 / Android r21+


Could you please use the exact comment string as I mentioned? I using this "pattern" allows me to grep Smack's source code for those situations and eventually fix them once the minimum Android API level was raised.

Flowdalic · 2019-07-31T11:02:10Z

smack-core/src/main/java/org/jivesoftware/smack/ConnectionConfiguration.java

+    public String getXmlLang() {
+        // TODO: use language.getLanguageTag() once we are on Java1.8 / Android r21+
+        // This will need a workaround for new Locale("").getLanguageTag() returning "und".
+        // Discussion in https://github.com/igniterealtime/Smack/pull/325


I would prefer an in-code comment summarizing the discussion, but this is also ok.

GitCop · 2019-07-31T11:26:46Z

There were the following issues with your Pull Request

Commit: ffd3432
Your subject line is longer than 72 characters

Guidelines are available at https://github.com/igniterealtime/Smack/wiki/Guidelines-for-Smack-Developers-and-Contributors

This message was auto-generated by https://gitcop.com

Flowdalic · 2019-08-01T21:02:05Z

This looks like it is good to go. Please squash the commits.

…PConnection This patch makes it possible to change the stream-level language as part of the connection configuration, to allow a properly implemented entities to provide i18n'ed response messages. The Locale type is used for this configuration, and the effective language string can be obtained via `ConnectionConfiguration.getXmlLang()`. This code does not cover XMPPBOSHConnection! Signed-off-by: Georg Lukas <georg@op-co.de>

GitCop · 2019-08-03T08:20:04Z

There were the following issues with your Pull Request

Commit: faaf8aa
Your subject line is longer than 72 characters

Guidelines are available at https://github.com/igniterealtime/Smack/wiki/Guidelines-for-Smack-Developers-and-Contributors

This message was auto-generated by https://gitcop.com

Flowdalic · 2019-08-03T20:44:19Z

Applied with minor modifications as 6a0e0f0

Thanks for your contribution.

Flowdalic requested changes Jul 27, 2019

View reviewed changes

ge0rg force-pushed the master branch from 548d19d to 65e515f Compare July 30, 2019 13:35

Flowdalic reviewed Jul 30, 2019

View reviewed changes

Flowdalic requested changes Jul 30, 2019

View reviewed changes

Flowdalic requested changes Jul 31, 2019

View reviewed changes

Flowdalic added the Needs squashing label Aug 1, 2019

ge0rg force-pushed the master branch from a706ab2 to faaf8aa Compare August 3, 2019 08:20

Flowdalic closed this Aug 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement xml:lang support in ConnectionConfiguration and AbstractXMPPConnection #325

Implement xml:lang support in ConnectionConfiguration and AbstractXMPPConnection #325

ge0rg commented Jul 26, 2019

GitCop commented Jul 26, 2019

Flowdalic Jul 27, 2019

ge0rg Jul 27, 2019

Flowdalic Jul 27, 2019 •

edited

Flowdalic Jul 27, 2019

GitCop commented Jul 29, 2019

ge0rg commented Jul 29, 2019

Flowdalic commented Jul 29, 2019

GitCop commented Jul 30, 2019

Flowdalic Jul 30, 2019

ge0rg Jul 30, 2019

vanitasvitae Jul 30, 2019

Flowdalic Jul 30, 2019

ge0rg Jul 31, 2019

Flowdalic Jul 30, 2019

ge0rg Jul 31, 2019

Flowdalic Jul 31, 2019

ge0rg Jul 31, 2019

Flowdalic Jul 31, 2019 •

edited

GitCop commented Jul 31, 2019

Flowdalic Jul 31, 2019

Flowdalic Jul 31, 2019

GitCop commented Jul 31, 2019

Flowdalic commented Aug 1, 2019

GitCop commented Aug 3, 2019

Flowdalic commented Aug 3, 2019

Implement xml:lang support in ConnectionConfiguration and AbstractXMPPConnection #325

Implement xml:lang support in ConnectionConfiguration and AbstractXMPPConnection #325

Conversation

ge0rg commented Jul 26, 2019

GitCop commented Jul 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flowdalic Jul 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GitCop commented Jul 29, 2019

ge0rg commented Jul 29, 2019

Flowdalic commented Jul 29, 2019

GitCop commented Jul 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flowdalic Jul 31, 2019 • edited

Choose a reason for hiding this comment

GitCop commented Jul 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GitCop commented Jul 31, 2019

Flowdalic commented Aug 1, 2019

GitCop commented Aug 3, 2019

Flowdalic commented Aug 3, 2019

Flowdalic Jul 27, 2019 •

edited

Flowdalic Jul 31, 2019 •

edited