359562 - Week/Day(Multiday) view orientation should persist

Status: RESOLVED WORKSFORME

(Calendar :: Calendar Frontend, defect)

Description

[gekacheka]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20061029 Calendar/0.4a1

Week view and Day view can be oriented with the time axis running either vertically or horizontally, but when Sunbird is restarted, the orientation is always vertical.

Persisting the orientation will restore it as it was when Sunbird was last exited by the user, so users can easily return to what they were last doing.

Reproducible: Always

Steps to Reproduce:
1. Go to week view.
2. Put Week view in horizontal orientation (time axis running horizontally)
3. Exit sunbird and restart


Actual Results:  
Week view in vertical orientation (time axis running vertically)

Expected Results:  
Week view in horizontal orientation, as it was.

Comment 1

[gekacheka]

Attachment: patch decorated views to propagate orient so it can be persisted

(patch -l -p 2 -i file.patch)

To persist an attribute, it must be on an element with a unique id.
Orientation changes are implemented in calendar-multiday-view, but it does not have a unique id where it is called.  It inherits orient from its caller, calendar-decorated-(week/day)-view, but orient changes were not propagated back to the caller.

So this patch 

1. modifies calendar-decorated-(week/day)-view to inherit orient from its call and pass it to its calendar-multiday-view, 
2. adds a listener to each so when the calendar-multiday-view orient changes, they are propagated back.
3. adds orient="vertical" and persist="orient" to their calls in calendar.xul so the orientation will be persisted.

With this change, both Week View and Day View now have persistent orientation, so after a restart the orientation is the same as it was when Sunbird was last exited.

Comment 2

[gekacheka]

Comment on attachment 244730 [details] [diff] [review]
patch decorated views to propagate orient so it can be persisted

mvl, do you agree persisting each view's orientation can be good UI?  It helps eliminate the need for more preferences (bug 343484 comment 8).

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 244730 [details] [diff] [review]
patch decorated views to propagate orient so it can be persisted

yeah, the orientation should persist. ui-review=mvl

Comment 4

[Matthew (lilmatt) Willis]

Comment on attachment 244730 [details] [diff] [review]
patch decorated views to propagate orient so it can be persisted

These comments apply to both calendar-decorated-day-view.xml and calendar-decorated-multiday-view.xml.
>--- mozilla/calendar/base/content/calendar-decorated-day-view.xml	2006-09-18 15:49:42.000000000 -0400
>@@ -71,7 +71,22 @@
> 
>                 viewElement.mMinVerticalPixelsPerMinute = 0.7;
>                 viewElement.mMinHorizontalPixelsPerMinute = 1.5;
>-                return;
>+
>+                // add a listener to view-element to propagate orient changes
This should read "orientation changes", and therefore will have to wrap to a second line.

>+                const decoratedView = this;
>+                const multidayView = document.getAnonymousElementByAttribute(
>+                                        this, "anonid", "view-element");
1. Why are these constants instead of regular variables?
2. Why can't we reuse viewElement from above rather than creating multidayView?

>+                function onAttrChange(event) {
>+                    if (event.eventPhase == event.AT_TARGET &&
>+                        event.attrName == "orient") {
Style nit: I'd wrap each comparison in ()s for readibility.

>+                      // propagate orientation change up so it can be persisted
>+                      var newOrient = multidayView.getAttribute("orient");
>+                      if (newOrient != decoratedView.getAttribute("orient")) { 
Trailing space on this line ^^^^

>+                        decoratedView.setAttribute("orient", newOrient);
>+                      }
>+                    }
You've also dropped to 2-space indenting here. 4-space is prevailing style in this file.

>+                }
>+                multidayView.addEventListener("DOMAttrModified", onAttrChange, false);
>             ]]></constructor>


Rather than copy/paste this code into both views, I wonder if there is a good way to include it in one of the parent xbl files so both views pick it up?

r- until we get the questions answered and the nits addressed

Comment 5

[gekacheka]

Attachment: buggy approach: add handler to multiday-view (might be more efficient if it can work?)

(patch -l -p 2 -i file.patch)

One approach to sharing the code is to make the listener a handler of multiday-view.  I thought this would be better because handler phase="target" might filter out more events so it might be able to run much less often.   [The addEventListener method does not provide a way to say to run the listener only during the target phase (i.e., not for events whose target is a child node).]  

However, I wasn't able to get it to work.  Strangely, the handler seems to receive DOMAttrModified events when the multiday-view reorient method reorients its descendants.  The events have the root multiday-view as the event.target, and if it sets the decoratingView orient attribute, some of the children end up in the wrong orientation.

Comment 6

[gekacheka]

Attachment: v2: patch decorated views to propagate orient so it can be persisted (add listener via decorated-base)

(patch -l -p 2 -i file.patch)

This approach moves the code for the listener into the base class.

Comment 7

[gekacheka]

Attachment: v3: patch decorated views to propagate orient so it can be persisted (add handler down in multiday-view)

(patch -l -p 2 -i file.patch)

This patch fixes the approach moving the propagator down into a handler of multiday-view.  The handler phase="target" attribute may filter out events so it might be able to run much less often than v2.   [The addEventListener method does not provide a way to say to run the listener only during the target phase (i.e., not for events whose target is a child node).]

This version avoids using event.newValue to workaround the erroneous events; instead it looks up the current value of the orient attribute.

Comment 8

[Matthew (lilmatt) Willis]

Comment on attachment 247293 [details] [diff] [review]
v3: patch decorated views to propagate orient so it can be persisted (add handler down in multiday-view)

In discussing this architecture with jminta and dmose, we came to the conclusion that rather than adding setDecoratingView, you probably want to use an event listener to do this.  You may need and/or want to fire your own event for when the orientation is changed, and listen just for that, or you may be able to find another event to piggyback off of.

Either way, using listeners would be preferred over the implementation in this patch.

Comment 9

[gekacheka]

Attachment: v4: patch decorated views to propagate orient so it can be persisted (fire reorient event in multiday view, add listener up in decorated-base)

(patch -l -p 2 -i file.patch)

Taking your good advice, this version extends the multiday-view reorient() method to fire a new "reorient" event with the new orientation.  Decorated-base-view provides initializeReorientListener to subscribe a listener for this event.

Comment 10

[Matthew (lilmatt) Willis]

Comment on attachment 248130 [details] [diff] [review]
v4: patch decorated views to propagate orient so it can be persisted (fire reorient event in multiday view, add listener up in decorated-base)

>--- mozilla/calendar/base/content/calendar-decorated-base.xml	2006-12-09 18:38:07.615590400 -0500
>+                    const VIEW = document.getAnonymousElementByAttribute(this, "anonid", "view-element");
>+                    VIEW.addEventListener("reorient", onReorient, false);
>+                ]]></body>
>+            </method>
>+            
This line ^^ has a bunch of trailing spaces


>--- mozilla/calendar/base/content/calendar-decorated-week-view.xml	2006-12-09 18:38:07.625604800 -0500
>+          // Notify decorating views so they can persist orientation.
>+          // At end so if this reorient threw an error, it is not persisted.
>+          this.fireEvent("reorient", orient);
The second comment needs a subject.  "This is located at the end so if..."


>--- mozilla/calendar/sunbird/base/content/calendar.xul	2006-12-09 18:38:13.524086400 -0500
>       <deck id="view-deck" flex="1" persist="selectedIndex">
>           <calendar-decorated-day-view id="day-view" flex="1" 
>-                                       context="context-menu"/>
>-          <calendar-decorated-week-view id="week-view" flex="1" 
>-                                        context="context-menu"/>
>+                                       context="context-menu"
>+                                       orient="vertical" persist="orient"/>
>+          <calendar-decorated-week-view id="week-view" flex="1"
>+                                        context="context-menu"
>+                                        orient="vertical" persist="orient" />
Remove the space between "orient" and />

All style nits.  Much cleaner implementation. Thanks!

r=lilmatt

Comment 11

[gekacheka]

Attachment: v5: patch decorated views to propagate orient so it can be persisted (fire reorient event in multiday view, add listener up in decorated-base) (nits addressed)

(patch -l -p 2 -i file.patch)

Comment 0 describes problem
Comment 1 describes approach
Comment 9 describes modularity

Comment 12

[Joey Minta]

Comment on attachment 248828 [details] [diff] [review]
v5: patch decorated views to propagate orient so it can be persisted (fire reorient event in multiday view, add listener up in decorated-base) (nits addressed)

Won't this leak on tb1.5, since we don't ever remove the listener?

Comment 13

[gekacheka]

> (From update of attachment 248828 [details] [diff] [review] [edit])
> Won't this leak on tb1.5, since we don't ever remove the listener?
> 
Could you elaborate a leak scenario?  During initialization the listener is added once to the multiday-view.  The multiday-view is never removed from the decorated view.  The decorated view is never removed from the window.  The window is never destroyed.  Where is the leak?  When would you like the listener removed?

Comment 14

[Joey Minta]

(In reply to comment #13)
> The
> window is never destroyed.  Where is the leak?  When would you like the
> listener removed?
> 
The window is destroyed, both in Sunbird on Mac (where closing a window does not quit an application) and in Thunderbird across the board (where double-clicking on an account opens it in a new window that can be closed).  Also, leaks-on-shutdown are still considered leaks.

I'd like the listener removed when the binding is destroyed.

Comment 15

[gekacheka]

Attachment: v6: patch decorated views to propagate orient so it can be persisted (fire reorient event in multiday view, add listener up in decorated-base) (destructors added)

(patch -l -p 2 -i file.patch)

As requested, this version removes the listener from the destructors.  It adds method "uninstallReorientListener" to decorated-base and calls it from the destructors of decorated week and day views.

Comment 16

[Joey Minta]

Comment on attachment 248993 [details] [diff] [review]
v6: patch decorated views to propagate orient so it can be persisted (fire reorient event in multiday view, add listener up in decorated-base) (destructors added)

+                    const SELF = this;
While I like this in principle, none of the rest of calendar/ uses constants this liberally.  Please use var and lowercase to remain consistent

+                    this.mOnReorientListener = onReorient;
Just assign when you're declaring the function.

+                    const VIEW = document.getAnonymousElementByAttribute(this, "anonid", "view-element");
Same comment as above applies here and throughout the patch.

-                return;
+                this.uninstallReorientListener();
This is dmose's pet peeve.  He says that without an explicit |return| it's  too hard to tell whether there was a coding mistake, simply falling off the end of the function.  The review comments for this line's blame probably include something to that effect.  I'd get in trouble if I let you remove it.

r=jminta with those.

Comment 17

[gekacheka]

Attachment: v7: patch decorated views to propagate orient so it can be persisted (fire reorient event in multiday view, add listener up in decorated-base) (nits addressed)

(patch -l -p 2 -i file.patch)

Nits addressed.
(Sorry, I intended to request second review last time)

(It would be interesting to know if Tamarin (JS JIT compiler) will recognize constant free name bindings and implement closures more efficiently in that case.  Using 'const' at least conveys the opportunity for less indirection in case some future implementation can use it.)

The 'return' convention doesn't apply to constructors and destructors since they do not return a value.

Comment 18

[gekacheka]

Comment on attachment 250119 [details] [diff] [review]
v7: patch decorated views to propagate orient so it can be persisted (fire reorient event in multiday view, add listener up in decorated-base) (nits addressed)

Removed request for 2nd review.  Incorporated ideas into bug 366989, but more modularly for use to propagate more than one attribute and event.

Comment 19

[Stefan Sitter [:ssitter]]

WORKSFORME using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.13pre) Gecko/2008020921 Calendar/0.8pre.