Last Comment Bug 856451 - Make the New Mail alert show time easy adjustable
: Make the New Mail alert show time easy adjustable
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Preferences (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Thunderbird 23.0
Assigned To: Richard Marti (:Paenglab)
:
Mentors:
Depends on:
Blocks: 856759 856454
  Show dependency treegraph
 
Reported: 2013-03-31 12:44 PDT by Richard Marti (:Paenglab)
Modified: 2013-04-13 05:12 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (3.61 KB, patch)
2013-03-31 12:49 PDT, Richard Marti (:Paenglab)
bwinton: ui‑review+
Details | Diff | Review
Screenshot of Preferences dialog (12.30 KB, image/png)
2013-03-31 12:50 PDT, Richard Marti (:Paenglab)
no flags Details
patch v2 (3.66 KB, patch)
2013-04-01 13:18 PDT, Richard Marti (:Paenglab)
richard.marti: ui‑review+
Details | Diff | Review
patch v3 (3.56 KB, patch)
2013-04-03 09:13 PDT, Richard Marti (:Paenglab)
mconley: review+
richard.marti: ui‑review+
Details | Diff | Review
patch for check-in (3.58 KB, patch)
2013-04-10 00:52 PDT, Richard Marti (:Paenglab)
richard.marti: review+
richard.marti: ui‑review+
Details | Diff | Review

Description Richard Marti (:Paenglab) 2013-03-31 12:44:21 PDT
To adjust the time where the New Mail notification isn't easy to do. In Options/Preferences in Advanced you have to go to general tab and then open the Config Editor. Here you have to search for alerts.totalOpenTime and then to know the value is in milliseconds.

In General 'Customize New Mail Alarm' where would be a good place. In it isn't much and a new pref doesn't make it overloaded.
Comment 1 Richard Marti (:Paenglab) 2013-03-31 12:49:18 PDT
Created attachment 731691 [details] [diff] [review]
patch
Comment 2 Richard Marti (:Paenglab) 2013-03-31 12:50:10 PDT
Created attachment 731692 [details]
Screenshot of Preferences dialog
Comment 3 Blake Winton (:bwinton) (:☕️) 2013-04-01 10:52:00 PDT
Comment on attachment 731691 [details] [diff] [review]
patch

Why do we want to let people easily adjust this?

I'm also not entirely sure, if we want to let people adjust it in the first place, why we would restrict it to whole seconds, and why we would restrict the range to between 1 and 99 seconds.  Surely someone out there requires the ability to show an alert for .25 seconds, or for an hour…  ;)

Based on these two questions, I'm going to say ui-r-, at least for now.
Comment 4 Richard Marti (:Paenglab) 2013-04-01 12:37:15 PDT
(In reply to Blake Winton (:bwinton) from comment #3)
> Comment on attachment 731691 [details] [diff] [review]
> patch
> 
> Why do we want to let people easily adjust this?

Bug 831737 changed the show time from 3 seconds to 10 because the fade-in and fade-out have each a duration of 2 seconds (equal to the alerts in Firefox) and with 3 seconds show time the notification wasn't shown with full opacity. I saw already people mentioning 10 seconds are to long (and other it's still to short). When this changes comes public with TB 24 I can think of a lot of complains about this change and a easy possibility to change could damp this complaints. And as I wrote already, this option wouldn't make the dialog overloaded.

> I'm also not entirely sure, if we want to let people adjust it in the first
> place, why we would restrict it to whole seconds, and why we would restrict
> the range to between 1 and 99 seconds.  Surely someone out there requires
> the ability to show an alert for .25 seconds, or for an hour…  ;)

I could add decimalplaces="1" then it would be adjustable by tenth of seconds. I restricted only the minimum to 1 second. And I tried to set one hour and I could set it but didn't tested it with an new Mail alert ;)

I'm okay when you say WONTFIX, but maybe it's worth a try.
Comment 5 Blake Winton (:bwinton) (:☕️) 2013-04-01 12:47:26 PDT
Comment on attachment 731691 [details] [diff] [review]
patch

Okay, that makes sense.  I'll say ui-r=me, if we can set it to 0 (to disable the alerts), and have a maximum time of something like an hour, and be able to go up in tenths of a second.

Thanks,
Blake.
Comment 6 Richard Marti (:Paenglab) 2013-04-01 13:18:05 PDT
Created attachment 732014 [details] [diff] [review]
patch v2

0 doesn't disable the alerts. It is still shown for the fade-in and fade-out time. The user can disable the notification in main preferences window with unticking the checkbox '[] Show an alert'. I'll let the minimum at 1. The maximum is now on 3600.

I'm carrying over the ui-r from previous patch.
Comment 7 Magnus Melin 2013-04-02 03:37:42 PDT
Tenth of a second? Really? Especially given we have fade-in/fade-outs of totally 5 secs in any case.
Comment 8 rsx11m 2013-04-02 09:09:39 PDT
+1; I've tried it and I find it very hard to actually change the decimal digit. The dials go in full seconds, and you need to highlight the decimal digit (and only it) to modify the fractions with the keyboard. That's a lot of effort to go with a quarter second more or less...
Comment 9 rsx11m 2013-04-02 09:52:39 PDT
(In reply to Magnus Melin from comment #7)
> fade-in/fade-outs

The user could disable those with alerts.disableSlidingEffect in the Config Editor, thus a minimum of 1sec still makes sense from that perspective (though you won't see much which such a short duration, but it's hard to define and impose a "useful" minimum).
Comment 10 rsx11m 2013-04-03 08:07:13 PDT
Comment on attachment 732014 [details] [diff] [review]
patch v2

>+      <textbox id="totalOpenTime" type="number" decimalplaces="1" size="3" min="1" max="3600"

FYI: The SeaMonkey patch in bug 856454 has received ui-r+ without the decimalplaces, thus full seconds only.

>+               onsyncfrompreference="let tot = document.getElementById(this.getAttribute('preference'));
>+                                     return (tot.instantApply ? tot.valueFromPreferences : tot.value) / 1000;"

That's more complicated than apparently necessary. Per Neil's bug 856454 comment #10, you don't seem to need the check for instantApply using valueFromPreferences instead of just value. I've used the following simplification and it works fine on both Windows (applied on "Ok") and Linux (instantly) in my patch:

>+                   onsyncfrompreference="return document.getElementById(this.getAttribute('preference')).value / 1000;"
Comment 11 Richard Marti (:Paenglab) 2013-04-03 09:13:01 PDT
Created attachment 732874 [details] [diff] [review]
patch v3

(In reply to rsx11m from comment #10)
> Comment on attachment 732014 [details] [diff] [review]
> patch v2
> 
> >+      <textbox id="totalOpenTime" type="number" decimalplaces="1" size="3" min="1" max="3600"
> 
> FYI: The SeaMonkey patch in bug 856454 has received ui-r+ without the
> decimalplaces, thus full seconds only.

I'll let Mike decide if we want to show the decimalplaces. I could also add increment="0.5" then it would always increment/decrement a half second and the tenth field isn't needed to select.

> >+                   onsyncfrompreference="return document.getElementById(this.getAttribute('preference')).value / 1000;"

Thank you for this tip. I tested it and it works. This patch has this now.
Comment 12 Mike Conley (:mconley) - (Needinfo me!) 2013-04-09 23:19:24 PDT
Comment on attachment 732874 [details] [diff] [review]
patch v3

Let's go with .5 seconds, as Richard suggested.

With that increment, r=me.
Comment 13 Richard Marti (:Paenglab) 2013-04-10 00:52:42 PDT
Created attachment 735627 [details] [diff] [review]
patch for check-in

Added the increment.

Carrying over r+ and ui-r+
Comment 14 Ryan VanderMeulen [:RyanVM] 2013-04-13 05:12:21 PDT
https://hg.mozilla.org/comm-central/rev/505edc73c23a

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