Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Revise layout of Alarms option pane in preference dialog

RESOLVED FIXED in 3.9

Status

Calendar
Preferences
RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: martinschroeder, Assigned: Paenglab)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
Dependency tree / graph

Details

Attachments

(2 attachments, 2 obsolete attachments)

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."

Updated

9 years ago
Severity: trivial → normal
Keywords: uiwanted
Summary: Adjustments to Alarms options pane → Revise layout of Alarms option pane in preference dialog

Comment 1

9 years ago
(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.
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

9 years ago
(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.
Depends on: 353492
(Reporter)

Updated

8 years ago
Blocks: 416482, 369962
Duplicate of this bug: 514336
(Reporter)

Comment 5

8 years ago
(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

8 years ago
Yup, that's what I reckon - and then setting Lightning/Sunbird defaults accordingly.
Duplicate of this bug: 546295
Duplicate of this bug: 724141

Comment 9

3 years ago
Hello,
Since 2012 this issue has not been resolved ?
Is it another way to install default reminders on lightning ?
Thank you
(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 (philipp@kewis.ch) 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".
Oops: Fallen (philipp@bugzilla.kewis.ch)
(Assignee)

Comment 12

3 years ago
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?
Flags: needinfo?(philipp)
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.
Flags: needinfo?(philipp)
(Assignee)

Comment 14

3 years ago
Created attachment 8527188 [details]
MozReview Request: bz://432675/Paenglab
Attachment #8527188 - Flags: review?(philipp)
(Assignee)

Comment 15

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

Pull down this commit:

hg pull review -r 66566d5006eed52eba4de604ca817c5b18d352c7
(Assignee)

Comment 16

3 years ago
Philipp, I hope it's okay when I try Review Board for this patch.
(Assignee)

Comment 17

3 years ago
Created attachment 8527189 [details]
NewPrefs.png

Screenshot of the new General Tab layout.

I moved the system colors pref down to group the date/time prefs together.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
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.
(Assignee)

Comment 19

3 years ago
(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.
(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);


}
(Assignee)

Updated

3 years ago
Attachment #8527188 - Flags: review?(neil)
(Assignee)

Comment 21

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

Pull down this commit:

hg pull review -r f297b523d9afff1a5bb9d208ea96c7a0ca62d220
(Assignee)

Comment 22

3 years ago
This patch is addressing the comments from last review.

Neil, please can you review the part for mailnews/base/util/iteratorUtils.jsm?
Comment on attachment 8527188 [details]
MozReview Request: bz://432675/Paenglab

>+     while (aEnum.hasMore()) yield aEnum.getNext();
yield on its own line please.
Attachment #8527188 - Flags: review?(neil) → review+
(Assignee)

Updated

3 years ago
Attachment #8527188 - Flags: review+
(Assignee)

Comment 24

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

Pull down this commit:

hg pull review -r 4cc87e354031d76bcb088ed6d3553c91919207c3
Attachment #8527188 - Flags: review?(philipp) → review+
https://reviewboard.mozilla.org/r/915/#review1005

Ship It!
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 26

3 years ago
https://hg.mozilla.org/comm-central/rev/6eb5c2543073
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.9
(Assignee)

Comment 27

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

https://hg.mozilla.org/comm-central/rev/9e3301977f89
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 28

3 years ago
Created attachment 8541879 [details] [diff] [review]
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
Attachment #8527188 - Attachment is obsolete: true
Attachment #8541879 - Flags: review?(philipp)
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
Attachment #8541879 - Flags: review?(philipp) → review-
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Depends on: 1115954
(Assignee)

Comment 30

3 years ago
Created attachment 8541904 [details] [diff] [review]
moveTZ.patch

I decided to use the old iteration code in this bug and the fixIterator change to do in bug 1115954.
Attachment #8541879 - Attachment is obsolete: true
Attachment #8541904 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 31

3 years ago
Check-in without the fixIterator change.

https://hg.mozilla.org/comm-central/rev/d9842ddc938b
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Reporter)

Updated

3 years ago
Keywords: uiwanted
You need to log in before you can comment on or make changes to this bug.