Closed
Bug 390508
Opened 17 years ago
Closed 17 years ago
Unify calendar command sets
Categories
(Calendar :: Calendar Frontend, defect)
Calendar
Calendar Frontend
Tracking
(Not tracked)
VERIFIED
FIXED
0.8
People
(Reporter: Fallen, Assigned: Fallen)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file, 4 obsolete files)
121.17 KB,
patch
|
Fallen
:
review+
Fallen
:
ui-review+
|
Details | Diff | Splinter Review |
During bug 388405 and also another one, it seemed like a good idea to unify the calendar sets between ltn and sb. Lightning has its sets in messenger-overlay-sidebar.js Sunbird has its sets in sunbird/base/content/calendar-sets.inc It would be good to have a base/content/calendar-sets.inc to include the sets in both apps.
Assignee | ||
Comment 1•17 years ago
|
||
I have come up with a patch. Since its fairly large, I would love to have it checked in soon so it doesn't bitrot. I can understand its harder to review though and rc1 is coming up. This patch consolidates a bunch of stuff I came across w.r.t the commands. A couple things to consider: Publish/Export: Should the toolbar buttons show a dialog with the calendar, or export selected events? This issue is also handled in the patch to bug 390147 (which this patch supersedes). I chose to let the user select a calendar to export when the button is clicked. This means there will be no way to publish a selection. I added the "Export selection" item from sunbird also to the lightning menus. I think this can be taken care of by creating an export wizard. In this wizard you could select either a range, a set of calendars, or the selection to be exported. I might add proposal xuls to bug 167255 soon. Christian, feel free to come up with something too. This patch also fixes the use of Ctrl+3 in Lightning. It currently did not switch to calendar mode correctly. Also, the possibility to use Ctrl+1 to switch back to mail mode was added. A few strings are not used anymore (calendar.wizard.label, calendar.wizard.accesskey). I will not remove them here since we are in string freeze and they don't really hurt. Post 0.7 should clean up those strings. I removed the calendar list context menu from calendar.xul. It seems I forgot to remove it for the calendar list unification bug. Please make sure I didn't miss anything for the sunbird hidden window (i.e test this on a mac). Another possible step would be to unify the toolbar buttons, since most are duplicated in calendar.xul/messenger-overlay-toolbar.xul. Since tb uses a mail-specific Id, I left this out for now. If this doesn't make 0.7, I think at least the part about using Ctrl+1/Ctrl+3 for mode switching should work.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #280293 -
Flags: ui-review?(christian.jansen)
Attachment #280293 -
Flags: review?(michael.buettner)
Comment 2•17 years ago
|
||
(In reply to comment #1) > This patch also fixes the use of Ctrl+3 in Lightning. It currently did not > switch to calendar mode correctly. Also, the possibility to use Ctrl+1 to > switch back to mail mode was added. This belongs to Bug 386479. Or at least add the dependency if your patch fixes this issue entirely.
Assignee | ||
Comment 3•17 years ago
|
||
This patch does not supersede the patch in bug 386479, but it does fix the keyboard buttons. Setting dependancy.
Comment 4•17 years ago
|
||
Hi Phillip, The patch works fine, but I've experienced som bugs (Tested on Intel Mac), where I don't know if they were caused by the patch. Switching from Cal to Mail: Warning: Error in parsing value for property 'max-height'. Declaration dropped. Source File: chrome://messenger/content/messenger.xul Line: 0 From UI perspective this feature is now working well r+ for that. Btw. We should ask the Thunderbird folks if they could move the "Address Book" shortcut from CTRL+2 to CTRL +4. This would make switching far more usable ;-) Export **************** I've experience the following problems: Create 2 calendars Select #2 Select "Export" from the Calendar Menu: -> Calendar #1 is selected. This should be changed (calendar #2 should be selected) -> Ok button does *not* work, if the calendar is empty. This should also be changed to a more failure tolerant behavior. -> "Save as" Dialog -> Should be renamed to Export -> The File Type List contains HTML, ICS & CSV ICS should be removed, because this is by definition no export, it is a SAVE AS function. Export should list only file formats we can't read, such as HTML or CSV Overall it makes sense to redesign and redefine the whole import export process. Bug 167255 seems to be a good starting point.
Comment 5•17 years ago
|
||
(In reply to comment #4) > -> The File Type List contains HTML, ICS & CSV > ICS should be removed, because this is by definition no export, it > is a SAVE AS function. Export should list only file formats we > can't read, such as HTML or CSV I think exporting a storage calendar or a google calendar to iCalendar format is a valid use case. In addition I think that we should not split the export feature in two different commands/locations.
Comment 6•17 years ago
|
||
Even if I get to reviewing this patch this week (which I doubt in light of the sheer number of patches on the 0.7 blocker list) I wouldn't want to take this for the 0.7 release. The reason is simply that it carries a high risk of introducing regressions but doesn't buy us a ton of new features. Besides that, thanks for this excellent patch, I'm taking my hat off...
Assignee | ||
Comment 7•17 years ago
|
||
This version of the patch is the post-0.7 version, with comments addressed and strings removed. (In reply to comment #4) > Warning: Error in parsing value for property 'max-height'. Declaration dropped. > Source File: chrome://messenger/content/messenger.xul > Line: 0 This is a different bug, it was there before I believe. > Btw. We should ask the Thunderbird folks if they could move the "Address Book" > shortcut from CTRL+2 to CTRL +4. This would make switching far more usable ;-) This doesn't really make sense if you are using just thunderbird, but I see what you mean > -> Calendar #1 is selected. > This should be changed (calendar #2 should be selected) Fixed for all places that use chooseCalendarDialog. > -> Ok button does *not* work, if the calendar is empty. This should also be > changed to a more failure tolerant behavior. So what should happen in this case? The changes I made will export an empty file, only containing the usual headers (i.e BEGIN:VCALENDAR, versions, END:VCALENDAR) > -> "Save as" Dialog -> Should be renamed to Export Fixed > ICS should be removed, because this is by definition no export, it > is a SAVE AS function. Export should list only file formats we > can't read, such as HTML or CSV I agree to ssitter, I think ICS should be a valid export option. I'll prepare a second patch to take care of just fixing the keys and attach it to the other bug ssitter mentioned.
Attachment #280293 -
Attachment is obsolete: true
Attachment #280356 -
Flags: ui-review+
Attachment #280356 -
Flags: review?(michael.buettner)
Attachment #280293 -
Flags: ui-review?(christian.jansen)
Attachment #280293 -
Flags: review?(michael.buettner)
Assignee | ||
Updated•17 years ago
|
Updated•17 years ago
|
Summary: Unify calendar sets → Unify calendar command sets
Assignee | ||
Comment 8•17 years ago
|
||
Unbitrotten patch. I had to do a sh*tload of merging, so please be extra careful when testing.
Attachment #280356 -
Attachment is obsolete: true
Attachment #282749 -
Flags: ui-review+
Attachment #282749 -
Flags: review?(michael.buettner)
Attachment #280356 -
Flags: review?(michael.buettner)
Assignee | ||
Comment 9•17 years ago
|
||
Missed some occurrences of the today() function.
Attachment #282749 -
Attachment is obsolete: true
Attachment #282753 -
Flags: ui-review+
Attachment #282753 -
Flags: review?(michael.buettner)
Attachment #282749 -
Flags: review?(michael.buettner)
Comment 10•17 years ago
|
||
Comment on attachment 282753 [details] [diff] [review] Unify Calendar List - v3 fixed The patch contains some hunks that seem to be unrelated to the bug, I assume they got into the patch on accident and won't be checked in. These hunks affect the following files: /base/content/chooseCalendarDialog.xul /base/content/import-export.js /locales/en-US/chrome/calendar/calendar.properties deleteSelectedEvents() should probably be part of calendar-views.js instead of calendar-item-editing.js. >+ var view = document.getElementById(type + "-view"); This line is not necessary, the variable isn't referenced from anywhere. Apart from that I didn't find anything to complain about. r=mickey.
Attachment #282753 -
Flags: review?(michael.buettner) → review+
Updated•17 years ago
|
Whiteboard: [checkin-needed after 0.7]
Assignee | ||
Comment 11•17 years ago
|
||
Updated version. Leaving the extra hunks in, as discussed on IRC.
Attachment #282753 -
Attachment is obsolete: true
Attachment #285987 -
Flags: ui-review+
Attachment #285987 -
Flags: review+
Assignee | ||
Comment 12•17 years ago
|
||
Checked in on HEAD and MOZILLA_1_8_BRANCH -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed after 0.7]
Target Milestone: --- → 0.8
Comment 13•17 years ago
|
||
The last checkin busted the tinderboxen: error: file 'mozilla/calendar/base/content/calendar-common-sets.xul' doesn't exist
Comment 14•17 years ago
|
||
Philipp, you forgot to check in the calendar-common-sets.xul file. I just committed it to fix the tinderbox bustage.
Assignee | ||
Comment 15•17 years ago
|
||
Simon, it seems the opening < of the file was munged with the checkin? Checking in patch to add it.
Comment 16•17 years ago
|
||
Philipp, you removed the persist="checked" for three checkboxes in messenger-overlay-sidebar.xul. Were they removed by accident?
Assignee | ||
Comment 17•17 years ago
|
||
I think so, yes. The persist attribute is now on the <command> element. Does the state not persist?
Comment 18•17 years ago
|
||
(In reply to comment #17) > The persist attribute is now on the <command> element. Does > the state not persist? It persists. Sorry for the confusion. I overlooked they were moved to calendar-common-sets.xul.
Updated•16 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•