Last Comment Bug 742524 - Expose mail.compose.big_attachments.insertNotification in the preferences dialog
: Expose mail.compose.big_attachments.insertNotification in the preferences dialog
Status: RESOLVED FIXED
[mentor=mconley][lang=js|xul]
:
Product: Thunderbird
Classification: Client Software
Component: FileLink (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: Thunderbird 18.0
Assigned To: zeyu [:car]
:
Mentors:
Depends on: 738299
Blocks: BigFiles
  Show dependency treegraph
 
Reported: 2012-04-04 13:54 PDT by Mike Conley (:mconley) - (Needinfo me!)
Modified: 2012-09-13 14:08 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
add the check box DisplayURL insertion notification (3.31 KB, patch)
2012-08-30 04:54 PDT, zeyu [:car]
mconley: review-
Details | Diff | Splinter Review
correct the spelling error, change the translation string (2.35 KB, patch)
2012-08-31 11:00 PDT, zeyu [:car]
no flags Details | Diff | Splinter Review
regenerate the patch (3.01 KB, patch)
2012-09-04 10:38 PDT, zeyu [:car]
no flags Details | Diff | Splinter Review
added button to uploading bar (1.58 KB, patch)
2012-09-13 07:12 PDT, zeyu [:car]
mconley: feedback+
Details | Diff | Splinter Review
changed the compseMsgs.properties (2.83 KB, patch)
2012-09-13 07:57 PDT, zeyu [:car]
mconley: review+
mconley: ui‑review+
Details | Diff | Splinter Review
Bug 742524 - Let users dismiss Filelink upload notification permanently. (2.86 KB, patch)
2012-09-13 08:28 PDT, zeyu [:car]
no flags Details | Diff | Splinter Review
Bug 742524 - Let users dismiss Filelink upload notification permanently (2.86 KB, patch)
2012-09-13 08:47 PDT, zeyu [:car]
mconley: review+
mconley: ui‑review+
Details | Diff | Splinter Review

Description Mike Conley (:mconley) - (Needinfo me!) 2012-04-04 13:54:21 PDT
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) - (Needinfo me!) 2012-04-16 12:08:47 PDT
Note that this preference has been changed to mail.compose.big_attachments.insert_notification.
Comment 2 zeyu [:car] 2012-08-17 23:55:45 PDT
hi, 

I would like to do the first contribution. Can you pls guide me to start? thanks.
Comment 3 Mike Conley (:mconley) - (Needinfo me!) 2012-08-20 09:01:43 PDT
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] 2012-08-24 09:46:51 PDT
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) - (Needinfo me!) 2012-08-27 07:07:24 PDT
(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] 2012-08-30 04:54:14 PDT
Created attachment 656819 [details] [diff] [review]
add the check box DisplayURL insertion notification
Comment 7 Mike Conley (:mconley) - (Needinfo me!) 2012-08-31 06:35:09 PDT
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) - (Needinfo me!) 2012-08-31 06:36:01 PDT
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] 2012-08-31 11:00:39 PDT
Created attachment 657364 [details] [diff] [review]
correct the spelling error, change the translation string
Comment 10 Mike Conley (:mconley) - (Needinfo me!) 2012-09-04 08:10:46 PDT
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] 2012-09-04 10:38:10 PDT
Created attachment 658148 [details] [diff] [review]
regenerate the patch
Comment 12 Mike Conley (:mconley) - (Needinfo me!) 2012-09-12 13:33:55 PDT
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) - (Needinfo me!) 2012-09-12 13:39:43 PDT
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] 2012-09-13 04:09:34 PDT
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) - (Needinfo me!) 2012-09-13 06:34:23 PDT
(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] 2012-09-13 07:12:51 PDT
Created attachment 660831 [details] [diff] [review]
added button to uploading bar
Comment 17 Mike Conley (:mconley) - (Needinfo me!) 2012-09-13 07:22:05 PDT
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] 2012-09-13 07:57:41 PDT
Created attachment 660842 [details] [diff] [review]
changed the compseMsgs.properties
Comment 19 Mike Conley (:mconley) - (Needinfo me!) 2012-09-13 08:18:25 PDT
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] 2012-09-13 08:28:06 PDT
Created attachment 660851 [details] [diff] [review]
Bug 742524 - Let users dismiss Filelink upload notification permanently.
Comment 21 Mike Conley (:mconley) - (Needinfo me!) 2012-09-13 08:30:26 PDT
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] 2012-09-13 08:47:51 PDT
Created attachment 660857 [details] [diff] [review]
Bug 742524 - Let users dismiss Filelink upload notification permanently
Comment 23 Mike Conley (:mconley) - (Needinfo me!) 2012-09-13 08:49:16 PDT
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] 2012-09-13 14:08:05 PDT
https://hg.mozilla.org/comm-central/rev/f6501add98ac

Note You need to log in before you can comment on or make changes to this bug.