enable View menu-items

VERIFIED FIXED in 0.9

Status

Calendar
Calendar Views
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: Berend Cornelius, Assigned: Berend Cornelius)

Tracking

unspecified

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
Created attachment 309103 [details] [diff] [review]
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.
Assignee: nobody → Berend.Cornelius
Status: NEW → ASSIGNED
Attachment #309103 - Flags: ui-review?(christian.jansen)
Attachment #309103 - Flags: review?(philipp)

Comment 2

10 years ago
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

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

see comments above
Attachment #309103 - Flags: ui-review?(christian.jansen) → ui-review+
(Assignee)

Comment 4

10 years ago
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.
(Assignee)

Updated

10 years ago
Blocks: 389150
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
Attachment #309103 - Flags: review?(philipp) → review+
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.
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?
(Assignee)

Comment 8

10 years ago
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!
(Assignee)

Comment 9

10 years ago
Created attachment 314114 [details] [diff] [review]
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.
Attachment #309103 - Attachment is obsolete: true
(Assignee)

Comment 10

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

Andreas found two minor issues that I worked in...
Attachment #314114 - Attachment is obsolete: true
(Assignee)

Comment 11

10 years ago
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
(Assignee)

Comment 12

10 years ago
Created attachment 314563 [details] [diff] [review]
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.
Attachment #314563 - Flags: review?
(Assignee)

Comment 13

10 years ago
Created attachment 314564 [details] [diff] [review]
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.
Attachment #314564 - Flags: review?
(Assignee)

Comment 14

10 years ago
Created attachment 314565 [details] [diff] [review]
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.
Attachment #314565 - Flags: review?(philipp)
Attachment #314563 - Attachment is obsolete: true
Attachment #314563 - Flags: review?
Attachment #314564 - Attachment is obsolete: true
Attachment #314564 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #314565 - Attachment description: patch v. #3 → patch v. #4
Attachment #314305 - Attachment description: patch v. #3 → [checked in] patch v. #3
Comment on attachment 314565 [details] [diff] [review]
patch v. #4

Looks good, r=philipp
Attachment #314565 - Flags: review?(philipp) → review+
(Assignee)

Comment 16

10 years ago
checked in last patch under trunk and MOZILLA_1_8_BRANCH
-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 17

10 years ago
Verified with Lt 2006041006
Status: RESOLVED → VERIFIED
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.
(Assignee)

Comment 19

10 years ago
As we clearified already this rule was only meant for pinstripe to consider the platform-specific indentations within tree-widgets.
You need to log in before you can comment on or make changes to this bug.