456385 - Thunderbird3: Integrate Calendar and Task mode menu items into new menu

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect, P1)

Description

[Christian Jansen]

The following items need to be integrated, or removed for an integration into the new menu structure [1]

* New Calendar
* Import Calendar
* Export Calendar
* Show Work Days Only
* Show Tasks in Calendar
* Show Completed Tasks
* Rotate View -> we don't want to have this in tb3, right? ;-)
* View Day
* View Week
* View Multi Week
* View Month
* View Minimonth
* View Calendars
* View Invitations
* Go Today
* Reload Calendar
* Publish
* Delete Calendar
* Find Calendar
* Filter Tasks
* Mark Completed
* Progress
* Calendar
* Convert to

[1] https://wiki.mozilla.org/Thunderbird:Menus

Comment 1

[Philipp Kewisch [:Fallen][📱📆]]

Does link [1] above contain the final state of how the menus should look? Is tere any menu mode switching planned? Please also decide for elements above that are not supposed to be in the menus, where they should alternativly be.

Comment 2

[Philipp Kewisch [:Fallen][📱📆]]

I'd like to see some traction on this bug...

Comment 3

[Berend Cornelius]

As suggested in Bug 467909 comment 4 and Bug 467909 comment 6 it should be evaluated if Sunbird´s menuitem Go/Date that raises the date dialog should not be integrated to a menu of Lightning as a substitution for the removed datepicker beneath the minimonth in the calendar view.

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

Bryan, any update on this?

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

Christian put together this proposal that I think looks pretty good.  There is some integration into the view menu, otherwise everything sits in the Events and Tasks menu.

View

    Calendar
        Day View
        Week View
        Multi Week View
        Month View
        ------------
        Hide Mini Month
        Hide Calendar List
        ------------
        Show
            Work Days Only
            Tasks in Calendar
            Completed Tasks
            Rotate View Clock-Wise
        Reload
    Tasks
        Hide Mini Month
        Hide Calendar List
        Hide Task Filter



Events and Tasks

    Schedule Event
    Create Task
    ----------
    Find Calendar
    New Calendar
    ----------
    Go to Today
    ----------
    Calendar
        Open from File ...
        ------
        Import
        Export
        --------
        Delete
       
    Publish Calendar
    Tasks
        Mark completed
        Progress
        Priority

    ----------
    Convert to
        Message
        Event
        Task
    Show Invitations

Comment 6

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #5)
> Christian put together this proposal that I think looks pretty good.  There is
> some integration into the view menu, otherwise everything sits in the Events
> and Tasks menu.
> 
> View
>     Calendar
>     Tasks
Where in the view menu should these items go?


>         Hide Mini Month
>         Hide Calendar List
>         Hide Task Filter
I'd prefer to formulate these in a positive manner (i.e Show Minimonth)


> Events and Tasks
>     Schedule Event
Why not "Create Event" ?

>     Create Task
>     ----------
>     Find Calendar
>     New Calendar
>     ----------
>     Go to Today
>     ----------
>     Calendar
>         Open from File ...
>         ------
>         Import
>         Export
>         --------
>         Delete
Why don't we integrate Open/New into the File Menu? The "Calendar" submenu looks quite misplaced to me.

Comment 7

[Simon Paquet [:sipaq]]

Philipp will take over as this turned out too hard for me

Comment 8

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: WiP Patch - v1

I don't really agree to the proposal above. Sorry for bashing it, but I think the menu elements fit in much better the way they are in this patch.

Whats missing in this patch is to fine-tune all the strings to make sure it fits well and to make sure the menuitems actually do what they are supposed to and are disabled when they are supposed to.

Something to note about the menu-restructure: Its not quite clear to me how stateful menuitems should behave that are valid for more than one mode:

* making them mode dependant might confuse the user
* making them mode independant destroys the possibility to have different options
  per mode.

Comment 9

[Martin Schröder [:mschroeder]]

Some comments regarding the menu structure proposal:

(In reply to comment #5)
> View
> 
>     Calendar
[...]
>         Hide Mini Month
>         Hide Calendar List

I don't we need neither "Hide" nor "Show" as a prefix if we have a menuitem of type checkbox.

[...]
>         Reload

I don't think this belongs in the View menu because it's an action and not just a setting like the other menuitems.

>     Tasks
>         Hide Mini Month
>         Hide Calendar List
>         Hide Task Filter

I don't we need neither "Hide" nor "Show" as a prefix if we have a menuitem of type checkbox.


> Events and Tasks
> 
>     Schedule Event

I agree with Philipp. "Create Event" sounds more common to me although "Schedule" might be the "better" verb.

[...]

>     Calendar
>         Open from File ...
>         ------
>         Import
>         Export
>         --------
>         Delete
> 
>     Publish Calendar

Where might the user look for these action first? "File" menu imho!

[...]


Should there be any menuitems for the "Today Pane" in the "View" menu?

Regarding the stateful menuitems, I see just following critical ones, where you must have selected a specific task/event/email to proceed:

    Tasks
        Mark completed
        Progress
        Priority

    ----------
    Convert to
        Message
        Event
        Task

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

This is my current menu structure as in the patch. I agree its not ideal (i.e the "Show..." issue Martin mentioned), but I think it better integrates the items into the existing menus.

In the below proposal there are probably more critical menuitems we need to think about.

=============================


File
  New
    Message
    Event
    Task
    ---
    Folder
    Saved Search
    ---
    Account
    Calendar
    ---
    AB Contact
  Open
    Saved Message
    Calendar File
  ...


Edit
  ...
  Folder Properties
  Account Settings
  Calendar Settings
  Preferences

View
  ...
  Folders
  ---
  Today Pane
    Show Today Pane
    ---
    Show Minimonth
    Show Mini-day
    Shoe none
  Calendar
    Day
    Week
    Multiweek
    Month
    ---
    Minimonth
    Calendar List
    ---
    Current View
      Workweek days only
      Show Tasks in view
      Show Completed Tasks
      Rotate View
  Tasks
    Filter Tasks
    ---
    All
    Today
    Next 7 Days
    Not Started
    ...(our other filters)...
  ---
  Sort by
  ...

Go
  ...
  Forward
  Back
  Today
  ---
  Mail Start Page


Events and Tasks
  New Event
  New Task
  ---
  Calendar
  Tasks
  ---
  Export
  Import
  Publish
  Delete Selected Calendar
  ---
  Mark Completed
  Priority
    Not specified
    Low
    Medium
    High
  ---
  Find Events

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

Large patches always tend to give you the feeling you need lots of time to fix it right. I've finally found some time to sit down and work on this. Since I haven't gotten any answer on the previous comment, I'm going to finish this patch and send it through review.

Comment 12

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v2

This is the patch, in its full beauty. Assigning primary review to dbo, but I'd appreciate if others could take a look also.

Comment 13

[Simon Paquet [:sipaq]]

Looks good on first glance.

Comment 14

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 379483 [details] [diff] [review]
Fix - v2

We have a new volunteer :-)

Comment 15

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 379483 [details] [diff] [review]
Fix - v2

I think the File -> New -> Event, Task, Calendar is unnecessary.  It's both a duplicate and I don't think people would go there when there is already an Events & Tasks top level menu.  

Otherwise it looks good to me.

I like the Edit -> Cal Settings, that makes a lot more sense.

Comment 16

[Martin Schröder [:mschroeder]]

Comment on attachment 379483 [details] [diff] [review]
Fix - v2

This is a a review with two parts: code and functional review. I will start with the results of my code review. In general, I seems the ids for the menuitems are not consistent. Maybe you can have another look, but it is not that important.

>diff --git a/calendar/base/content/calendar-task-tree.js b/calendar/base/content/calendar-task-tree.js
>--- a/calendar/base/content/calendar-task-tree.js
>+++ b/calendar/base/content/calendar-task-tree.js
>@@ -286,16 +283,14 @@
> }
> 
> /**
>- * Toggle the completed state on tasks retrieved from the given event.
>+ * Toggle the completed state on selected tasks.
>  *
>- * XXXberend Please clarify if this is correct and describe more clearly.
>- *
>- * @param aEvent    The command event that holds information about the tasks.
>+ * @param aEvent    The originating event, can be null.
>  */
> function toggleCompleted(aEvent) {
>     if (aEvent.target.getAttribute("checked") == "true") {
>+        contextChangeTaskProgress(aEvent, 0);
>+    } else {
>         contextChangeTaskProgress(aEvent, 100);
>-    } else {
>-        contextChangeTaskProgress(aEvent, 0);
>     }
> }

Check if aEvent is null?


>diff --git a/calendar/lightning/content/lightning-scripts.inc b/calendar/lightning/content/lightning-scripts.inc
>--- a/calendar/lightning/content/lightning-scripts.inc
>+++ b/calendar/lightning/content/lightning-scripts.inc
>@@ -46,11 +46,3 @@
> <script type="application/javascript" src="chrome://lightning/content/messenger-overlay-sidebar.js"/>
> <script type="application/javascript" src="chrome://lightning/content/messenger-overlay-toolbar.js"/>
> <script type="application/javascript" src="chrome://calendar/content/calendar-invitations-manager.js"/>
>-<script type="application/javascript">
>-    var calendarmenulabel = "&lightning.calendar.label;";
>-    var calendarmenuaccesskey = "&lightning.calendar.accesskey;";
>-    var messagemenulabel = "&msgMenu.label;";
>-    var messagemenuaccesskey = "&msgMenu.accesskey;";
>-    var tasksmenulabel = "&lightning.tasks.label;";
>-    var tasksmenuaccesskey = "&lightning.tasks.accesskey;";
>-</script>

Please, also remove the line including messenger-overlay-toolbar.js as you remove it with your patch.


>diff --git a/calendar/lightning/content/messenger-overlay-sidebar.js b/calendar/lightning/content/messenger-overlay-sidebar.js
>--- a/calendar/lightning/content/messenger-overlay-sidebar.js
>+++ b/calendar/lightning/content/messenger-overlay-sidebar.js
>@@ -133,6 +133,7 @@
>     var filter = document.getElementById("task-tree-filtergroup");
>     filter.value = filter.value || "all";
>     document.getElementById("modeBroadcaster").setAttribute("mode", gCurrentMode);
>+    document.getElementById("modeBroadcaster").setAttribute("checked", "true");
> 
>     let mailContextPopup = document.getElementById("mailContext");
>     if (mailContextPopup)

What is the 'checked' attribute for?


>+function ltnSwitch2Mail() {
>+  if (gCurrentMode != 'mail') {
>+    var switch2mail = document.getElementById("switch2mail");
>+    var switch2calendar = document.getElementById("switch2calendar");
>+    var switch2task = document.getElementById("switch2task");
>+    switch2mail.setAttribute("checked", "true");
>+    switch2calendar.removeAttribute("checked");
>+    switch2task.removeAttribute("checked");
>+
>+    gCurrentMode = 'mail';
>+    document.getElementById("modeBroadcaster").setAttribute("mode", gCurrentMode);
>+
>+    document.commandDispatcher.updateCommands('calendar_commands');
>+
>+    // Disable the rotate view menuitem
>+    document.getElementById("calendar_toggle_orientation_command")
>+            .setAttribute("disabled", "true");
>+    window.setCursor("auto");
>+  }
>+}

Why is the rotation of day/week view handled here?


>diff --git a/calendar/lightning/content/messenger-overlay-sidebar.xul b/calendar/lightning/content/messenger-overlay-sidebar.xul
>--- a/calendar/lightning/content/messenger-overlay-sidebar.xul
>+++ b/calendar/lightning/content/messenger-overlay-sidebar.xul

>+        <menuitem id="tasks-view-filter-all"
>+                  name="filtergroup"
>+                  value="all"
>+                  type="radio"
>+                  command="calendar_task_filter_command"
>+                  label="&calendar.task.filter.all.label;"
>+                  accesskey="&calendar.task.filter.all.accesskey;"/>

name="filtergroup" seems to be obsolete for this and the other menuitems!


>diff --git a/calendar/locales/en-US/chrome/calendar/menuOverlay.dtd b/calendar/locales/en-US/chrome/calendar/menuOverlay.dtd
>--- a/calendar/locales/en-US/chrome/calendar/menuOverlay.dtd
>+++ b/calendar/locales/en-US/chrome/calendar/menuOverlay.dtd

>-<!ENTITY calendar.export.calendar.label         "Export Calendarâ?¦">
>-<!ENTITY calendar.export.calendar.accesskey     "E">
>+<!ENTITY calendar.export.label                  "Exportâ?¦">
>+<!ENTITY calendar.export.accesskey              "E">

Why do you drop the 'Calendar' part?


>-<!ENTITY calendar.importcalendar.label          "Import Calendarâ?¦">
>-

Why do you drop the 'Calendar' part?

Comment 17

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #15)
> (From update of attachment 379483 [details] [diff] [review])
> I think the File -> New -> Event, Task, Calendar is unnecessary.  It's both a
> duplicate and I don't think people would go there when there is already an
> Events & Tasks top level menu.
I just thought it may make sense for sake of consistancy. Different users may have different ways of creating new items. Users that are used to using File > New > Message may wonder why there is no File > New > Event. Your call, should I really remove them?

Comment 18

[Martin Schröder [:mschroeder]]

Comment on attachment 379483 [details] [diff] [review]
Fix - v2

Now the observations from functional review. Maybe some issue are better suited for follow-up bugs. ;)

'View' menu:
------------
* 'Calendar > Minimonth' enables/disables the minimonth also in Task mode, intended?
* 'Tasks' enabled in Calendar tab, intended?
* Missing 'Show Tasks', 'Show Events', 'Show Events and Tasks' are needed for 'Today Pane'.
* Possibility to change calendar view in Task tab should be removed.
* 'Current View > Workweek Days Only' active in calendar day view where it does not make sense
* 'Tasks': No filter selected after start up
* 'Rotate View' the same for day & week view, intended?

'Go' menu:
----------
* Separator above 'Today'

'Events and Tasks' menu:
------------------------
* 'Find Events' also active in Task mode, intended?
* Missing 'Privacy', 'Priority' and 'Status' for editing events and task.
* Today Pane in Mail tab does not allow to edit a shown task using the 'Events and Tasks' menu.
* Contrast problem with <menupopup type="task-priority"/> (with Windows XP, Classic theme; also see additional attachment).

Comment 19

[Martin Schröder [:mschroeder]]

Attachment: menu highlight contrast issue

Comment 20

[Martin Schröder [:mschroeder]]

Comment on attachment 379483 [details] [diff] [review]
Fix - v2

r-, but just to set a flag, and because I want to have a look at the next/final patch. ;)

Comment 21

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #16)


> > function toggleCompleted(aEvent) {
> Check if aEvent is null?
I think we shouldn't. Since the function only works if there is an event anyway, we should not swallow the error if no event is passed to make sure developers don't accidentally forget to pass the event.

> >+    document.getElementById("modeBroadcaster").setAttribute("checked", 
> What is the 'checked' attribute for?
Not quite sure, I moved this out of ltnInitializeMenus, I think things didn't work without.

> >+function ltnSwitch2Mail() {
> Why is the rotation of day/week view handled here?
Good question. Probably historical reasons. I moved this into our command handler.

> name="filtergroup" seems to be obsolete for this and the other menuitems!
In case someone decides to put another radio group into that menu, I'd rather leave them in there.

> >-<!ENTITY calendar.export.calendar.label         "Export Calendarâ?¦">
> >-<!ENTITY calendar.importcalendar.label          "Import Calendarâ?¦">
> Why do you drop the 'Calendar' part?
Because I changed the entity (also, why another calendar when there is already one? ;-)

(In reply to comment #18)
> 'View' menu:
> ------------
> * 'Calendar > Minimonth' enables/disables the minimonth also in Task mode,
> intended?
> * 'Rotate View' the same for day & week view, intended?
Kind of. This is one of the issues mentioned in the above comments. How do we decide whether a menuitem is mode dependant or not? Doing so may make it confusing for the user, but not doing so may make users complain that they can not set it independantly.



> * Missing 'Show Tasks', 'Show Events', 'Show Events and Tasks' are needed for
> 'Today Pane'.
Did we have this before?

> * Possibility to change calendar view in Task tab should be removed.
Kind of hard to do, we don't want to disable those commands since they are also used to switch from mail mode to calendar mode. Disabling the menu might work, but right now hiding the minimonth is not mode-dependent. I'll leave this for later.

> * 'Tasks': No filter selected after start up
Works for me


> 'Events and Tasks' menu:
> ------------------------
> * Missing 'Privacy', 'Priority' and 'Status' for editing events and task.
We don't have bindings for status and privacy. I added progress though, priority was already there. Unfortunately there is a problem here again with selection and focus. right now todo_items_selected returns true if you select a task and then switch modes, we have no good and easy way to check if any <calendar-task-tree> is visible. I'll leave this to a follow up bug too.


> * Today Pane in Mail tab does not allow to edit a shown task using the 'Events
> and Tasks' menu.
Same issue, same follow up bug.


> * Contrast problem with <menupopup type="task-priority"/> (with Windows XP,
> Classic theme; also see additional attachment).
I have no working windows platform to develop this on, maybe you can look into this? Otherwise followup bug.


Other issues taken care, will attach patch later

Comment 22

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v3

Comment 23

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #17)
> (In reply to comment #15)
> > (From update of attachment 379483 [details] [diff] [review] [details])
> > I think the File -> New -> Event, Task, Calendar is unnecessary.  It's both a
> > duplicate and I don't think people would go there when there is already an
> > Events & Tasks top level menu.
> I just thought it may make sense for sake of consistancy. Different users may
> have different ways of creating new items. Users that are used to using File >
> New > Message may wonder why there is no File > New > Event. Your call, should
> I really remove them?

Since it's a submenu I don't think it's a big deal to leave them in and is a win for consistency.

Comment 24

[Martin Schröder [:mschroeder]]

Comment on attachment 383237 [details] [diff] [review]
Fix - v3

Two things you could have a look at before checkin:
* Setting priority from the menu does not work for events.
* View > Calendar should be disabled in Mail mode.

I still observe problems with no selected filter, and also no priority/progress for selected tasks. For this and other issues followup bugs should be filed.

r=mschroeder

Comment 25

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #24)
> (From update of attachment 383237 [details] [diff] [review])
> Two things you could have a look at before checkin:
> * Setting priority from the menu does not work for events.
This causes some more work, since the binding is currently only implemented for the selected tasks. I filed a followup bug for this.

> * View > Calendar should be disabled in Mail mode.
Done


I have filed the following followup bugs: 

bug 499472 - Menu View > Tasks: No filter selected at startup
bug 499474 - Task operations can be done when selecting a task and then changing modes
bug 499475 - Task in Today Pane in Mail mode does not allow editing using the 'Events and Tasks' menu
bug 499476 - Text color is wrong for menus that use the "task-priority" binding
bug 499478 - Can't set priority for events from the new "Events and Tasks" menu
bug 499480 - Create bindings or menu items for setting progress and status from the events and tasks menu

Comment 26

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/ffb301b5cd6b>

-> FIXED