Closed
Bug 432675
Opened 16 years ago
Closed 9 years ago
Revise layout of Alarms option pane in preference dialog
Categories
(Calendar :: Preferences, defect)
Calendar
Preferences
Tracking
(Not tracked)
RESOLVED
FIXED
3.9
People
(Reporter: mschroeder, Assigned: Paenglab)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
41.65 KB,
image/png
|
Details | |
24.32 KB,
patch
|
Paenglab
:
review+
|
Details | Diff | Splinter Review |
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•16 years ago
|
Severity: trivial → normal
Keywords: uiwanted
Summary: Adjustments to Alarms options pane → Revise layout of Alarms option pane in preference dialog
Comment 1•16 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.
Comment 2•16 years ago
|
||
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•16 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•15 years ago
|
Reporter | ||
Comment 5•15 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•15 years ago
|
||
Yup, that's what I reckon - and then setting Lightning/Sunbird defaults accordingly.
Comment 9•10 years ago
|
||
Hello, Since 2012 this issue has not been resolved ? Is it another way to install default reminders on lightning ? Thank you
Comment 10•10 years ago
|
||
(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".
Comment 11•10 years ago
|
||
Oops: Fallen (philipp@bugzilla.kewis.ch)
Assignee | ||
Comment 12•10 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)
Comment 13•10 years ago
|
||
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•10 years ago
|
||
Attachment #8527188 -
Flags: review?(philipp)
Assignee | ||
Comment 15•10 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•10 years ago
|
||
Philipp, I hope it's okay when I try Review Board for this patch.
Assignee | ||
Comment 17•10 years ago
|
||
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
Comment 18•10 years ago
|
||
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•10 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.
Comment 20•10 years ago
|
||
(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•10 years ago
|
Attachment #8527188 -
Flags: review?(neil)
Assignee | ||
Comment 21•10 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•10 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 23•10 years ago
|
||
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•10 years ago
|
Attachment #8527188 -
Flags: review+
Assignee | ||
Comment 24•10 years ago
|
||
/r/917 - Bug 432675 - Move the timezone prefs to General. r=philipp, neil Pull down this commit: hg pull review -r 4cc87e354031d76bcb088ed6d3553c91919207c3
Updated•10 years ago
|
Attachment #8527188 -
Flags: review?(philipp) → review+
Comment 25•10 years ago
|
||
https://reviewboard.mozilla.org/r/915/#review1005 Ship It!
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 26•9 years ago
|
||
https://hg.mozilla.org/comm-central/rev/6eb5c2543073
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.9
Assignee | ||
Comment 27•9 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•9 years ago
|
||
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 29•9 years ago
|
||
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-
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 30•9 years ago
|
||
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•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 31•9 years ago
|
||
Check-in without the fixIterator change. https://hg.mozilla.org/comm-central/rev/d9842ddc938b
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•