1147006 - TB shows instructions with [File] - [Offline] - [Synchronize] instead of [Download/Sync Now]

Status: RESOLVED FIXED

Description

[Tim [:pollti]]

Attachment: patch

When TB is offline and a message body could not be downloaded, TB shows the following message:
>The body of this message has not been downloaded from
>the server for reading offline. To read this message,
>you must reconnect to the network, choose Offline from
>the File menu and then uncheck Work Offline.
>In the future, you can select which messages or folders to read offline. To do
>this, choose Offline from the file menu and then select Synchronize. You can
>adjust the Disk Space preference to prevent the downloading of large messages.
"Synchronize" should be replaced by "Download/Sync Now…"

Comment 1

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8582517 [details] [diff] [review]
patch

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

Other than the nit below, I like it.
Adding Neil for the review of the Suite code.

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +306,5 @@
>  the server for reading offline. To read this message, \
>  you must reconnect to the network, choose Offline from \
>  the File menu and then uncheck Work Offline. \
>  In the future, you can select which messages or folders to read offline. To do \
> +this, choose Offline from the file menu and then select Download/Sync Now…. \

I would remove the … from this, since it doesn't add any benefit, and looks confusing.

Comment 2

[[e-mail]]

Comment on attachment 8582517 [details] [diff] [review]
patch

Whoa, regression from bug 71125 ;-)

Unfortunately due to our sucky l10n process we need to change the string key (here and in the C++) so that localisers notice the problem.

Comment 3

[Tim [:pollti]]

Attachment: patch #2

Thanks for your feedback! This removes the "…".
Neil, could you explain, what I have to do, please? (I'm sorry - I'm not very familiar with TB l10n update processes.)

Comment 4

[[e-mail]]

Comment on attachment 8583165 [details] [diff] [review]
patch #2

> nocachedbodybody=The body of this message has not been downloaded from \
The name "nocachedbodybody" identifies this message. Unfortunately if you change the message without changing the name, then localisers will not realise that the message has changed, as the l10n dashboard will only display differences in names. You therefore need to change the name, not only in these two files, but also in the C++ file that also references this message.

Comment 5

[Tim [:pollti]]

Attachment: patch, v3

Thanks! This should update the string's name in all the files.

Comment 6

[[e-mail]]

Comment on attachment 8584977 [details] [diff] [review]
patch, v3

> nocachedbodybodyupdated=The body of this message has not been downloaded from \
Somehow this isn't showing up as a change in your patch?
Also, when changing strings, we normally use a numeric suffix, in case the string ever needs to be changed again (i.e. the suffix is 2, 3, etc. for each subsequent change).

Comment 7

[Tim [:pollti]]

I don't see the reason for the not working changes at the moment. I'll try to fix it next week.

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8584977 [details] [diff] [review]
patch, v3

I'm going to wait for the working patch, but this seems pretty good so far.  ;)

Comment 9

[Tim [:pollti]]

Attachment: patch, v4

I'm sorry for my mistakes. Hopefully, this one will work properly.

Comment 10

[[e-mail]]

Comment on attachment 8588973 [details] [diff] [review]
patch, v4

This looks OK by code inspection; I didn't actually apply the patch.

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8588973 [details] [diff] [review]
patch, v4

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

Looks good to me too!

Comment 12

[Tim [:pollti]]

Thanks!
What are the next steps here? / Is there anything I can do?

Comment 13

[Blake Winton (:bwinton) (:☕️)]

Put "checkin-needed" in the keywords, and someone will check the code in for you!

Comment 14

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

Thank you for working on this. There are several issues with the patch, please fix them and resubmit (feel free to ping me on IRC if you have questions). Output from the console:

unable to find 'suite/locales/en-US/chrome/messenger/messenger.properties' for patching
1 out of 1 hunks FAILED -- saving rejects to file suite/locales/en-US/chrome/messenger/messenger.properties.rej
patching file mailnews/base/util/nsMsgIncomingServer.cpp
Hunk #1 FAILED at 1607
1 out of 1 hunks FAILED -- saving rejects to file mailnews/base/util/nsMsgIncomingServer.cpp.rej
patch failed, unable to continue (try -v)
cleaning up working directory...done
abort: invalid date: 'Mid Apr 07 10:32:00 2015 +0100'

Comment 15

[Tim [:pollti]]

Attachment: patch, v5

Two fixes. Thanks, Archaeopteryx!

Comment 16

[Tim [:pollti]]

Attachment: patch, v6

Comment 17

[Tim [:pollti]]

Attachment: patch, v7

[added missing whitspace]

Comment 18

[Tim [:pollti]]

Attachment: text as patch

Comment 19

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

http://hg.mozilla.org/comm-central/rev/bb995f40ce81

Comment 20

[:aceman]

Are string changes for TB38 still allowed ?

Comment 21

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

Ah, string changes. No, that's way too late.