Closed Bug 390508 Opened 13 years ago Closed 12 years ago

Unify calendar command sets

Categories

(Calendar :: Calendar Views, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: Fallen, Assigned: Fallen)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 4 obsolete files)

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.
Attached patch Unify Calendar Sets - v1 (obsolete) โ€” โ€” Splinter Review
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)
(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.
This patch does not supersede the patch in bug 386479, but it does fix the keyboard buttons. Setting dependancy. 
Blocks: 167255, 386479, 390147
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.
(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.
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...
Attached patch Unify Calendar Sets - v2 (obsolete) โ€” โ€” Splinter Review
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)
No longer blocks: 386479
Depends on: 386479
Blocks: 340025
Summary: Unify calendar sets → Unify calendar command sets
Attached patch Unify Calendar List - v3 (obsolete) โ€” โ€” Splinter Review
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)
Attached patch Unify Calendar List - v3 fixed (obsolete) โ€” โ€” Splinter Review
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 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+
Whiteboard: [checkin-needed after 0.7]
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+
Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed after 0.7]
Target Milestone: --- → 0.8
The last checkin busted the tinderboxen:
error: file 'mozilla/calendar/base/content/calendar-common-sets.xul' doesn't exist
Philipp, you forgot to check in the calendar-common-sets.xul file. I just committed it to fix the tinderbox bustage.
Simon, it seems the opening < of the file was munged with the checkin? Checking in patch to add it.
Philipp, you removed the persist="checked" for three checkboxes in messenger-overlay-sidebar.xul. Were they removed by accident?
I think so, yes. The persist attribute is now on the <command> element. Does the state not persist?
Blocks: 401730
(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.
Depends on: 404056
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.