386479 - Switch to Calendar mode don't work properly using buttons, Calendar menu or keyboard shortcut

Status: VERIFIED FIXED

(Calendar :: Lightning Only, defect)

Description

[Omar Bajraszewski]

After update 0.5--->0.7 (lightning 0.7pre 2007063017) I noticed that toolbar buttons don't work very well with new switch mode


1) Install lightning 0.5
2) Add calendar buttons to toolbar: day view, month view and week view.
3) Install lightning 0.7pre 2007063017
4) While you are in mail view click month day button

Result:
1) The tab in lower left corner still shows mail mode


I think that after updating from 0.5 to 0.7 the calendar buttons should be removed from toolbar or selecting them should change view mode to calendar mode

Comment 1

[Stefan Sitter [:ssitter]]

Same happens if the Calendar buttons are added to the menu bar (like in my setup). I think the switch to calendar view should also switch the sidebar to calendar mode.

Comment 2

[Stefan Sitter [:ssitter]]

Using the keyboard shortcut for calendar mode (Ctrl+3) has the same issue. The view is switched to calendar but the sidebar stays in mail mode. I think this is all the same issue and this bug should handle all the incidents.

Comment 3

[Jean-Michel Reghem]

confirming this
with 0.5 i had adding a "this month" button in the mail toolbar

after upgrading to 0.7, if i click on this button, it switch to the calendar, but button in mode toolbar stay in "mail" and not calendar

--> notice that it is no more possible to add a "this month" button to the mail toolbar, so i remove it ... but yes, it should be done automatically

Comment 4

[Steffen Wilberg]

Same with the Calendar menu, e.g. Calendar -> Month.

Comment 5

[Sven Giermann]

Attachment: Patch to assure Calendar-Mode in ShowCalendarView

This one calls ltnSwitch2Calendar, if not already in Calendar-Mode.
I also had to slightly modify ltnSwitch2Calendar() to set the current mode before calling ShowCalendarView() to avoid recursion errors.

Comment 6

[Michael Büttner (no reviews TFN)]

Comment on attachment 277683 [details] [diff] [review]
Patch to assure Calendar-Mode in ShowCalendarView

>+    // assure CalendarMode first (we get called by this again)
>+    if (ltnCurrentMode() != 'calendar') {
>+      ltnSwitch2Calendar();
>+      return;
>+    }
These lines are indeed missing and are perfectly correct (although the indentation is wrong), but they are just half of the story. If we should find toolbar buttons configured in the mail toolbar that belong to the calendar mode they should be removed. These buttons all have an attribute called 'mode' that is set to 'calendar'. If we find such an element to be a child of the 'mail-bar2' it should be removed (delete the element from the tree and remove it from the current set of the mail toolbar). We shouldn't allow functions to be available in the wrong mode, although the above lines are indeed missing.

Comment 7

[Sven Giermann]

Mickey,

thanks for your commendation! Indeed it should be discussed, whether the old (and no more available) buttons should be removed - but I would suggest to file a spin-off bug for that for two reasons:

1. The removal of buttons is more cosmetical while the subject of this bug is strict functional.

2. I don't know whether it is right to just remove the buttons and do not make them available for mail mode...
Why don't we offer functions for the toolbar, that are still accessible through the menu?

Of course it makes sense to have 2 different toolbars (as in 3-pane-view and message-view), because we probably don't need functions like 'Reply' or 'Forward' in mail-mode. There are really much more of both modes, that are not required or even not functional in the other mode.

But I would still like to have the following features in future versions:
Mail-mode: 'New Event', 'New Task', 'GoToToday'
Calendar-mode: 'Compose', 'Addressbook'

What do you think about that? Would it be possible to have 'dual-mode' buttons?
But as mentioned above, this should go into another spin-off bug.
So, please reconsider your review or tell me your opinion regarding the above.

Comment 8

[Martin Schröder [:mschroeder]]

Mickey, can you answer comment#7?

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: lightning mode switch keys

In addition to the patch Sven proposed, this patch takes care of allowing to use Ctrl+1 and Ctrl+3 to switch between modes. This patch is independent of Sven's patch though.

I'd like to have this patch for 0.7!

Comment 10

[Michael Büttner (no reviews TFN)]

(In reply to comment #7)
> What do you think about that? Would it be possible to have 'dual-mode'
> buttons? But as mentioned above, this should go into another spin-off bug.
The initial idea was to have separate toolbars for the different modes. Commands are tied to one and only one mode, which suggests to only allow commands (toolbar buttons, that is) to be available in the mode that they apply to. Some commands make sense in more than one mode (e.g. delete), but have different semantics (in mail mode delete a mail, in calendar mode delete an event). Allowing buttons to be available in a mode that their commands doesn't belong to goes against the initial idea (and adds to the confusion in case of commands with semantics depending on the mode).

In essence this is the reason why allowing calendar-buttons in the mail-mode or visa versa should be avoided. At least that's my opinion, but as always I'll obey to Christian's final decision. Christian, any comment on this?

Comment 11

[Michael Büttner (no reviews TFN)]

Comment on attachment 280382 [details] [diff] [review]
lightning mode switch keys

>+toMessengerWindow = function ltnToMessengerWindow() {
Please add a comment that you're overriding the function from mail\base\content\mailCore.js.

>+     window.open(uri, "_blank", ...
Please add
> var uri = "chrome://messenger/content/messenger.xul";
otherwise window.open() will throw since uri won't be defined.

>    gLastShownCalendarView = type;
>
>+    if (gCurrentMode != 'calendar') {
>+      ltnSwitch2Calendar();
>+      return;
>+    }
>
>    // If we got this call while a mail-view...
>    // hide all of the mail stuff so we have...
>    var calendarViewBox = ...
Please also add the above lines marked with '+' to messenger-overlay-sidebar.js into function showCalendarView(). This ensures that if a calendar button happens to be available in mail mode, we're at least switching back to calendar mode instead of ending up with showing a calendar view while still being in mail mode.

r=mickey with the above comments being addressed.

Comment 12

[Christian Jansen]

(In reply to comment #10)
> (In reply to comment #7)
[...]
> 

I totally agree with Mickey.

Comment 13

[Michael Büttner (no reviews TFN)]

Philipp, this needs to land before code freeze. Any reason why this still waits to be checked in?

Comment 14

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED

Comment 15

[Sven Giermann]

(In reply to comment #10)
> In essence this is the reason why allowing calendar-buttons in the mail-mode or
> visa versa should be avoided.

Filed spin-off bug 396144, as proposed in comment #7.

Comment 16

[Omar Bajraszewski]

Verified with 2007092104