432675 - Revise layout of Alarms option pane in preference dialog

Status: RESOLVED FIXED

(Calendar :: Preferences, defect)

Description

[Martin Schröder [:mschroeder]]

Follow-up from bug 397896:

The default snooze preference should be move from the "General" options pane to the "Alarms" pane. But on Windows the Thunderbird preference dialog is fixed size, and currently the "Alarms" and "Views" pane are already partly off screen (bug 385249). There is no space to add another line to these tabs.

A drop-down list solution with on/off like for the default alarm setting doesn't
make sense to me. Also the secondary settings for the alarms before events and
tasks should depend on the enabled/disabled state of the first preference.
ssitter suggested following solution:
"I would prefer something space saving like
  [ ] default alarm for new events [___] minutes
  [X] default alarm for new tasks  [_15] minutes

Problem is that current preference is stored as integer and can't be mapped
easily to a checkbox. Either we need to manually map it or use a new
preference. The former results in rather ugly code, the latter in users
wondering why their settings were reset."

Comment 1

[Sven Giermann]

(In reply to comment #0)
> Problem is that current preference is stored as integer and can't be mapped
> easily to a checkbox. Either we need to manually map it or use a new
> preference. The former results in rather ugly code, the latter in users
> wondering why their settings were reset."

I would advise the second option. There is no need to produce "ugly code", if there's only little affect for the user, as long as we are at version 0.x!
Additionally one could write some kind of upgrade helper, that reads and deletes the old pref to translate it into the new bool pref.

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

This will get even more interesting when bug 353492 is fixed. Anyway, if the layout stays like above (i.e no before/after the event), then we can use a value of -1 for no default alarm. That isn't too ugly, since you can do |checkbox.checked = (intPrefValue != -1);|

Comment 3

[Sven Giermann]

(In reply to comment #2)
> This will get even more interesting when bug 353492 is fixed.

Thanks for this hint - setting dependency, since you are nearly done with your fix, so this one should not change anything before your changes have been checked in.

Comment 5

[Martin Schröder [:mschroeder]]

(In reply to comment #4)
> *** Bug 514336 has been marked as a duplicate of this bug. ***

That bug requested showing the default reminder settings from Google via the GData provider.

Comment 6

[Morgan Read]

Yup, that's what I reckon - and then setting Lightning/Sunbird defaults accordingly.

Comment 9

[Alain Térieur]

Hello,
Since 2012 this issue has not been resolved ?
Is it another way to install default reminders on lightning ?
Thank you

Comment 10

[Tony Mechelynck [:tonymec]]

(In reply to Alain Térieur from comment #9)
> Hello,
> Since 2012 this issue has not been resolved ?
> Is it another way to install default reminders on lightning ?
> Thank you

Hello Alain.

The Calendar team is short on manpower (only one person, doing it in his spare time after coming back from his paying job IIUC). He fixes the severest problems first, and enhancements like this one are left for when there's nothing else to do (there always is), or for when someone comes up with a patch (that does happen, but not as often as we might wish for).

Do you know how to fix this problem? Then feel free to attach a patch and ASSIGN the bug to yourself: I'm sure Fallen ([e-mail]) will be happy to review it. Once you've got a positive review, set the "checkin-needed" keyword and someone will push your patch to the permanent source repository.

See also https://bugzilla.mozilla.org/page.cgi?id=etiquette.html section 1, "Commenting".

Comment 11

[Tony Mechelynck [:tonymec]]

Oops: Fallen ([e-mail])

Comment 12

[Richard Marti (:Paenglab)]

I would say the initial issue is already resolved. The default snooze preference is now in "Reminders" tab together with all the other reminder options.

The only thing I'm seeing which should be changed is the "Timezone" tab with it's only menulist.

Philipp, are you planning a more sophisticated timezone chooser or could it make sense to move the menulist to "General" tab? Or what do you think should be changed in the prefs?

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I think we talked about this in Toronto, right? I'm fine with moving the dropdown to the General tab, I think we can do better on the timezone picker, but right now its not one of my priorities.

Comment 14

[Richard Marti (:Paenglab)]

Attachment: MozReview Request: bz://432675/Paenglab

Comment 15

[Richard Marti (:Paenglab)]

/r/917 - Bug 432675 - Move the timezone prefs to General. r=philipp

Pull down this commit:

hg pull review -r 66566d5006eed52eba4de604ca817c5b18d352c7

Comment 16

[Richard Marti (:Paenglab)]

Philipp, I hope it's okay when I try Review Board for this patch.

Comment 17

[Richard Marti (:Paenglab)]

Attachment: NewPrefs.png

Screenshot of the new General Tab layout.

I moved the system colors pref down to group the date/time prefs together.

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

https://reviewboard.mozilla.org/r/915/#review501

::: calendar/base/content/preferences/general.js
(Diff revision 1)
> +        while (enumerator.hasMore()) {

Can we use fixIterator here?

::: calendar/base/content/preferences/general.js
(Diff revision 1)
> +        for (let i = 0; i < displayNames.length; ++i) {

Lets switch to for..of here.

Comment 19

[Richard Marti (:Paenglab)]

(In reply to Philipp Kewisch [:Fallen] from comment #18)
> https://reviewboard.mozilla.org/r/915/#review501
> 
> ::: calendar/base/content/preferences/general.js
> (Diff revision 1)
> > +        while (enumerator.hasMore()) {
> 
> Can we use fixIterator here?

I'm sorry, I don't know how to do this. Please, could you show me how?

> ::: calendar/base/content/preferences/general.js
> (Diff revision 1)
> > +        for (let i = 0; i < displayNames.length; ++i) {
> 
> Lets switch to for..of here.

Done.

Comment 20

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(btw I think you have to answer in reviewboard for the comments to show up there)


(In reply to Richard Marti (:Paenglab) from comment #19)
> (In reply to Philipp Kewisch [:Fallen] from comment #18)
> > https://reviewboard.mozilla.org/r/915/#review501
> > 
> > ::: calendar/base/content/preferences/general.js
> > (Diff revision 1)
> > > +        while (enumerator.hasMore()) {
> > 
> > Can we use fixIterator here?
> 
> I'm sorry, I don't know how to do this. Please, could you show me how?

This should do it, untested. You might need to Cu.import the jsm if its not already there, its in iteratorUtils.

for (let tzid in fixIterator(tzService.enumerator)) {
  let tz = tzService.getTimezone(tzid);


}

Comment 21

[Richard Marti (:Paenglab)]

/r/917 - Bug 432675 - Move the timezone prefs to General. r=philipp

Pull down this commit:

hg pull review -r f297b523d9afff1a5bb9d208ea96c7a0ca62d220

Comment 22

[Richard Marti (:Paenglab)]

This patch is addressing the comments from last review.

Neil, please can you review the part for mailnews/base/util/iteratorUtils.jsm?

Comment 23

[[e-mail]]

Comment on attachment 8527188 [details]
MozReview Request: bz://432675/Paenglab

>+     while (aEnum.hasMore()) yield aEnum.getNext();
yield on its own line please.

Comment 24

[Richard Marti (:Paenglab)]

/r/917 - Bug 432675 - Move the timezone prefs to General. r=philipp, neil

Pull down this commit:

hg pull review -r 4cc87e354031d76bcb088ed6d3553c91919207c3

Comment 25

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

https://reviewboard.mozilla.org/r/915/#review1005

Ship It!

Comment 26

[Richard Marti (:Paenglab)]

https://hg.mozilla.org/comm-central/rev/6eb5c2543073

Comment 27

[Richard Marti (:Paenglab)]

backed out because of mozmill failure in test-cloudfile-add-account-dialog.js::setupModule

https://hg.mozilla.org/comm-central/rev/9e3301977f89

Comment 28

[Richard Marti (:Paenglab)]

Attachment: moveTZ.patch

Moved the code to check for nsIStringEnumerator after the code to check for nsISimpleEnumerator.

Mozmill is green on Try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=648af014d4e2

Comment 29

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8541879 [details] [diff] [review]
moveTZ.patch

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

::: mailnews/base/util/iteratorUtils.jsm
@@ +118,5 @@
>      return { __iterator__: iter };
>    }
>  
> +  // How about nsIStringEnumerator or nsIUTF8StringEnumerator?
> +  if (aEnum instanceof Ci.nsIStringEnumerator || aEnum instanceof Ci.nsIUTF8StringEnumerator) {

I'd suggest adding a comment why it needs to happen after the former block. I've dug in one step deeper and have come to this conclusion:

// This needs to happen after nsISimpleEnumerator because the default C++ enumerator implementation offers both for empty enumerators.

While it may work in the Lightning case because our string enumerator is js and only implements nsIUTF8StringEnumerator, I'm not yet sure its the right approach: what happens if its the default implementation [1] is used for something that is actually a string enumerator? Will the code think its a nsISimpleEnumerator and just return the nsISupports instances instead of strings?

I haven't had time to find a such case to test with, I think we should do that before pushing this code. Maybe its easier to just revert the iteratorUtils changes and iterate manually, then open a new bug to add the code to iteratorUtils.

r- if you'd like to do everything in this bug, r+ with the old iteration code if you prefer.


[1] http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/glue/nsEnumeratorUtils.cpp#16

Comment 30

[Richard Marti (:Paenglab)]

Attachment: moveTZ.patch

I decided to use the old iteration code in this bug and the fixIterator change to do in bug 1115954.

Comment 31

[Richard Marti (:Paenglab)]

Check-in without the fixIterator change.

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