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

RESOLVED FIXED in Thunderbird 18.0

Status

Thunderbird
FileLink
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mconley, Assigned: car)

Tracking

Trunk
Thunderbird 18.0
x86
All
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=mconley][lang=js|xul])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Whiteboard: [mentor=mconley][lang=js|xul]
(Reporter)

Updated

5 years ago
OS: Mac OS X → All
(Reporter)

Comment 1

5 years ago
Note that this preference has been changed to mail.compose.big_attachments.insert_notification.
(Reporter)

Updated

5 years ago
Component: Preferences → FileLink
QA Contact: preferences → filelink
(Assignee)

Comment 2

5 years ago
hi, 

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

Comment 3

5 years ago
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
(Assignee)

Comment 4

5 years ago
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.
(Reporter)

Comment 5

5 years ago
(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!
(Reporter)

Updated

5 years ago
Assignee: nobody → zeyufly
(Assignee)

Comment 6

5 years ago
Created attachment 656819 [details] [diff] [review]
add the check box DisplayURL insertion notification
Attachment #656819 - Flags: review?(mconley)
(Reporter)

Comment 7

5 years ago
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"
Attachment #656819 - Flags: review?(mconley) → review-
(Reporter)

Comment 8

5 years ago
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?
(Assignee)

Comment 9

5 years ago
Created attachment 657364 [details] [diff] [review]
correct the spelling error, change the translation string
Attachment #657364 - Flags: review?(mconley)
(Reporter)

Updated

5 years ago
Attachment #656819 - Attachment is obsolete: true
(Reporter)

Comment 10

5 years ago
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
Attachment #657364 - Flags: review?(mconley)
(Assignee)

Comment 11

5 years ago
Created attachment 658148 [details] [diff] [review]
regenerate the patch
Attachment #658148 - Flags: review?(mconley)
(Reporter)

Comment 12

5 years ago
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
(Reporter)

Updated

5 years ago
Attachment #658148 - Flags: review?(mconley)
(Reporter)

Comment 13

5 years ago
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!
(Assignee)

Comment 14

5 years ago
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?
(Reporter)

Comment 15

5 years ago
(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! :)
(Assignee)

Comment 16

5 years ago
Created attachment 660831 [details] [diff] [review]
added button to uploading bar
Attachment #660831 - Flags: review?(mconley)
(Reporter)

Comment 17

5 years ago
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").
Attachment #660831 - Flags: review?(mconley) → feedback+
(Assignee)

Comment 18

5 years ago
Created attachment 660842 [details] [diff] [review]
changed the compseMsgs.properties
Attachment #660842 - Flags: review?(mconley)
(Reporter)

Updated

5 years ago
Attachment #660831 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #658148 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #657364 - Attachment is obsolete: true
(Reporter)

Comment 19

5 years ago
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.
Attachment #660842 - Flags: ui-review+
Attachment #660842 - Flags: review?(mconley)
Attachment #660842 - Flags: review+
(Assignee)

Comment 20

5 years ago
Created attachment 660851 [details] [diff] [review]
Bug 742524 - Let users dismiss Filelink upload notification permanently.
Attachment #660851 - Flags: review?(mconley)
(Reporter)

Comment 21

5 years ago
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. :)
Attachment #660851 - Flags: review?(mconley)
(Assignee)

Comment 22

5 years ago
Created attachment 660857 [details] [diff] [review]
Bug 742524 - Let users dismiss Filelink upload notification permanently
Attachment #660857 - Flags: review?(mconley)
(Reporter)

Comment 23

5 years ago
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!
Attachment #660857 - Flags: ui-review+
Attachment #660857 - Flags: review?(mconley)
Attachment #660857 - Flags: review+
(Reporter)

Updated

5 years ago
Attachment #660842 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #660851 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/f6501add98ac
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0
You need to log in before you can comment on or make changes to this bug.