516802 - Refactor & cleanup mode and calendar view switching + today pane code

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Martin Schröder [:mschroeder]]

Attachment: Patch v1

Changes in this patch:

* Remove collapse/uncollapse of calendar-view-box and no longer needed messenger-overlay-sidebar.css (Lightning only)

* Remove unused "checked" attribute from switch2task/mail/calendar commands (Lightning only)

* Correct name of "ltnCalendarMenuPopup" (Lightning only)

* Disable "ltnCalendarMenu" when not in calendar or task mode (Lightning only)

* Enable switching of calendar views just in calendar mode and Sunbird

* Moved "calendar_day-view_command", "calendar_week-view_command", "calendar_multiweek-view_command", "calendar_month-view_command" to calendarController and use goDoCommand now to call them

* Merge functions "selectCalendarView" and "showCalendarView" to "switchCalendarView" with addditional argument for forcing calendar view to foreground and removal of unused argument

* Move call of function "onCalendarViewResize" from function "showCalendarView" to end of function "switchToView"

* Merge functions "ltnSelectCalendarView" and ltnShowCalendarView" to "ltnSwitchCalendarView" with additional argument for forcing calendar view to foreground (Lightning only)

Comment 1

[Martin Schröder [:mschroeder]]

Attachment: [checked in] Patch (2) v1

Comment 2

[Martin Schröder [:mschroeder]]

Comment on attachment 400994 [details] [diff] [review]
[checked in] Patch (2) v1

Changes in this patch:

* Cleanup whitespace & intending, add comments, change "let" to "var" and add "cal." prefix for calUtils functions where appropriate in today-pane.js

* Replace wrapper for "onLoad" handler with direct call of "TodayPane.onLoad" in today-pane.js

* Add listener for window "unload" event to call function "onLoad" again!

* Move "document.getElementById('miniday-month-panel').hidePopup()" from function "setDaywithjsDate" in today-pane.js to element "todayMinimonth" in today-pane.xul

* Remove unused function "showsYesterday" from today-pane.js and function "yesterday" from messenger-overlay-sidebar.js (only caller was "showsYesterday"!)

* Remove unused function "findMailSearchBox" from messenger-overlay-sidebar.js

* Change call of "storeWidthAndState" to "TodayPane.storeWidthAndState" in messenger-overlay-sidebar.xul after moving the function to the "TodayPane" object

* Fix "calendar-invitations-label" keypress fucntion in messenger-overlay-sidebar.xul

Comment 3

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

Comment on attachment 400856 [details] [diff] [review]
Patch v1

>-                  observes="calendar_day-view_command"/>
>+                  command="calendar_day-view_command"/>
I'm not quite sure how this was, but its quite possible that using command= only works for taking over the disabled attribute. We need either both or only observes and should test this before checking in.


Aside from that the code looks fine, I haven't tested it though. r=philipp for the code.

Comment 4

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

Comment on attachment 400994 [details] [diff] [review]
[checked in] Patch (2) v1

This one looks fine, r=philipp

Comment 5

[Martin Schröder [:mschroeder]]

Comment on attachment 400994 [details] [diff] [review]
[checked in] Patch (2) v1

http://hg.mozilla.org/comm-central/rev/a9907d7f1cde
http://hg.mozilla.org/releases/comm-1.9.1/rev/48172a160ded

Comment 6

[Martin Schröder [:mschroeder]]

(In reply to comment #3)
> (From update of attachment 400856 [details] [diff] [review])
> >-                  observes="calendar_day-view_command"/>
> >+                  command="calendar_day-view_command"/>
> I'm not quite sure how this was, but its quite possible that using command=
> only works for taking over the disabled attribute. We need either both or only
> observes and should test this before checking in.

Philipp, which other attributes than "disabled" are used in this case (for a menuitem)? I couldn't find any.

Comment 7

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

(In reply to comment #6)
> Philipp, which other attributes than "disabled" are used in this case (for a
> menuitem)? I couldn't find any.

Since its a type="radio", I belive it uses the "checked" attribute.

Comment 8

[Martin Schröder [:mschroeder]]

(In reply to comment #7)
> (In reply to comment #6)
> > Philipp, which other attributes than "disabled" are used in this case (for a
> > menuitem)? I couldn't find any.
> 
> Since its a type="radio", I belive it uses the "checked" attribute.

I doublechecked everything, and the view switching seems to work flawlessly with just using the "command" attribute and not "observes".

But I'll attach an updated patch with includes a fix for a minor bug in my patch.

Comment 9

[Martin Schröder [:mschroeder]]

Attachment: Patch v2

Requesting review for additional changes to the patch:
* Removed unused command "switch2mail"
* Updated comment on valid strings for modes (used in modeBroadcaster)
* Removed spare semicolon
* Added type="checkbox" to "Calendar"/"Tasks" menuitem in "Events and Tasks" menu.
* Added "observes" element to those menuitems to get checked value indirectly from the modeBroadcaster (instead of using a "checked" attribute on the "switch2task" and "switch2calendar" commands).

Comment 10

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

(In reply to comment #9)
> * Added type="checkbox" to "Calendar"/"Tasks" menuitem in "Events and Tasks"
> menu.
Unchecking the entry is not possible, i.e doesn't close the tab. I'd suggest keeping the menuitem as is, since unchecking causing a tab to close seems quite uncommon to me.

Do you want to take this patch for the beta? It looks quite invasive?

Comment 11

[Martin Schröder [:mschroeder]]

(In reply to comment #10)
> (In reply to comment #9)
> Do you want to take this patch for the beta? It looks quite invasive?

Not necessarily.

Comment 12

[Martin Schröder [:mschroeder]]

Attachment: [medium,test] Patch v3

I improved the behavior of the menuitem by using autocheck="false" to prevent clicks form toggling the checkmark. I think the user experience is okay, but we can come back later to discuss other solutions.

Comment 14

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

Comment on attachment 410352 [details] [diff] [review]
[medium,test] Patch v3


>+            case "calendar_week-view_command":
>+                switchCalendarView("week", true);
>+                break;
If we handle the commands like this, we probably don't have to use a - between week and view. This was done for easier extration of the view name from the command, maybe you can get rid of that too (this bug or other).


>-function showCalendarView(type, event) {
>+function switchCalendarView(aType, aShow) {
>     if (isSunbird()) {
Go ahead and change to cal.isSunbird()


>+
>+    onCalendarViewResize();
> }
I believe the resize handler was recently removed, do we still need this?


> function ltnSwitch2Mail() {
>   if (gCurrentMode != 'mail') {
>     gCurrentMode = 'mail';
>     document.getElementById("modeBroadcaster").setAttribute("mode", gCurrentMode);
> 
>     document.commandDispatcher.updateCommands('calendar_commands');
>     window.setCursor("auto");
>   }
> }
Most of this code is called by each mode switcher, can this be further consolidated?


>     <menu id="ltnCalendarMenu"
>+          observes="calendar_in_foreground"
>           insertbefore="viewSortMenuSeparator"
>           label="&lightning.menu.view.calendar.label;"
>           accesskey="&lightning.menu.view.calendar.accesskey;">
Don't we want to allow switching to the calendar via  View > Calendar menu?


I haven't tested this yet. If you feel comfortable with this patch, go ahead and check in. Otherwise I'll test it this evening or tomorrow. I'm especially interested how the checkbox in the events and tasks menu works. Does unchecking close the tab?

r=philipp

Comment 15

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

Martin, whats the state of this patch? Do you want to check it in?

Comment 16

[Martin Schröder [:mschroeder]]

(In reply to comment #15)
> Martin, whats the state of this patch? Do you want to check it in?

I'll fix the nits, and check it in asap.

Comment 17

[Martin Schröder [:mschroeder]]

Checked in Patch v3: http://hg.mozilla.org/comm-central/rev/0b4c2f8b864b

Comment 18

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

This is needed-beta, feel free to check in on comm-1.9.2 any time.

Comment 19

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

Backported to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/218aba6ba35b>
a=philipp