422640 - enable View menu-items

Status: VERIFIED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Berend Cornelius]

The menuitems "Calendar List" "Minimonth" and "Sort Tasks" in the View menu have to be enabled and activated. Besides that the label "Sort Tasks" which is to toggle the filter list in task mode has to be renamed. The "minimonth" menuitem as well as the "Calendar list" menuitem have to be available in calendar mode and task mode.

Comment 1

[Berend Cornelius]

Attachment: patch v. #1

I tried to resolve the problem in common way: In this bug we are dealing with three panes that are supposed to appear depending on the application mode and depending on the user settings in the menuitems. This problem appears for other panes too and therefor I implemented a xul:box deduction that does this automatically by listening to a global broadcaster that has a defined attribute mode and where a command or other control may be attached to. Therefor I hope that this patch will become an importannt prerequisite for other issues, like implementing the taskmode in Sunbird and make the todaypane available for other modes (see Bug 389150: Display Today Pane in Calendar mode too).
I changed one menu label to "Filter Tasks". Talked to Christian beforehand and we decided that "Filter Task Pane" is too technical. But Christian did not like "Filter Tasks" too. So I leave this open for discussion.

Comment 2

[Christian Jansen]

I have to admit that I do not have a better idea for the menu label. So, we should go with "Filter Tasks". One small nit. Please change the order of the menu items from: Mini Month, Calendar List, Sort Tasks to Mini Month, Sort Tasks, Calendar List. ui+ for the patch.

Comment 3

[Christian Jansen]

Comment on attachment 309103 [details] [diff] [review]
patch v. #1

see comments above

Comment 4

[Berend Cornelius]

In reply to comment #2: I think the order of the menuitems should reflect the order of the panes in the sidebar, that's why I put the "Filter Task" - menuitem at position #2. I found the other order disturbing.

Comment 5

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

Comment on attachment 309103 [details] [diff] [review]
patch v. #1

>+#expand    skin/classic/calendar/calendar-widgets.css         (themes/__THEME__/calendar-widgets.css)
If this file is used for bindings, no need to make it theme-dependant. We may even want to put it into content, similar to calendar-view-bindings.css


>+   - The Initial Developer of the Original Code is
>+   - Sun Microsystems.
Please put in one line

>+   - Portions created by the Initial Developer are Copyright (C) 2007
Did you really create this in 2007?

>+    - The attribute "control" points to a control either a "command", "checkbox" or a "treenode-checkbox" or other
This might conflict with the xul "control" attribute, used for accessibility?

>+      additionally verified if the xul-element denoted by "control" is checked or not. During runtime attributes are
>+      created following the notation "collapsedin" + mode + "mode" and made persistent. -->
We might want to change this to to use attributes more like collapsedInModes="task mail calendar"? its still easily possible to select this using the css =~ selector.
>+        if (this.hasAttribute("broadcaster")) {
>+            this.mBroadcaster = document.getElementById(this.getAttribute("broadcaster"));
>+        if (this.hasAttribute("control")) {
>+            this.mControl = document.getElementById(this.getAttribute("control"));

What if the broadcaster or control attributes change? You should either override setAttribute or not save the broadcaster element.


>+            }
>+            else if (aCheckModeCollapsed === true) {
} else if (...) { here and elsewhere


>+            if (aMode) {
>+                var lMode = aMode;
>+            }
>+            else {
>+                var lMode = currentMode;
>+            }
var lMode = aMode || currentMode;

>+                lModes = modeString.split(",");
Lets be more standard-compliant and and use |modeString.split(/[, ]/)|.


In general, you might want to prefix quasi-non-public methods with "_".

r=philipp

Comment 6

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

Sorry, I missed half of the patch due to dialup not loading all of it.


>+function containsElement(aArray, aCompString) {
>+    for (var i = 0; i < aArray.length; i++) {
>+        if (aArray[i] && aArray[i] == aCompString) {
>+            return true;
>+        }
>+    }
>+    return false;
>+}
Am I missing something or is this just  return (aArray.indexOf(aCompString) > -1) ? If so, please just use indexOf everywhere containsElement is used.


Please check all copyright dates, I also found something from 2005?


I take back the comment about expanding calendar-widgets.css, I see it contains also additional styles. We have been strictly splitting up -moz-binding rules and normal style rules, is this something we want to continue?

>+    document.getElementById("modeBroadcaster").setAttribute("mode", "mail");
Please make this mode-dependant, in case we start in a different mode. (i.e use gCurrentMode


r=philipp, once again.

Comment 7

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

The "make this mode-dependant" comment regards the line in ltnInitializeMenu().

Come to think of it, is this really something that has to do with initializing the menus?

Comment 8

[Berend Cornelius]

I applied Philipp's objected issues.
Some notes:

>I take back the comment about expanding calendar-widgets.css, I see it contains
>also additional styles. We have been strictly splitting up -moz-binding rules
>and normal style rules, is this something we want to continue?

I separated the the binding rules from the rest by introducing a new file "calendar-widget-bindings". As we discussed I added the following folders:
calendar/base/content/widgets/
calendar/base/themes/winstripe/widgets
calendar/base/themes/pinstripe/widgets
(calendar/base/thems/common/widgets is bound to be added, too)
"calendar-widget-bindings" has been added to calendar/base/content/widgets
"calendar-widgets.xml" has been added to calendar/base/content/widgets , too
The other "calendar-widgets.css" files have been added to calendar/base/themes/winstripe/widgets and calendar/base/themes/pinstripe/widgets

These new folders help us to keep commonly needed bindings separately which would otherwise blow up calendar/base/content

>>+    - The attribute "control" points to a control either a "command", >"checkbox" or a "treenode-checkbox" or other
>This might conflict with the xul "control" attribute, used for accessibility?

I replaced the attribute by "refcontrol" although I was not quite sure if it was really necessary. There is also a "control" attribute at xul:label for a similar purpose.

>What if the broadcaster or control attributes change? You should either
>override setAttribute or not save the broadcaster element.

I overrode "setAttribute" in "modebox" binding.

>We might want to change this to to use attributes more like
>collapsedInModes="task mail calendar"?

Very smart idea. I implemented "collapsedinmodes".

>Please make this mode-dependant, in case we start in a different mode. (i.e use
>gCurrentMode
I moved the attribute to the XUL file. I don't think that gCurrentMode we will keep 'gCurrentMode' very long.

Thank you for your valuable input!

Comment 9

[Berend Cornelius]

Attachment: patch v. #2

patch with the objected issues of philipp. I will wait one day until I check it in to give Andreas an opportunity to test it.

Comment 10

[Berend Cornelius]

Attachment: [checked in] patch v. #3

Andreas found two minor issues that I worked in...

Comment 11

[Berend Cornelius]

patch checked in on trunk and MOZILLA_1_8_BRANCH
-> FIXED

I hope that my implemented "modebox" bindings  will be made use of when implementing the task-mode in Sunbird or for the implementation of the today-pane for the calendar-mode

Comment 12

[Berend Cornelius]

Attachment: patch v. #3

When Andreas tested the latest nightlies that contained the patch #2 of this issue he had - under certain profiles - problems to collapse/uncollapse the filtergroup -radio buttons in task mode. I could not reproduce this problem at all but by looking into one of his localstore.rdf files I found out that the radiogroup "task-tree-filter" still had the "collapsed" attribute persistent although I changed the "persists" attribute of the radiogroup within my patch. Obviously Mozilla is not smart enough to realize that this setting has become obsolete.
I guess the only way out is to rename this widget and also the widget "calendar-list-tree" so that these settings in the localstore.rdf are being ignored. I realize that by doing so I keep some invalid entries in the localstore.rdf.
This patch has been tested through by Andreas.

Comment 13

[Berend Cornelius]

Attachment: patch v. #3

When Andreas tested the latest nightlies that contained the patch #2 of this issue he had - under certain profiles - problems to collapse/uncollapse the filtergroup -radio buttons in task mode. I could not reproduce this problem at all but by looking into one of his localstore.rdf files I found out that the radiogroup "task-tree-filter" still had the "collapsed" attribute persistent although I changed the "persists" attribute of the radiogroup within my patch. Obviously Mozilla is not smart enough to realize that this setting has become obsolete.
I guess the only way out is to rename this widget and also the widget "calendar-list-tree" so that these settings in the localstore.rdf are being ignored. I realize that by doing so I keep some invalid entries in the localstore.rdf.
This patch has been tested through by Andreas.

Comment 14

[Berend Cornelius]

Attachment: patch v. #4

When Andreas tested the latest nightlies that contained the patch #2 of this issue he had - under certain profiles - problems to collapse/uncollapse the filtergroup -radio buttons in task mode. I could not reproduce this problem at all but by looking into one of his localstore.rdf files I found out that the radiogroup "task-tree-filter" still had the "collapsed" attribute persistent although I changed the "persists" attribute of the radiogroup within my patch. Obviously Mozilla is not smart enough to realize that this setting has become obsolete.
I guess the only way out is to rename this widget and also the widget "calendar-list-tree" so that these settings in the localstore.rdf are being ignored. I realize that by doing so I keep some invalid entries in the localstore.rdf.
This patch has been tested through by Andreas.

Comment 15

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

Comment on attachment 314565 [details] [diff] [review]
patch v. #4

Looks good, r=philipp

Comment 16

[Berend Cornelius]

checked in last patch under trunk and MOZILLA_1_8_BRANCH
-> FIXED

Comment 17

[Omar Bajraszewski]

Verified with Lt 2006041006

Comment 18

[Simon Paquet [:sipaq]]

I noticed a rule ".task-tree-subpane only" in Pinstripe's lightning.css, which is not available in Winstripe? Any reason for that or pure oversight?

In general, your patch only touched Pinstripe not Winstripe. I can't find the reason for that here in the bug.

Comment 19

[Berend Cornelius]

As we clearified already this rule was only meant for pinstripe to consider the platform-specific indentations within tree-widgets.