742524 - Expose mail.compose.big_attachments.insertNotification in the preferences dialog

Status: RESOLVED FIXED

(Thunderbird :: FileLink, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

mail.compose.big_attachments.insertNotification controls whether or not we display a URL insertion notification once uploads begin.

This defaults to true.  We should allow users to pref it off in the Preferences dialog.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Note that this preference has been changed to mail.compose.big_attachments.insert_notification.

Comment 2

[zeyu [:car]]

hi, 

I would like to do the first contribution. Can you pls guide me to start? thanks.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Hey zeyu!

The first thing you'll need to do is get a Thunderbird build going - see https://developer.mozilla.org/en-US/docs/Simple_Thunderbird_build

Once you've done that, come find me in irc.mozilla.org in #maildev - I'm mconley. I'll coach you through the rest.

Thanks!

-Mike

Comment 4

[zeyu [:car]]

Hi Mike,

I looked for you on irc several times, it seems that our time zone is not compatible :(. I have built thunderbird successfully, what should I do next? Thanks.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

(In reply to zeyu [:car] from comment #4)
> Hi Mike,
> 
> I looked for you on irc several times, it seems that our time zone is not
> compatible :(. I have built thunderbird successfully, what should I do next?
> Thanks.

Next, you need to add the preference checkbox.

The way I see it, the checkbox could / should go beneath the "Offer to share files larger than" checkbox.

The code for that dialog that you should be interested in is here:

http://mxr.mozilla.org/comm-central/source/mail/components/preferences/applications.xul#159

The line I've highlighted is the start of the containment box for the "Offer to share files larger than" checkbox.

Add something similar below it for our new toggle. You'll also need to add a corresponding <preference> tag in here: http://mxr.mozilla.org/comm-central/source/mail/components/preferences/applications.xul#44

(See https://developer.mozilla.org/en-US/docs/XUL/preference for more details on that)

Finally, you'll want to make sure you add translation strings for the new checkbox label. You'll want to do that in here:

http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/preferences/applications.dtd

Once you've got that done, follow the instructions here to generate a patch and attach it to this bug, and then set me as the reviewer.

https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch

If you have any questions, feel free to send me mail. Thanks for the help!

Comment 6

[zeyu [:car]]

Attachment: add the check box DisplayURL insertion notification

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 656819 [details] [diff] [review]
add the check box DisplayURL insertion notification

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

Thanks for the patch, car!

So I don't think this works exactly - when I check the box, the preference doesn't seem to be set. I think the problem is an erroneous preference name / id in the <preference> XUL tag.

See other notes below.

::: mail/components/preferences/applications.xul
@@ +50,5 @@
>                    type="int"/>
>      </preferences>
>  
> +    <preferences id="displayURLPreferences">
> +      <preference id="mail.compose.big_attachements.insert_notification"

This doesn't work, because the id is wrong - it should be "big_attachments" as opposed to "big_attachements".  Same below.

::: mail/locales/en-US/chrome/messenger/preferences/applications.dtd
@@ +35,5 @@
>  <!ENTITY authRequired.button.accesskey    "U">
>  
>  <!ENTITY enableCloudFileAccountOffer.label "Offer to share for files larger than">
>  <!ENTITY enableCloudFileAccountOffer.mb "MB">
> +<!ENTITY enableDisplayURL.label "Display URL insertion notification">

I think I'd prefer, "Let me know when uploads begin"

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Blake:

Here's an idea of where the toggle would be placed:

http://i.imgur.com/eKpd8.png

I'm hemming and hawing about this now...are we at risk of making the dialog too confusing, with too many toggles?

Comment 9

[zeyu [:car]]

Attachment: correct the spelling error, change the translation string

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 657364 [details] [diff] [review]
correct the spelling error, change the translation string

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

Hey car,

Thanks - can you base this patch off of trunk, as opposed to your last patch?

-Mike

Comment 11

[zeyu [:car]]

Attachment: regenerate the patch

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Hey car,

So I tried your patch, and bwinton and I talked it over a bit, and we're going to have to change requirements a little bit on this one (sorry!).

What we'd like instead is for the preference to be maybe exposed as part of the notification bar itself:

http://i.imgur.com/ABKHV.png

Interested in tackling it with that approach?

-Mike

-Mike

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Here's where the notification is inserted (and where you can specify buttons):

http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/bigFileObserver.js#267

And here's an example where we've given a notification some buttons (the example is for the attachment reminder notification):

http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#1946

Let me know if you have any questions!

Comment 14

[zeyu [:car]]

Hi Mike,
I think I added this button successfully. This button now can set the notification to be false.
Things need to clarify:
Do we keep the toggle added in previous patch?
If not, how can we set mail.compose.big_attachments.insert_notification default to true?

Comment 15

[Mike Conley (:mconley) (:⚙️)]

(In reply to zeyu [:car] from comment #14)
> Hi Mike,
> I think I added this button successfully. This button now can set the
> notification to be false.

Awesome! That was fast! :)

> Things need to clarify:
> Do we keep the toggle added in previous patch?

No, I think we should remove it.

> If not, how can we set mail.compose.big_attachments.insert_notification
> default to true?

I don't think the use case is common enough to warrant exposing a toggle in the UI. In the event that a user *does* want to bring the notification back, they can manually flip the pref in the Config Editor (Preferences > Advanced > Config Editor), which is similar to Firefox's about:config.

I can't wait to see your patch! :)

Comment 16

[zeyu [:car]]

Attachment: added button to uploading bar

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 660831 [details] [diff] [review]
added button to uploading bar

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

This is definitely the right idea. Just a few style nits, and instructions for l10n are below.

Thanks!

-Mike

::: mail/components/compose/content/bigFileObserver.js
@@ +262,5 @@
>        return;
>  
>      let message = this.formatString("cloudFileUploadingNotification");
>      message = PluralForm.get(aAttachments.length, message);
> +    var showUploadButton = {

We prefer let over var. Also, some style nits: we generally don't put a space before colons, and we tend to do two-space indentation.

@@ +263,5 @@
>  
>      let message = this.formatString("cloudFileUploadingNotification");
>      message = PluralForm.get(aAttachments.length, message);
> +    var showUploadButton = {
> +	    accessKey : "N",

In order to localize these, you need to add strings to /mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties.

You'll want to add something called "stopShowingUploadingNotification.label" and "stopShowingUploadingNotification.accesskey".

You should then be able to refer to them here in the Javascript with this.formatString("stopShowingUploadingNotification.label") and this.formatString("stopShowingUploadingNotification.accesskey").

Comment 18

[zeyu [:car]]

Attachment: changed the compseMsgs.properties

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 660842 [details] [diff] [review]
changed the compseMsgs.properties

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

Excellent! This works really well. r=me, and ui-r=me.

Just one small nit before it's ready to go in - can you fix the localization note? See my comment.

Also, can you update the patch commit message to say this:

Bug 742524 - Let users dismiss Filelink upload notification permanently.

Once that's done, we'll set checkin-needed, and this thing can get committed.

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +438,5 @@
>  ## %3$S is the name of the cloud storage service, and %4$S is the link to the
>  ## attachment.
>  cloudAttachmentListItem=* %1$S (%2$S) hosted on %3$S: %4$S
> +
> +## LOCALIZATION NOTE(stopShowingUploadingNotification): A string used on a button in

I think this is clearer:

This string is used in the Filelink upload notification bar to allow the user to dismiss the notification permanently.

Comment 20

[zeyu [:car]]

Attachment: Bug 742524 - Let users dismiss Filelink upload notification permanently.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 660851 [details] [diff] [review]
Bug 742524 - Let users dismiss Filelink upload notification permanently.

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

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +438,5 @@
>  ## %3$S is the name of the cloud storage service, and %4$S is the link to the
>  ## attachment.
>  cloudAttachmentListItem=* %1$S (%2$S) hosted on %3$S: %4$S
> +
> +## LOCALIZATION NOTE(stopShowingUploadingNotification): This string is used in the Filelink 

Ok, last thing to fix - just remove this piece of whitespace, and you're good. :)

Comment 22

[zeyu [:car]]

Attachment: Bug 742524 - Let users dismiss Filelink upload notification permanently

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 660857 [details] [diff] [review]
Bug 742524 - Let users dismiss Filelink upload notification permanently

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

r+ui-r=me. Thanks car!

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/f6501add98ac