856451 - Make the New Mail alert show time easy adjustable

Status: RESOLVED FIXED

(Thunderbird :: Preferences, defect)

Description

[Richard Marti (:Paenglab)]

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)]

Attachment: patch

Comment 2

[Richard Marti (:Paenglab)]

Attachment: Screenshot of Preferences dialog

Comment 3

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

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)]

(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) (:☕️)]

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)]

Attachment: 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 [:mkmelin]]

Tenth of a second? Really? Especially given we have fade-in/fade-outs of totally 5 secs in any case.

Comment 8

[rsx11m]

+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]

(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]

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)]

Attachment: 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) (:⚙️)]

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)]

Attachment: patch for check-in

Added the increment.

Carrying over r+ and ui-r+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/505edc73c23a