Consolidation of the navigation bar

VERIFIED FIXED in 1.0b1

Status

Calendar
Calendar Views
VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: Berend Cornelius, Assigned: Berend Cornelius)

Tracking

unspecified
1.0b1

Details

Attachments

(6 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
The implementation of the new navigation bar has been done within  Bug 454933 -  [calendar integration] move month, day, week mode buttons into calendar view
but as I mentioned already in bug 454933 comment #4 the way this bar is embedded within the views is kind of suboptimal: Historically we had different navigation bars for each view which lead us to the concept of the decorated views that again embed the different calendar-views. The current navigation bar however is basically the same over all views except for the description of the displayed range. In this context it is kind of suboptimal to have the tab-buttons implemented separately for each view - there must be only one tabbox control with the tab-buttons above all views. I noticed a certain focus flaw which probably also derives from this redundancy.
So I suggest to make the navigation bar singular over all calendar-views. The final consequence of this would be to make the decorated view obsolete and merge the methods of "calIDecoratedView.idl" into "calIcalendarView.idl" and into the navigation-bar and to replace the existing view-deck control by a tabbox control.
(Assignee)

Comment 1

9 years ago
Created attachment 351870 [details] [diff] [review]
patch v. #1

first steps - that should not harm too much - to achieve of what I tried to explain in the bug description.
Assignee: nobody → Berend.Cornelius
Status: NEW → ASSIGNED
(Assignee)

Comment 2

9 years ago
Created attachment 351872 [details] [diff] [review]
patch v. #2

detected and resloved a bug that was not displayed by the error console.
Attachment #351870 - Attachment is obsolete: true
(Assignee)

Comment 3

9 years ago
Created attachment 355973 [details] [diff] [review]
[checked in] patch v. #3

I think this issue is too big to put it into one patch, so I deliver a first one that majorly moves the navigationbar out of the decorated-view-panes, so that becomes a singleton. Alongside I removed some no longer needed functions in messenger-overlay-sidebar.js - hope that's ok.
Attachment #351872 - Attachment is obsolete: true
Attachment #355973 - Flags: review?(philipp)
Comment on attachment 355973 [details] [diff] [review]
[checked in] patch v. #3

>diff --git a/calendar/base/content/calendar-common-sets.xul b/calendar/base/content/calendar-common-sets.xul
>+    <vbox id="calendar-view-box" context="calendar-view-context-menu">
Sorry I didn't say anything earlier, but I don't think common-sets is the right file for this. That file was thought for things like: popupset, commandset, stringbundlesets, broadcastersets, ...

I'd rather see a new file, maybe calendar-view-overlay.xul or maybe also calendar-views.xul 

>+++ b/calendar/lightning/content/messenger-overlay-sidebar.xul
>+          <vbox id="calendar-view-box"/>
Please add a comment that this is needed since it will be overlayed by the calendar core code.


>diff --git a/calendar/sunbird/base/content/calendar.xul b/calendar/sunbird/base/content/calendar.xul
>+    <vbox id="calendar-view-box" flex="1"/>
Same here.

r=philipp
Attachment #355973 - Flags: review?(philipp) → review+
(Assignee)

Comment 5

9 years ago
pushed to commm-central:

http://hg.mozilla.org/comm-central/rev/da405ac09a9a

When giving this patch a final test after the checkin I detected bug:
The Unifinder overlays itself between the navigation-bar and the calendar views as can be seen when the Unifinder is opened (check Menu item View/Find Events). But it should appear above the navigation-bar. I will deliver a second patch to resolve this.
(Assignee)

Comment 6

9 years ago
Created attachment 356490 [details] [diff] [review]
[checked in] patch v. #4

This should fix it. I also removed some deprecated sources from calendar-common-sets.xul.
Attachment #356490 - Flags: review?(philipp)
Attachment #355973 - Attachment description: patch v. #3 → [checked in] patch v. #3
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → 1.0
Comment on attachment 356490 [details] [diff] [review]
[checked in] patch v. #4

r=philipp
Attachment #356490 - Flags: review?(philipp) → review+
(Assignee)

Comment 8

9 years ago
patch v. #4 pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/a048b5420d62

issue remains open
(Assignee)

Updated

9 years ago
Attachment #356490 - Attachment description: patch v. #4 → [checked in] patch v. #4
(Assignee)

Comment 9

9 years ago
Created attachment 356732 [details] [diff] [review]
[checked in] patch v. #5

With this patch I moved the navigationbar into the new file calendar-views.xul. I don't see a need for a binding in this case anymore and this way we have all the code around the calendar-views in one place.
Attachment #356732 - Flags: review?(philipp)
Attachment #356732 - Flags: review?(philipp) → review+
Comment on attachment 356732 [details] [diff] [review]
[checked in] patch v. #5

>+                    navigationBar.setDateRange(this.rangeStartDate, this.rangeEndDate, aToolTipTexts);
Since we don't have any namespaces in chrome js files, I think we should either name this calNavigationBar or create a namespace object cal and then have the bar be called cal.navigationBar

>+++ b/calendar/base/content/calendar-views.js
>+ *   Berend Cornelius <berend.cornelius@sun.com> *
Remove extra star at end of line.


>+let navigationBar = {
>+};
I'd prefer the syntax:

let navigationBar = {
  onLoad: function loadNavigationBar() { ... }
};

in case you prefer to go with calNavigationBar or otherwise:

var cal = cal || {};
cal.navigationBar = {
  onLoad: function loadNavigationBar() { ... }
};


>+++ b/calendar/base/content/calendar-views.xul
>+<?xml-stylesheet type="text/css" href="chrome://global/skin/global.css"?>
Why do you need global skin here? calendar-views.xul isn't an extra dialog and global.css should be included through the opening dialog.

>+<!DOCTYPE overlay[
>+    <!ENTITY % dtd4 SYSTEM "chrome://calendar/locale/calendar.dtd" > %dtd4;
> ]>
>+
<!DOCTYPE overlay SYSTEM "chrome://calendar/locale/calendar.dtd" >


>+        <hbox id="nav-control">
We might want to prefix with calendar- since other extensions might think of the same name?

>+       <deck flex="1"
>             id="view-deck"
>             persist="selectedIndex"
>             ondraggesture="nsDragAndDrop.startDrag(event, calendarViewDNDObserver);"
If I saw this right the view-deck is now in calendar-views.xul, which itself is an overlay. Please make sure that anything that overlays the view-deck (i.e the views themselves) overlay calendar-views.xul directly and not the window.

r=philipp with comments considered.
(Assignee)

Comment 11

9 years ago
addressed the comments of philipp and pushed patch v. #5 to comm-central:
http://hg.mozilla.org/comm-central/rev/d225e9ddbea1

I think I will need another patch to complete this issue.
(Assignee)

Updated

9 years ago
Attachment #356732 - Attachment description: patch v. #5 → [checked in] patch v. #5

Comment 12

9 years ago
I checked patch v5, the cop attribute in the navigation bar is gone now.

Comment 13

9 years ago
Oh, I mean 'crop attribute'

Comment 14

9 years ago
Created attachment 357338 [details]
navbar text
(Assignee)

Comment 15

9 years ago
Created attachment 357671 [details] [diff] [review]
[checked in] patch v. #6

The patch attached is quite big and I apologize for that. Within it I removed the calendar-decorated-base binding which involved quite a lot of consolidation work. When i then tested the patch afterwards I found there were several weird things in relation with the preference observation that I then fixed and consolidated, too.

What remains to be done?
- The bindings "calendar-decorated-month-view" and "calendar-decorated-multiday-base-view" are not needed anymore and can be merged into the bindings "calendar-multiday-view" and "calendar-month-view".
- the tabbox element, that was to replace the "view-deck" element is not yet implemented
- The use of the view properties "[start|end]Date, query[start|end], range[start|end], [start|end]day is confusing and should be overworked. Some of them can certainly be removed.
- I find, that all the "calendar-decorated-...-view" bindings should be renamed to "calendar-[day|week|multi-week|month]-view" as in fact we don't have any decorated views in their original meaning anymore. As the bindings are not very large anymore we can move them to a new single file base/content/calendar-views.xml

I did not want to make my patch even bigger than it is already now so I would like to deal with these leave these issues for another bug that I will set up then if it's Ok. I consequently suggest to close this bug after this patch has been pushed to comm-central.
Attachment #357671 - Flags: review?(philipp)
Comment on attachment 357671 [details] [diff] [review]
[checked in] patch v. #6


>+      <field name="mPrefObserver"><![CDATA[
>+      ({ calView: this,
>+         observe: function calViewPrefChange(subj, topic, pref) {
>+             this.calView.handlePreference(subj, topic, pref);
>+             return;
>+         }
>+      })
Why does this need to be an extra field? Can't we just add an observe method to the views directly?

>+      <property name="rotated">
>+        <getter><![CDATA[
>+            return (this.orient == 'horizontal');
>+        ]]></getter>
>+        <setter><![CDATA[
>+            this.orient = (val ? 'horizontal' : 'vertical');
>+            return val;
>+        ]]></setter>
>+      </property>
You could compact these:

<property onget="return (this.orient == 'horizontal')"
          onset="return (this.orient = (val ? 'horizontal' : 'vertical'))"/>


>+      <!-- A preference handler which is called by the preference observer.
>+           Can be overriden in derived bindings. -->
Typo: overridden

>+               case "calendar.alarms.indicator.show":
>+                   break;
Please add a comment here, á la "Break here to ensure the view is refreshed".

>     <binding id="calendar-decorated-multiday-base-view"
>+             extends="chrome://calendar/content/calendar-multiday-view.xml#calendar-multiday-view">
The decorated view now extends the multiday view? This seems kind of strange to me. Do we even need the extra layer of decoration now?



>   <binding id="calendar-month-view" extends="chrome://calendar/content/calendar-base-view.xml#calendar-base-view">
>+    <content style="overflow: auto;" flex="1" xbl:inherits="context,item-context">
Can't we move this style to a css file? Also, did you test if xbl:inherits also works on the content node itself?



The rest looks good, r=philipp with comments considered.
Good work!
Attachment #357671 - Flags: review?(philipp) → review+
(Assignee)

Comment 17

9 years ago
patch pushed to comm-central
http://hg.mozilla.org/comm-central/rev/f384b7b81452

Some remarks about comment #16:

>The decorated view now extends the multiday view? This seems kind of strange to
>me. Do we even need the extra layer of decoration now?

See my comment #15 where I said I would like to do this in a follow-up bug.

>Can't we move this style to a css file? Also, did you test if xbl:inherits also
>works on the content node itself?

I was aware of this when I implemented it but I left it as it is because
1) I already suggested to remove the decorated bindings.
2) Is there a way to set up a css selector bound to a "calendar-month-view" that is now the parent class of two other classes?
I will change it in a follow-up issue.

>Why does this need to be an extra field? Can't we just add an observe method to
>the views directly?
I somehow did not manage to set up an implementation that was easier than mine.

I would like to set this bug to "fixed", so that Andreas, who will be on holiday from Wednesday on, can test this for regressions.
I will set up a follow up issue then
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Berend, out of interest, does this massive consolidation also have a positive/negative performance impact?
(Assignee)

Comment 19

9 years ago
This issue resolves quite some no longer necessary indirections, so the performance impact should be positive. I can see no negative impact, yet have not measured this. But my primary intention for this issue was to make the code easier to maintain.

Comment 20

9 years ago
I checked patch v. #6 -> outcome:

- bug 'comment #12' isn't fixed.

- 'View/Current view/workweek days only' is defect.
(Assignee)

Comment 21

9 years ago
Created attachment 358366 [details] [diff] [review]
patch #7

This patch fixes the issue mentioned in comment #12 with the missing crop attribute and also the one in comment #20.  Bug 474601 -  "Start the week on " option doesn't affect Month and Multiweek Views should be fixed with this patch, too. Was just a stupid orthographical mistake
Attachment #358366 - Flags: review?(philipp)
(Assignee)

Updated

9 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #357671 - Attachment description: patch v. #6 → [checked in] patch v. #6
Attachment #358366 - Flags: review?(philipp) → review+
Comment on attachment 358366 [details] [diff] [review]
patch #7

r=philipp
(Assignee)

Comment 23

9 years ago
patch pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/32974272c46e

issue is fixed with respect to comment #15 and comment #16 for which I will file follow - up bugs.
As far as I can see 
Bug 474601 -  Start the week on option doesn't affect Month and Multiweek Views
and
Bug 473547 -  'Start the week on' functionality is broken in multiweek/month view

are fixed with is patch, too.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED

Comment 24

9 years ago
Checked in lightning and sunbird build 20090125 -> VERIFIED.
Status: RESOLVED → VERIFIED
These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.