Status

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: clokep, Assigned: clokep)

Tracking

trunk
0.3b1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [0.3-blocking-beta])

Attachments

(1 attachment, 3 obsolete attachments)

*** Original post on bio 680 at 2011-02-03 21:26:00 UTC ***

+++ This bug was initially created as a clone of Bug #954112 (bio 677) +++

The Twitter code must be localized to be part of an official release.

Note that it's possible a general way of localizing protocols can be done, which would be very helpful for all js-proto work.
Blocks: 954035
Whiteboard: [0.3-blocking]
Whiteboard: [0.3-blocking] → [0.3-blocking-beta]
*** Original post on bio 680 at 2011-05-31 02:19:49 UTC ***

I'm working on this. I mostly have it done (I think), just a localization question. Do we localize:
1. Things like "Twitter" or "twitter.com"?
2. Things inside error messages (i.e. throw or dump messages)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
*** Original post on bio 680 at 2011-05-31 09:33:19 UTC ***

(In reply to comment #1)
> I'm working on this. I mostly have it done (I think), just a localization
> question. Do we localize:
> 1. Things like "Twitter" or "twitter.com"?

No.

> 2. Things inside error messages (i.e. throw or dump messages)

The error messages displayed in the account manager should be localized. Those displayed in the error console only can stay in US English.
Posted patch v1.0 (obsolete) — Splinter Review
*** Original post on bio 680 as attmnt 676 at 2011-05-31 21:33:00 UTC ***

I believe this translates all the strings (I double and triple checked, but there might be one hidden in there I missed. :()

I mostly just pulled the strings out of twitter.js, they shouldn't really need to be changed.
Attachment #8352419 - Flags: review?(florian)
Comment on attachment 8352419 [details] [diff] [review]
v1.0

*** Original change on bio 680 attmnt 676 at 2011-05-31 22:41:19 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352419 - Flags: review?(florian) → review-
*** Original post on bio 680 at 2011-05-31 22:41:20 UTC ***

Comment on attachment 8352419 [details] [diff] [review] (bio-attmnt 676)
v1.0

>diff --git a/purple/locales/en-US/twitter.properties b/purple/locales/en-US/twitter.properties

These first 2 strings appear as system messages inside conversations:
>+error.tooLong=Status is over 140 characters.
>+error.general=An error %S occured while sending: %S
This definitely needs a localization note explaining what the %S will be replace by.

This is a connection error which will appear in the account manager.
>+error.userMismatch=Username mismatch.

>+# LOCALIZATION NOTE (timeline):
>+#   This is the title of the conversation tab.
>+timeline=%S timeline
Tell the localizer what %S will be replaced by. I think it's @<username> here.

>+# LOCALIZATION NOTE (connection.*):
>+#   These will show in the account manager during different connection states.
>+connection.receivedToken=Received request token.
>+connection.failedToken=Failed to get request token.
>+connection.authPrompt=Give permission to use your Twitter account
>+connection.authCancelled=Authorization process cancelled.
>+connection.authFailed=Failed to get authorization.
>+connection.authReceived=Received access token.

Some of these strings will appear during connection, to indicate progress.
Some are error messages indicating why the account couldn't be connected.
What's the rational for a string being error.* or connection.* here?
Maybe use connection.progress.* and connection.error.* ?

For the error messages, it's probably OK to have strings that contain specific technical words about the underlying issue, but connection progress messages should be more descriptive I think.
And maybe we should be indicating what's going on (especially at the beginning of something that may take a while, like and HTTP request), rather than what's finished (what's finished sounds more like things we should LOG to the error console).


>+
>+# LOCALIZATION NOTE (options.*):
>+#   These will show in the protocol options menu.

Is there a protocol options menu anywhere?

-> "These are the protocol specific options shown in the account manager and account wizard windows."? Not really a great wording either :-(.

>diff --git a/purple/purplexpcom/src/twitter.js b/purple/purplexpcom/src/twitter.js

>@@ -341,42 +346,43 @@ Account.prototype = {
>   requestToken: function() {

Maybe add a connection progress message here saying "Initiating authentication process..."? Or "Preparing authentication"?

>     let oauthParams =
>       [["oauth_callback", encodeURIComponent(this.completionURI)]];
>     this.signAndSend("oauth/request_token", null, [],
>                      this.onRequestTokenReceived, this.onError, this,
>                      oauthParams);
>   },
>   onRequestTokenReceived: function(aData) {
>-    this.base.connecting("Received request token.");
>+    this.base.connecting(bundle.GetStringFromName("connection.receivedToken"));
This should probably be logged instead.

>     let data = this._parseURLData(aData);
>     if (!data.oauth_callback_confirmed ||
>         !data.oauth_token || !data.oauth_token_secret) {
>       this.gotDisconnected(this._base.ERROR_OTHER_ERROR,
>-                           "Failed to get request token.");
>+                           bundle.GetStringFromName("connection.failedToken"));
>       return;
>     }
>     this.token = data.oauth_token;
>     this.tokenSecret = data.oauth_token_secret;
> 
>     this.requestAuthorization();
>   },
>   requestAuthorization: function() {

Progress: "Waiting for your authorization..."?

>     const url = this.baseURI + "oauth/authorize?oauth_token=";
>     this._browserRequest = {
>-      promptText: "Give permission to use your Twitter account",
>+      get promptText() bundle.GetStringFromName("connection.authPrompt"),

This string is not displayed in the account manager, so the connection.* may be misleading here.

>-        this.account.gotDisconnected(this.account._base.ERROR_AUTHENTICATION_FAILED,
>-                                     "Authorization process cancelled.");
>+        this.account
>+            .gotDisconnected(this.account._base.ERROR_AUTHENTICATION_FAILED,
>+                             bundle.GetStringFromName("connection.authCancelled"));

Should this be made more explicit that the error is caused by a user action ("You cancelled ...")?

>       },
>       loaded: function(aWindow, aWebProgress) {
>         if (!this._active)
>           return;
> 
>         this._listener = {
>           QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener,
>                                                  Ci.nsISupportsWeakReference]),
>@@ -423,31 +429,31 @@ Account.prototype = {
>     if ("_listener" in this._browserRequest)
>       this._browserRequest._listener._cleanUp();
>     delete this._browserRequest;
>   },
>   onAuthorizationReceived: function(aData) {
>     let data = this._parseURLData(aData.split("?")[1]);
>     if (data.oauth_token != this.token || !data.oauth_verifier) {
>       this.gotDisconnected(this._base.ERROR_OTHER_ERROR,
>-                           "Failed to get authorization.");
>+                           bundle.GetStringFromName("connection.authFailed"));
>       return;
>     }
>     this.requestAccessToken(data.oauth_verifier);
>   },
>   requestAccessToken: function(aTokenVerifier) {

Progress: "Finalizing authentication"?

>     this.signAndSend("oauth/access_token", null, [],
>                      this.onAccessTokenReceived, this.onError, this,
>                      [["oauth_verifier", aTokenVerifier]]);
>   },
>   onAccessTokenReceived: function(aData) {
>-    this.base.connecting("Received access token.");
>+    this.base.connecting(bundle.GetStringFromName("connection.authReceived"));

LOG instead?


We need at least one progress message in getTimelines. It may be nice to update the connection progress each time we receive the response for one of our timeline requests, but that may not be really needed.
Posted patch v2.0 (obsolete) — Splinter Review
*** Original post on bio 680 as attmnt 683 at 2011-06-02 00:34:00 UTC ***

This is a second cut, it adds a few strings and makes a few of them LOG calls (and removes them from the localization file).

Apparently my last patch missed my changes to jar.mn, I decided to put twitter.properties just under the purple folder, but I could do it was twitter/prpl.properties as all the other protocols use.

I put a requesting timelines string in, but not one when they're received since (hopefully) soon after that we should just be "Connected". I'm not sure what other people think of this.
Attachment #8352426 - Flags: review?(florian)
Comment on attachment 8352419 [details] [diff] [review]
v1.0

*** Original change on bio 680 attmnt 676 at 2011-06-02 00:34:32 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352419 - Attachment is obsolete: true
Comment on attachment 8352426 [details] [diff] [review]
v2.0

*** Original change on bio 680 attmnt 683 at 2011-06-02 07:53:32 UTC ***

>diff --git a/purple/locales/en-US/twitter.properties b/purple/locales/en-US/twitter.properties
>new file mode 100644
>--- /dev/null
>+++ b/purple/locales/en-US/twitter.properties
>@@ -0,0 +1,36 @@
>+# LOCALIZATION NOTE (error.*)
>+#   These are errors that will be shown to the user in conversation.
>+#   error.general:
>+#     First %S is the error (as returned by Twitter, will be in English).
>+#     Second %S is the sent message.
>+error.tooLong=Status is over 140 characters.
>+error.general=An error %S occured while sending: %S

Put the comment about error.general directly above it, not inside the general comment for the 2 error.* strings.

The comment should be more specific about the replacements.
The first %S will be either the error string returned by the twitter server, in English, inside parenthesis, or the empty string if we have no specific message for the error.
The second %S is the message that caused the error.

Firefox's spellchecker tells me 'occured' should be 'occurred' here.

>+# LOCALIZATION NOTE (connection.*)
>+#   These will show in the account manager to show progress of the connection.

(Useless remark: "will show ... to show" sounds a bit redundant to me. They will appear/be displayed in ... ?)

>+connection.initAuth=Initiating authentication process.
>+connection.requestAuth=Waiting for your authorization.
>+connection.requestAccess=Finalizing authentication.
>+connection.requestTimelines=Requesting user timelines.

These strings will be displayed using:
account.connection.progress=Connecting: %...

So don't include the end period in them :).

>+# LOCALIZATION NOTE (connection.error.*)
>+#   These will show in the account manager if an error occurs while connection.

while "connecting"? "trying to connect"? "attempting to connect"?

>+connection.error.authCancelled=You cancelled the authorization process!

I'm not sure users will like the "!" here ;).

>diff --git a/purple/locales/jar.mn b/purple/locales/jar.mn

> % locale novell @AB_CD@ %locale/@AB_CD@/novell/
> 	locale/@AB_CD@/novell/prpl.properties	(%novell.properties)
>+	locale/@AB_CD@/purple/twitter.properties	(%twitter.properties)

purple/twitter.properties is fine with me.

Just for information, the point of using <protocolname>/prpl.properties is that <protocolname> is used as the package for the chrome registry, allowing the protocol to be moved into an add-on without requiring any code change.
An add-on can't (cleanly) add a file inside the purple package which already exists.

I don't think we need to care about this here, but it may matter to you later for the JS-IRC work if we land something and you decide to experiment with new features in an add-on replacing the default JS-IRC component (though you can also change the path to the locale file in the code of the add-on if that's easier).



>diff --git a/purple/purplexpcom/src/twitter.js b/purple/purplexpcom/src/twitter.js

The changes in this file look pretty good to me! :)

Only missing detail is you forgot to include the changes to the onError method and to jsProtoHelper that we discussed to avoid having a single localizable string there, and a non-sense error message in case HTTP connection can't be established because the user is offline.
Attachment #8352426 - Flags: review?(florian) → review-
Posted patch v2.1 (obsolete) — Splinter Review
*** Original post on bio 680 as attmnt 689 at 2011-06-03 01:19:00 UTC ***

I added the change to jsProtoHelper (I think this is what we decided on), the string I added makes sense to one of my not very computer savvy friends.

I fixed some of the localization notes too, hopefully they make a bit more sense now. :)
Attachment #8352432 - Flags: review?(florian)
Comment on attachment 8352426 [details] [diff] [review]
v2.0

*** Original change on bio 680 attmnt 683 at 2011-06-03 01:19:01 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352426 - Attachment is obsolete: true
Posted patch Patch (v3?)Splinter Review
*** Original post on bio 680 as attmnt 694 at 2011-06-03 10:18:00 UTC ***

The patch looks quite good. I tweaked a localization note, but at this point it's almost bikeshedding.
I see you have struggled in 2 or 3 places with the indentation to keep the lines under 80 columns. In this case, I think I would have preferred having some lines of 85 or so columns rather than weired indents. Anyway, this lead me to the (already known) conclusion that the current string bundle API completely sucks, and I've written a JS helper to relieve the pain. I'd like your feedback on this :).
Attachment #8352437 - Flags: review?(clokep)
Comment on attachment 8352437 [details] [diff] [review]
Patch (v3?)

*** Original change on bio 680 attmnt 694 at 2011-06-03 10:27:19 UTC ***

Looks good! The JS API for the string bundles simplifies the code a lot. :)
Attachment #8352437 - Flags: review?(clokep) → review+
Comment on attachment 8352432 [details] [diff] [review]
v2.1

*** Original change on bio 680 attmnt 689 at 2011-06-03 10:33:27 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352432 - Attachment is obsolete: true
Attachment #8352432 - Flags: review?(florian)
Comment on attachment 8352432 [details] [diff] [review]
v2.1

*** Original change on bio 680 attmnt 689 at 2011-06-03 10:34:31 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352432 - Flags: review+
*** Original post on bio 680 at 2011-06-03 10:39:09 UTC ***

https://hg.instantbird.org/instantbird/rev/5ea81eb82790
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.3b1
*** Original post on bio 680 at 2011-06-08 17:55:42 UTC ***

Follow-up: https://hg.instantbird.org/instantbird/rev/d2f37c50fdba - export 'LOG' in jsProtoHelper.
Component: General → Twitter
You need to log in before you can comment on or make changes to this bug.