Event dialog: Customize Toolbar dialog should offer more buttons for functionality

RESOLVED FIXED in 3.0

Status

defect
--
minor
RESOLVED FIXED
13 years ago
4 years ago

People

(Reporter: ulf.stroehler, Assigned: bv1578)

Tracking

Sunbird 0.7

Details

(Whiteboard: [good first bug][mentor=Fallen][lang=js])

Attachments

(4 attachments, 3 obsolete attachments)

STR (steps To Reproduce)

install lightning-wcap.xpi

1. open the Event dialog "File" -> "New" -> "Event"
2. select "View" -> "Toolbars" -> "Customize"

Finding

"Customize" dialog is poor

Expectation

"Customize" dialog should offer important functionality from the menus as e.g. "Importance" setting, "Print", "Show Time as", "Timezone" switch, "Undo" and "Redo" buttons
Accepted
Status: NEW → ASSIGNED
OS: SunOS → All
Hardware: PC → All
Version: Trunk → unspecified
Summary: [Proto] Event dialog: "View" -> "Toolbars" -> "Customize" is poor → [Proto] Event dialog: Customize Toolbar dialog should offer more buttons for functionality
Duplicate of this bug: 392733
Assignee: christian.jansen → nobody
Status: ASSIGNED → NEW
Component: Lightning Only → General
QA Contact: lightning → general
Assignee: nobody → christian.jansen
Status: NEW → ASSIGNED
Component: General → Theme
Product: Calendar → Firefox
Target Milestone: --- → Firefox 3
Version: unspecified → Trunk
Oops, sorry, my bad, returning to the right component, awfully sorry about stomping your target milestone, please send hatemail to this address :(
Component: Theme → General
Product: Firefox → Calendar
Target Milestone: Firefox 3 → ---
Version: Trunk → Sunbird 0.7
Duplicate of this bug: 421669
Duplicate of this bug: 440463
Duplicate of this bug: 448822
Assignee: chris.j.bugzilla → nobody
Severity: normal → minor
Summary: [Proto] Event dialog: Customize Toolbar dialog should offer more buttons for functionality → Event dialog: Customize Toolbar dialog should offer more buttons for functionality
Status: ASSIGNED → NEW
Component: General → Dialogs
QA Contact: general → dialogs
Duplicate of this bug: 491559
This issue is still present in Lightning 1.0b1

Inside the 'Event' dialog, the 'Privacy' button already exists.

In the same manner, the 'Event' dialog should display by default at least following additional buttons :
-  Priority
-  Status

Each button should :

-  permit to easily toggle between authorized values :  See https://bugzilla.mozilla.org/show_bug.cgi?id=362948

-  have an associated field inside the dialog status bar :  See https://bugzilla.mozilla.org/show_bug.cgi?id=536536

Thank you in advance.
Duplicate of this bug: 557950
Duplicate of this bug: 636125
I think these would make sense:

Importance
Show Time as
Priority
Status

These might not (from above):
"Undo" and "Redo" -- We don't have an undo/redo queue for the event dialog
"Print" -- We don't have an option for printing a single event yet
"Timezone" -- For start or end date? or both? Could be ambiguous 


Anyway, this is a good first bug to solve since its just a matter of some (copy&paste) XUL and CSS. If you need icons, please ask :andreasn.
Whiteboard: [good first bug]
Whiteboard: [good first bug] → [good first bug][mentor=Fallen][lang=js]
Hi, I'm new here. I would like to work on this bug please.
Murali wants to work on this bug for the Software Development Module focussing on the Mozilla Platform taught in the National University of Singapore. I am available to mentor him.

@Murali: could you come with a suggestion of how do u want to fix this and then perhaps a patch. I believe you might need some icons. @Philipp?
Hi,

Basically I understand that the dialog box in question lacks many options. I am planning to add in some icons next to the existing few and link them up to back end code to perform appropriate tasks. Mostly I guess looking at the existing code will give me a concrete picture though I might need some help with the basics. Thanks a lot!
For icons I suggest you talk to andreasn. I have put Andreas on CC,  could you compile a list of icons you need and post them here?
Murali, any updates on this? Are you still working on this bug?
Sorry for the delay, got overwhelmed with school work. I'm definitely working on this bug. Just need some more time. Thanks!
I've talked with Murali he is not working anymore on the bug so I take the bug assignment.
Assignee: nobody → bv1578
Status: NEW → ASSIGNED
Posted patch patch - v1 (obsolete) — Splinter Review
This is a first patch, but, apart from errors, a lot of things need to be sorted out.

1. Icons.
I've made some rough icons (see next screenshots, from left to right: priority, status, free/busy, timezones) only for testing and maybe to apply the patch until we get better icons that I'm going to ask to Andreas Nilsson. The only correct icon should be the priority one (the exclamation mark) that already exist, with the same meaning, in other Lightning's image where I copied it.

2. Strings for tooltips.
Keeping in count that many buttons also allow to change the property's value other than showing the dropdown menu, what should be the correct sentences in the tooltips?

3. Using other strings.
Many strings used in the patch already exist in calendar-event-dialog.dtd file but they come from very different contexts, I reused many of them, should new strings be created instead?

4. Statusbar labels.
Since many toolbarbuttons directly change a property value, IMHO these priorities need to be displayed somehow in the dialog. I've added them in the status bar along with the others that already are there. I've also added a property's name label for each property, but with all the options enabled, the status bar might be a bit crowded (I've also seen bug 415477 about the priority icon and bug 536536 comment 2). 
On Win7 the look might be OK, but I think that on Linux it could be slightly worse.
What's your opinion?

5. Menuitem "checkbox".
The submenus of the menuitems "Priority", "Status" and "Show Time as" are "checkbox" instead of "radio" (one option at a time), unless I missed something, we should change them.
Attachment #726383 - Flags: review?(philipp)
Posted image rough icons
Icons for priority, status, free/busy time, timezones in order to allow to test the patch.
Comment on attachment 726383 [details] [diff] [review]
patch - v1

Review of attachment 726383 [details] [diff] [review]:
-----------------------------------------------------------------

r=philipp

::: calendar/base/content/dialogs/calendar-event-dialog.xul
@@ +640,5 @@
> +                           label="&event.menu.options.priority2.label;"
> +                           tooltiptext="&event.toolbar.priority.tooltip;"
> +                           oncommand="rotatePriority()">
> +                <menupopup id="event-priority-menupopup">
> +                    <menuitem id="event-priority-none-menuitem"

It seems this file uses 2-space indent, I think we should do the same for the new code.
Attachment #726383 - Flags: review?(philipp) → review+
Andreas, any news about the icons?
If you think the icons added in this patch are acceptable (at least until we get better ones) I could push the patch as it is. Just let me know.
Posted patch patch - v2 (v1 updated) (obsolete) — Splinter Review
I've updated the patch because the previous doesn't apply on trunk.
Attachment #726383 - Attachment is obsolete: true
Richard, may you take a look to the icons?

I've attached a zip file with all the .png images from the repository with the new icons added. You can see them in the attachment 726386 [details].
I think the icons I did have a poor quality maybe with the exception of the aero theme (that are a bit simple). 
- The icon for the priority should be right because I copied that one used in other context (the exclamation point);
- the icons for the free/busy time (the clock with the background black/white) could have a logic sense;
- the same for the timezone (the world image); 
- I have non idea at all about an icon for the status (I used a full circle in order to test the patch), the only thing I could think is a rubber-stamp like icon but I don't know how to realize it for Linux and Windows themes different from aero.

If you can do some work on them otherwise we have to ask to someone else for a complete restyling.
Flags: needinfo?(richard.marti)
Posted file Images
This is already looking good. I've attached a archive with slightly changed images. What do you think about them? I'm also no graphics designer.

I changed the icon order on the files for Aero and Linux small to be always Priority, Status, Free/Busy and Timezone.

I now use the same icons for Linux and XP. The icons for big and small where to different. I used a different clock to be sharper and made some small changes for the small status icon.

> - I have non idea at all about an icon for the status (I used a full circle
> in order to test the patch), the only thing I could think is a rubber-stamp
> like icon but I don't know how to realize it for Linux and Windows themes
> different from aero.

I think we should try this icons. If we see it (the status icon) doesn't work, we can search someone to style it.
Flags: needinfo?(richard.marti)
Posted patch patch - v3 (obsolete) — Splinter Review
(In reply to Richard Marti [:Paenglab] from comment #25)

> This is already looking good. I've attached a archive with slightly changed
> images. What do you think about them? I'm also no graphics designer.
>
I like your corrections and the clock for the free busy icon.
 
> I changed the icon order on the files for Aero and Linux small to be always
> Priority, Status, Free/Busy and Timezone.
>
Thanks for that, there was also an error in the Linux theme because I had inverted the -moz-image-region between Status and Free/Busy icon, with your changes everything should be fine now (I've fixed the same for the aero theme in the css file).

> I think we should try this icons. If we see it (the status icon) doesn't
> work, we can search someone to style it.

Philipp, if you agree I'm going to push this patch with the icons corrected by Richard.
Attachment #826487 - Attachment is obsolete: true
Comment on attachment 827270 [details] [diff] [review]
patch - v3

Review of attachment 827270 [details] [diff] [review]:
-----------------------------------------------------------------

A few comments on the patch I think you should consider before checking in:

::: calendar/base/content/dialogs/calendar-event-dialog.js
@@ +1601,5 @@
> +function rotatePriority() {
> +    let hasPriority = capSupported("priority");
> +    if (hasPriority) {
> +        if (gPriority == 0) {                          // not specified
> +            gPriority = 9;

You might want to put this in an else-branch, in case a third party app uses priorities other than 0-9.

@@ +1709,4 @@
>   */
>  function updateStatus() {
>      let found = false;
> +    let statusLabels = ["status-status-tentative-label",

let or const, your choice, but be consistent with rotateStatus.

::: calendar/base/content/dialogs/calendar-event-dialog.xul
@@ -451,2 @@
>                              type="checkbox"
> -                            persist="checked"

If you are unsetting persist, you will need to change the id, otherwise it will still be in users' localstore.rdf.

@@ +1228,5 @@
>                        align="center"
>                        flex="1"
>                        collapsed="true"
> +                      pack="start"
> +                      style="overflow: hidden;">

Maybe do this style in the css file?

::: calendar/locales/en-US/chrome/calendar/calendar-event-dialog.dtd
@@ +154,5 @@
> +<!ENTITY  event.toolbar.privacy.tooltip                   "Change Privacy">
> +<!ENTITY  event.toolbar.priority.tooltip                  "Change Priority">
> +<!ENTITY  event.toolbar.status.tooltip                    "Change Status">
> +<!ENTITY  event.toolbar.freebusy.tooltip                  "Change Free/Busy time">
> +<!ENTITY  event.toolbar.timezones.tooltip                 "Show Timezones">

If the tooltip is the same as the button text, is there even a need for a tooltip? I'd say no. If you disagree, I would reuse the strings from the button text, I don't think there is a case where translators will want to use different texts.
Attachment #827270 - Flags: review+
Posted patch patch - v4Splinter Review
(In reply to Philipp Kewisch [:Fallen] from comment #27)

> You might want to put this in an else-branch, in case a third party app uses
> priorities other than 0-9.

Since the code for the Priority menus treats all the priority values outside the interval [0-9] like "Not specified", I've implemented the same for the the rotatePriority function:

+function rotatePriority() {
+    let hasPriority = capSupported("priority");
+    if (hasPriority) {
+        if (gPriority <= 0 || gPriority > 9) {         // not specified
+            gPriority = 9;
...

> let or const, your choice, but be consistent with rotateStatus.

Yes, it had to be a const.

> If you are unsetting persist, you will need to change the id, otherwise it
> will still be in users' localstore.rdf.

Changed the id and all the references in comm-central.
 
> Maybe do this style in the css file?

Done.

> If the tooltip is the same as the button text, is there even a need for a
> tooltip? I'd say no. If you disagree, I would reuse the strings from the
> button text, I don't think there is a case where translators will want to
> use different texts.

Generally speaking, IMHO, the tooltips should always be present where the user can customize a toolbar with the option "Icons without text" (as I used to do, otherwise with many buttons the texts enlarge too much the window) and there where the icons don't have an obvious meaning.
As I said in comment #19, in this case most buttons not only show a dropdown menu but also have the function to change the property which they are referred to and for this reason the tooltip text should specify that ability, but the same sentence should not be used for the button text because of the room they would take in the dialog, hence they would need different strings, all this IMHO obviously.

Text button        Tooltip text
-------------      ------------------
Status             Change Status
Priority           Change Priority
Show Time as       Change Free/Busy time
Show Timezones     Show Timezones

The issue would arise for the timezone button, but keep also in count that the buttons already existing in the dialog all have the tolltip and many have the same text of the button like "Save and Close", "Invite Attendees", "Delete". Not using the tooltip only for a few buttons would be odd.
If you agree I would leave the tooltip even in the Timezone button for coherence with the other buttons, anyway I reused the label of the text button.


Personally I would use shorter text for the *text buttons* even if different from the corresponding menu item for room reasons e.g.
"Free/Busy"   instead of   "Show Time as"
"Timezones"   instead of   "Show Timezones"
but this another issue.
Attachment #827270 - Attachment is obsolete: true
Attachment #830230 - Flags: review?(philipp)
(In reply to Decathlon from comment #28)

> 
> Generally speaking, IMHO, the tooltips should always be present where the
> user can customize a toolbar with the option "Icons without text" (as I used
> to do, otherwise with many buttons the texts enlarge too much the window)
> and there where the icons don't have an obvious meaning.

Ah yes, true. I didn't think of that case.

> "Delete". Not using the tooltip only for a few buttons would be odd.
> If you agree I would leave the tooltip even in the Timezone button for
> coherence with the other buttons, anyway I reused the label of the text
> button.
I agree to both points :)


> 
> Personally I would use shorter text for the *text buttons* even if different
> from the corresponding menu item for room reasons e.g.
> "Free/Busy"   instead of   "Show Time as"
> "Timezones"   instead of   "Show Timezones"
> but this another issue.
Also fine with me, feel free to create a patch in a separate issue.
Comment on attachment 830230 [details] [diff] [review]
patch - v4

r=philipp
Attachment #830230 - Flags: review?(philipp) → review+
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/549a83dc09a2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.0
Is it on purpose that the status bar shows only "Time as: Busy" instead of "Show Time as: Busy"? It looks strange to me that way.
(In reply to Stefan Sitter from comment #32)

It's a way to mitigate, but not to eliminate, the problem of the room in the status bar.
As I said in comment 19 when all the options are assigned, and with the longer strings selected, the bar becomes full of informations and quite chaotic, in most cases the dialog has to be enlarged in order to read all. I didn't test on Linux, but the problem should be even worst since the standard characters are a bit larger than Windows.

If we want to display *all* the informations correctly in every case we should think to something different e.g. to enlarge the dialog according to the the informations displayed or using a different position than the status bar (another row e.g. above the Description area) and put the informations on two lines. Another thing to do should be to use a different icon for the priority, with a small size and, above all, fixed size (and maybe the same used in the task view and in the today pane).

I'm open to any solution.

Since we are talking about "room", what's your opinion about what mentioned in comment 28 about the text in the buttons:
> Personally I would use shorter text for the *text buttons* even if different
> from the corresponding menu item for room reasons e.g.
> "Free/Busy"   instead of   "Show Time as"
> "Timezones"   instead of   "Show Timezones"
> but this another issue.
Duplicate of this bug: 398129
You need to log in before you can comment on or make changes to this bug.