871697 - GCLI disconnectDesc and disconnectManual strings are wrong

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[Joe Walker [:jwalker]]

clones of connectDesc and connectManual

Comment 1

[Eduardo Trápani]

connectManual and disconnectManual have the same string:

connectManual: Connect to the server, creating local versions of the commands on the server. Remote commands initially have a prefix to distinguish them from local commands (but see the context command to get past this)

disconnectManual: Connect to the server, creating local versions of the commands on the server. Remote commands initially have a prefix to distinguish them from local commands (but see the context command to get past this)

Comment 2

[Théo Chevalier]

I think that since those strings are already on Aurora (which is string frozen), this trivial fix should be landed ASAP.

Btw, connectDesc and disconnectDesc seems to have the same problem.

Comment 3

[Francesco Lodolo [:flod]]

I'm trying to review and fix the comments in gcli.properties, but I'm not sure this is a trivial fix.

Why a "disconnect" command should need the "prefix" parameter?

Comment 4

[Joe Walker [:jwalker]]

(In reply to Francesco Lodolo [:flod] from comment #3)
> I'm trying to review and fix the comments in gcli.properties, but I'm not
> sure this is a trivial fix.
> 
> Why a "disconnect" command should need the "prefix" parameter?

Connections have names to enable you to be connected to more than one system at a time.

Comment 5

[Francesco Lodolo [:flod]]

Attachment: Review strings and l10n comment, fix disconnectDesc and disconnectManual

The approach to this file is a bit more aggressive than what I've done for other properties files in devtools. This is the reason: for 101 entities, the current file has 312 lines of comments, while the updated version has only 135.

This patch tries to:
* Remove all repetitions about *Desc being a short description and *Manual being a longer description of a command, replace them with a single explanation at the beginning of the file.
* Group together similar/connected strings and add only one comment for the group.
* Replace %1$S with %S when only one parameter is used.

To fix specifically this bug, it also replaces disconnectDesc and disconnectManual. I'm using a numbered suffix instead of a more explanatory name to maintain the structure *command*Desc and *command*Manual. 

Joe: please check if the proposed strings are correct.

I've also asked feedback from Pike and Théo to understand if I'm oversimplifying things.

Comment 6

[Axel Hecht [:Pike]]

Comment on attachment 757832 [details] [diff] [review]
Review strings and l10n comment, fix  disconnectDesc and disconnectManual

Review of attachment 757832 [details] [diff] [review]:
-----------------------------------------------------------------

This is generally OK, but two things:

The disconnect command is totally confusing.

More to the patch, whenever a localization note applies to more than one entity, please list them all, see the comment inline on the first occurance. There are loads of them, I didn't comment on the following.

::: browser/locales/en-US/chrome/browser/devtools/gcli.properties
@@ +28,5 @@
>  # parameters.
>  canonDefaultGroupName=Options
>  
> +# LOCALIZATION NOTE: these commands are used to execute commands on a remote
> +# system (using a proxy). Parameters: %S is the name of the remote system.

If a note is for two or more entities, it should reference them in ().

Notes with no () implicitly only refer to the following entity.

@@ +58,1 @@
>  fieldSelectionSelect=Select a %S…

What's %S here?

Note, this may very well not work in languages with grammar without extra stunts, thus it's even more important to say what %S really is.

Comment 7

[Francesco Lodolo [:flod]]

Comment on attachment 757832 [details] [diff] [review]
Review strings and l10n comment, fix  disconnectDesc and disconnectManual

No point in asking a review on this patch, clearing flags.

Comment 8

[Francesco Lodolo [:flod]]

(In reply to Axel Hecht [:Pike] from comment #6)
> The disconnect command is totally confusing.
I agree, but I couldn't find something more clear.

> There are loads of them, I didn't comment on the following.
> If a note is for two or more entities, it should reference them in ().
Ok, I thought the exact opposite.

> fieldSelectionSelect=Select a %S…
> Note, this may very well not work in languages with grammar without extra
> stunts, thus it's even more important to say what %S really is.

Good question, I didn't check this. 

jwalker, any help on this and the disconnect string/comment?

Comment 9

[Francesco Lodolo [:flod]]

Attachment: Patch addressing pike's comments

This patch should address all notes made in comment 6.

I didn't touch fieldSelectionSelect's comment: I talked with jwalker about this string on IRC, and since %S could be a lot of things (connection, name, file, etc.) I'll file a separate bug for changing that to a more generic "Please select…" or similar (at this moment this string is not in use).

Comment 10

[Francesco Lodolo [:flod]]

> I didn't touch fieldSelectionSelect's comment: I talked with jwalker about
> this string on IRC, and since %S could be a lot of things (connection, name,
> file, etc.) I'll file a separate bug for changing that to a more generic
> "Please select…" or similar (at this moment this string is not in use).

That would be bug 880155

Comment 11

[Joe Walker [:jwalker]]

Comment on attachment 759018 [details] [diff] [review]
Patch addressing pike's comments

Review of attachment 759018 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for looking over these strings.

I've done the work to make my script generate output similar to your proposals, which I'll attach.

There are some changes that I wasn't sure about - Some of the strings were re-ordered (e.g. some, but not all, of the prefSet strings were moved after the prefReset strings). And I've restored the capital letters at the start of comments, but nothing significant.

Comment 12

[Joe Walker [:jwalker]]

Attachment: Revert key ordering, and use initial capitals

Comment 13

[Francesco Lodolo [:flod]]

(In reply to Joe Walker [:jwalker] from comment #11)

> There are some changes that I wasn't sure about - Some of the strings were
> re-ordered (e.g. some, but not all, of the prefSet strings were moved after
> the prefReset strings).

Thanks Joe. In general I tried to group string to reduce the number of comments (e.g. max, min, ecc.), it could be that I missed some in the "pref *" commands.

Comment 14

[Francesco Lodolo [:flod]]

Joe, what's next here?

Comment 15

[Joe Walker [:jwalker]]

Phew - sorry for the delay :flod.

Comment 16

[Joe Walker [:jwalker]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=93660c241ba4
Fx-Team: https://tbpl.mozilla.org/?tree=Fx-Team&rev=7c3d5bdc569c
HG Revision: https://hg.mozilla.org/integration/fx-team/rev/7c3d5bdc569c

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/7c3d5bdc569c