Closed Bug 327780 Opened 18 years ago Closed 16 years ago

Need to sort out what sort of toolbar UI to offer for Lightning

Categories

(Calendar :: Lightning Only, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: jminta, Assigned: berend.cornelius09)

References

Details

(Whiteboard: [roadmap 0.8])

Attachments

(3 files, 3 obsolete files)

Followup to Comment #10 Bug 293192.  We need to figure out which buttons we want to add to the normal toolbar list.  We also need to figure out which additional toolbar buttons we may want to add when displaying the calendar view.
Summary: Need to sort out what sort what sort of toolbar UI to offer for Lightning → Need to sort out what sort of toolbar UI to offer for Lightning
As a starting reference point: My current Mail Toolbar Buttons (icons & text) are:

[Get Mail] [Compact] [Write] [Addr.Book] [Reply] [Repl.All] [Fwd] [Next] [Mark] [Delete] [Junk] [Print] [Stop] [Search: subject or sender]

These use up the complete witdh of my (nearly maximized) window (1024x768).

How do you want to enable switching between mail & calendar (and other modules?) Add (yet) another (toggle) button: [Mail/Calendar]?

[Mail/Calendar] [Get Mail] [Compact] [Write] [Addr.Book] [Reply] ...

Would then, when in Mail, clicking the [Mail/Calendar] button, to go to Calendar, yield these buttons:

[Mail/Calendar] [New Event] [New Task] [Edit] [Delete] [GoTo Today] ...?

A better way might be to do it like Outlook: Have a bunch of icons along the left side to switch between Mail/Calendar/whatever, and completely replace the top toolbar button when switching. But that is probably (at least related to) another bug (bug 327783).
Blocks: 315962
I think this should be discussed for Lightning 0.7 because we now have a separate toolbar for Lightning.
Flags: blocking-calendar0.7?
Christian, with respect to the current nightlies, do you consider the toolbars to be complete. In other words, do you consider this issue to be fixed?
(In reply to comment #3)
> Christian, with respect to the current nightlies, do you consider the toolbars
> to be complete. In other words, do you consider this issue to be fixed?
> 

No, they are not complete. Please find the draft for Lightning 1.0 here:
http://wiki.mozilla.org/Calendar:Thunderbird_Toolbar_Integration

Given our tight schedule and the fact that the Calendar devs already have too
much on their plate for 0.7, this is unlikely to make it for 0.7 unless someone
steps up and finishes this. In addition, with the patch for bug 371916 this is already 95% finished.
Flags: blocking-calendar0.7? → blocking-calendar0.7-
Is this a duplicate of bug 392584?
(In reply to comment #6)
> Is this a duplicate of bug 392584?
> 

I don't think so. Toolbar != Menu.
Let's finalize this for 0.8.
Flags: wanted-calendar0.8+
Whiteboard: [roadmap 0.8]
Target Milestone: --- → 0.8
Flags: blocking-calendar0.7-
So i tried to tackle this today, but this is harder to finish than I thought. These are the open issues as far as I can see:

1. You can't add sub-items to the "Write" button in mail mode, because it is 
   just a simple button. So there is no <menupopup> to overlay into.

2. We currently can't distinguish between calendar mode and task mode as far as 
   I can see. We currently only set 'mode="calendar"' on buttons or menuitems 
   that we don't want to show in mail mode. What we need is something like 
   *  'mode="task"' for something to show up only in task mode 
   *  'mode="calendar"' for something to show up only in calendar mode
   *  'mode="calendar,task"' for something to show up in both modes

3. If I remember correctly we have still weird issues, when re-using buttons 
   from Thunderbird. I don't know a workaround for this currently.

I'd appreciate some comments on this.
For item 1., I know it won't help w/ 2.x, but it'd be good to file a bug against Tb 3.x to change the toolbar items to allow for this kind of change.  
(In reply to comment #9)
> We currently can't distinguish between calendar mode and task mode as far as
> I can see. We currently only set 'mode="calendar"' on buttons or menuitems
> that we don't want to show in mail mode.
We definitely *can* distinguish between different modes. The rule is that elements (menuitems, buttons, etc.) that belong to the mail mode don't need to specify a 'mode'-attribute. Elements that should belong to some other mode (either calendar or task) just specify an appropriate 'mode'-attribute. This will be picked up by the mode switching logic and handled appropriately (as far as there are no bugs, but that's how it's intended to be used).
> The rule is that elements (menuitems, buttons, etc.) that belong to the mail 
> mode don't need to specify a 'mode'-attribute. Elements that should belong to 
> some other mode (either calendar or task) just specify an appropriate 
> 'mode'-attribute. This will be picked up by the mode switching logic and 
> handled appropriately

Mickey, if this is true, then why is the toolbar in task mode currently identical to the toolbar in calendar mode, even though only 'mode="calendar"' is set on all the toolbarbuttons?

The only code piece relating to this, that I could find is http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/calendar/lightning/content/messenger-overlay-sidebar.js&rev=1.21.2.69&mark=720-723#719

This only talks about menuitems, but I guess it's the same for the toolbarbuttons.
> Mickey, if this is true, then why is the toolbar in task mode currently
> identical to the toolbar in calendar mode, even though only 'mode="calendar"'
> is set on all the toolbarbuttons?
Probably because Berend didn't take other modes than 'calendar' into account when he wrote the switching logic for the menues/toolbars. I didn't claim that it works out of the box, I just tried to explain how I thought this attribute is intended to work. Certainly there are still bugs with handling this attribute, but you already found the location that needs to be enhanced in order to allow for the 'task' value of the 'mode' attribute.
Assignee: nobody → Berend.Cornelius
Flags: blocking-calendar0.8?
Flags: blocking-calendar0.8? → blocking-calendar0.8+
This first patch contains all resource strings that we need. Some notes about the patch:I made one slight modification to Christians proposal in comment #4: "Complete Tasks" is now "Completed Tasks" conforming to what we decided in bug 408798.
Attachment #297783 - Flags: ui-review?(christian.jansen)
Attachment #297783 - Flags: review?(michael.buettner)
Comment on attachment 297783 [details] [diff] [review]
[checked in] first patch with resources

r=mickey.
Attachment #297783 - Flags: review?(michael.buettner) → review+
Comment on attachment 297783 [details] [diff] [review]
[checked in] first patch with resources

r=christian
Attachment #297783 - Flags: ui-review?(christian.jansen) → ui-review+
patch 'first patch with resources' checked in on trunk and MOZILLA_1_8_BRANCH
Attachment #297783 - Attachment description: first patch with resources → [checked in] first patch with resources
Status: NEW → ASSIGNED
'Find events' button should be removed from task mode because it's only used in calendar mode
Omar, isn't the 'Find events'-button a feature?
the zip-file attached contains the new images and can be extracted from the calendar folder. The images are optimized by pngcrush.
Attached patch patch v. 1 (obsolete) — Splinter Review
The patch attached follows the guideline that Christian created in
http://spreadsheets.google.com/pub?key=plCAueWeXt4jIoK2sb3c0QQ&output=html

Andreas tested the patch already and found no issues. When I tested the final version I noticed that the bottom lines of the toolbar-menu-buttons "Category" and "Priority" are not well aligned under Windows. I could not yet figure out why, but I hope this is minor enough to fix it in a later step. I will keep on track.
The 'remove Category' and 'New Category' menuitem is not implemented. Christian wanted to have the preference dialog with the category tabpage popup, but it seemed to me that this is not possible although thunderbird provides a function for it.
Attachment #300880 - Flags: ui-review?(christian.jansen)
Attachment #300880 - Flags: review?(philipp)
The attached winstripe toolbar images changes many icons from the currently used calendar symbol (white calendar with orange header) to a different one that is supposed to match the Thunderbird default theme.

Is it planned to update all other icons and images too? For example the Lightning icon, the preference dialog icon, the alarm dialog icon, the remaining toolbar icons etc.?

Or is it planned to ship 0.8 with a mixed style winstripe theme?
I am sorry, I should have mentioned that Christian overworked the existing icons when he added the new ones.
Berend, how does this patch affect Sunbird? As far as I can see all the new task-related buttons will be enabled in Sunbird even though we don't have a task mode or task view in Sunbird?

Is this really the intended design? Wouldn't it be better to move the new buttons to lightning-toolbar.inc?
Flags: wanted-calendar0.8+
In reply to comment #25:
Of course you are right: The buttons should not be enabled in Sunbird. I will move them to lightning-toolbar.inc. Yet in the long run I can see no reason why the toolbar buttons should not be used as they are in Sunbird, which is why I put them to calendar-toolbar.inc. Once somebody will implement the task-mode for Sunbird he will be grateful when the task-related toolbar buttons are at their proper place ready to be hooked in. I think we all have not always implemented our modules from this point of view but IMO this is the way to go. Implementing functionality only for one application at first and than consolidate this in order to use it for the other application has generally costed us quite some effort in the past.
Berend, I totally agree with you. However I don't think that enabling the buttons in Sunbird once we have a task mode there as well will be such a hassle since most of the logic and styles will already be present in files used by both apps.
No, don't get me wrong. I also don't think this is a big hassle. Yet one hassle I just noticed is that I consequently should also shift all my css selectors  calendar-toolbar.css to lightning.css. This would mean that I would have to copy the logic of all commonly used buttons (New Event button, new Task button Delete button...) to two different places. Therefor I suggest to move the toolbar-buttons to lightning-toolbar.inc but leave the style definitions in calendar-toolbar.css.
Yes, please leave the style definitions in calendar-toolbar.css. They don't hurt anybody there. The case is different for calendar-toolbar.inc where all toolbarbuttons will automatically be shown in Lightning and Sunbird.
(In reply to comment #29)
> Yes, please leave the style definitions in calendar-toolbar.css. They don't
> hurt anybody there. 
That's not entirely true.  There's a startup/repaint time cost to every css rule included.

(In reply to comment #30)
>> Yes, please leave the style definitions in calendar-toolbar.css. They don't
>> hurt anybody there. 
> That's not entirely true.  There's a startup/repaint time cost to every css
> rule included.

I know, but the effects are negligible in the grand scheme of things.
Attached patch patch v. 2 (obsolete) — Splinter Review
I overworked the patch by shifting the toolbar-buttons to lightning-toolbar.xul as Simon proposed - thank you for the hint. Yet thinking more about this solution I find it still suboptimal: Basically the user has the opportunity to display tasks in the calendar View - also under Sunbird. Consequently he should also be offered the opportunity to customize the calendar-toolbar with the buttons "Mark Complete", "Category" and "Priority" that are bound to the selected tasks. All we need is to define an API method "getselectedTasks" that is called depending on the view and mode. By default these buttons should not be inserted into the calendar-toolbar. However this is something for the time after 0.8 delivery...

I fixed the alignment problem that occured under Windows, too.
Attachment #300880 - Attachment is obsolete: true
Attachment #301477 - Flags: review?
Attachment #300880 - Flags: ui-review?(christian.jansen)
Attachment #300880 - Flags: review?(philipp)
Attachment #301477 - Flags: ui-review?(christian.jansen)
Attachment #301477 - Flags: review?(philipp)
Attachment #301477 - Flags: review?
Attached patch patch v. 3 (obsolete) — Splinter Review
Christian informed me that the patch does not apply anymore
Attachment #301477 - Attachment is obsolete: true
Attachment #301477 - Flags: ui-review?(christian.jansen)
Attachment #301477 - Flags: review?(philipp)
Blocks: 389150
Comment on attachment 301657 [details] [diff] [review]
patch v. 3

r=christian. Works great on mac.
Attachment #301657 - Flags: ui-review+
Comment on attachment 301657 [details] [diff] [review]
patch v. 3


>+function uncheckChildNodes(aEvent) {
>+    var liveList = aEvent.target.getElementsByAttribute("checked", "true");
>+    // Delete in reverse order.  Moz1.8+ getElementsByAttribute list is
>+    // 'live', so when attribute is deleted the indexes of later elements
>+    // change, but Moz1.7- is not.  Reversed order works with both.
>+    for (var i = liveList.length - 1; i >= 0; i-- ) {
>+        var commandName = liveList.item(i).getAttribute("command");
>+        var command = document.getElementById(commandName);
>+        if (command) {
>+            command.setAttribute("checked", "false");
>+        }
>+    }
>+}
We dont support moz1.7- so go ahead and use the moz1.8 way. At least drop the comment. This seems like a good candidate for calendar-ui-utils.js ?



>+function changeMenuByPropertyName(aEvent, aPropertyName) {
A comment describing what this function would be good. In fact, it would be a good idea for all new functions. I have been having problems understanding all the toolbar/menu functions that are new, mostly because they are missing a comment. If this takes too much time before the rc, please file a bug to add comments and take care after 0.8


>+                            onselect="document.commandDispatcher.updateCommands('calendar_commands')"
>+                            onblur="document.commandDispatcher.updateCommands('calendar_commands')"/>
I assume this is needed, but we should avoid updating commands if its not. If you are not sure, please check if it also works without this update onblur.

>+function appendCategoryItems(aItem, aCategoryMenuList, aCommand) {
>+    var categoriesString = getLocalizedPref("calendar.categories.names", "");
>+
>+    // If no categories are configured load a default set from properties file
>+    if (!categoriesString || categoriesString == "") {
>+        categoriesString = calGetString("categories", "categories");
>+        setLocalizedPref("calendar.categories.names", categoriesString);
>+    }
>+
>+    var categoriesList = categoriesString.split(",");
gekachecka recently added some great functions to retrieve an array of categories, which also takes care of categories that contain (escaped) commas. Please use those functions in this function.


>+function clonePopupMenu(aMenuPopupId, aNewPopupId, aNewIdPrefix) {
Great comments on this function! It could be generalized to also work on non-menupopups, but we can leave this to happen when needed.

> function ltnInitializeMenus(){
>     copyPopupMenus();
>     ltnRemoveMailOnlyItems(calendarpopuplist, "calendar");
>     ltnRemoveMailOnlyItems(taskpopuplist, "task");
>-    document.getElementById("calendar-toolbar").setAttribute("collapsed", "true")
>     var modeToolbar = document.getElementById("mode-toolbar");
>     var visible = !modeToolbar.hasAttribute("collapsed");
>     document.getElementById("modeBroadcaster").setAttribute("checked", visible);
>-    }
>+    document.getElementById("calendar-toolbar").setAttribute("collapsed", "true");
>+    document.getElementById("task-toolbar").setAttribute("collapsed", "true");
>+    var progressToolbarPopupMenu = clonePopupMenu("calendar.context.progress-menu", "toolbar-progress-menu", "tb-");
>+    document.getElementById("task-progress-button").appendChild(progressToolbarPopupMenu);
>+    var priorityToolbarPopupMenu = clonePopupMenu("calendar.context.priority-menu", "toolbar-priority-menu", "tb-");
>+    document.getElementById("task-priority-button").appendChild(priorityToolbarPopupMenu);
>+}
While you are at it, you could make sure the elements are only collapsed if we are in mail mode. It may later on be possible to start in calendar mode, then we would need to take care at latest, but before we forget you might want to do it here.



r+ since most comments are minor or can be taken care in a different bug.
Attachment #301657 - Flags: review+
(In reply to comment #35)
>> +function changeMenuByPropertyName(aEvent, aPropertyName) {
> A comment describing what this function would be good. In fact, it would be a
> good idea for all new functions. I have been having problems understanding all
> the toolbar/menu functions that are new, mostly because they are missing a
> comment. If this takes too much time before the rc, please file a bug to add
> comments and take care after 0.8

Berend, I would really appreciate it, if you would add the comments *before* checking in this patch. All this followup stuff tends to get overlooked and forgotten after some time. And it is still a few weeks until we ship 0.8
Attached patch final patchSplinter Review
final patch that I just checked in
Attachment #301657 - Attachment is obsolete: true
patch checked in on trunk and MOZILLA_1_8_BRANCH

=> FIXED

I addressed all issues mentioned by Philipp and Simon -> Thanks a lot.

>>+                            onblur="document.commandDispatcher.updateCommands('calendar_commands')"/>
>I assume this is needed, but we should avoid updating commands if its not. If
>you are not sure, please check if it also works without this update onblur.

I removed this line. Now the taskmode behaves compliant to the mail-mode where all mail-related toolbar-buttons are enabled even when the messenger-tree-pane does not have the focus. Before I thought the task-related buttons should not be enabled when the focus is not on the task tree.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Verified with Lt 2008020902
Status: RESOLVED → VERIFIED
This checkin seems to have caused major regressions in the task/event dialog, for example Bug 416525 and Bug 416526.
Depends on: 416525, 416526
The regressions seem to be caused by the removal of the function setItemProperty() from the dialog. What was the reason for this?
It seems to me this function was moved to calUtils?
The change in setItemProperty() might also have regressed Bug 416584, see Bug 416584 Comment #5. I assume that for new created events/tasks the calendar is not yet set on the event/task. Previously the calendar was taken from the corresponding dropdown menu in the dialog.
Depends on: 416584
Berend, your patch removed parts of gekachecka's patch from bug 321010, which touched sun-calendar-event-dialog.js (more precisely: building the categories list). You should fix this because I already found a minor regression. We should be more careful when updating patches to the most recent version of a file. In this case the differences have been significant enough to spot them imo.
This was also part of the review comment, please take care.
Depends on: 418251
Installed Feb 19 lightning xpi on Vista 32bit, click Task Mode icon, and the new Task bar appears 3 times. 
Installed Feb 19 lightning xpi on Vista 32bit, click Task Mode icon, and the new Task bar appears 3 times. 
You need to log in before you can comment on or make changes to this bug.