Closed Bug 380335 Opened 17 years ago Closed 17 years ago

Add picture to Task in MonthView (instead of *)

Categories

(Calendar :: Calendar Frontend, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: Taraman, Assigned: Taraman)

Details

Attachments

(4 files, 3 obsolete files)

In the MonthView in front of a Task there is a "*" which should be a picture instead.
Attached patch adding picture (obsolete) — Splinter Review
This could be a way to add pictures to the ToDo-items in MonthView
Attachment #264413 - Flags: ui-review?(christian.jansen)
Attachment #264413 - Flags: review?(daniel.boelzle)
Attached file Picture Binaries (obsolete) —
The two .png-files for the patch
Comment on attachment 264413 [details] [diff] [review]
adding picture

moving review for view changes over to mickey
Attachment #264413 - Flags: review?(daniel.boelzle) → review?(michael.buettner)
Comment on attachment 264413 [details] [diff] [review]
adding picture

Images are ok, but need to be redesigned when we update the icons for being more Thunderbird complient.
Attachment #264413 - Flags: ui-review?(christian.jansen) → ui-review+
I would prefer a much simpler icon. The only thing that makes the not-completed image say that it's about a todo is the very small green check in it. The rest is not really used.
I would propose maybe something like a yellow 'sickie' paper. Or else try to find a way to remove the clutter around the small green check and make the check more clear.
Comment on attachment 264413 [details] [diff] [review]
adding picture

>+    skin/classic/calendar/day-box-todo.png            (themes/common/day-box-todo.png)
>+    skin/classic/calendar/day-box-todo-completed.png  (themes/common/day-box-todo-completed.png)
I suggest that we combine those icons in a single file, that way we keep the number of files down and group related content together.

>+                <xul:image anonid="item-icon"
>+                           class="calendar-month-day-box-item-image"
>+                           type="todo"
>+                           hidden="true"/>
You probably don't want to introduce a type attribute that is hardwired to "todo", even if this element is to be used for an event. I propose that you just don't specify anything here and set the required attribute later in the constructor.

>           var str = null;
>           // if val is an event and not an all day event, show start time in title
>           if (isEvent(val) && !val.startDate.isDate) {
>             var df = Cc["@mozilla.org/calendar/datetime-formatter;1"].
>                      getService(Ci.calIDateTimeFormatter);
>             str = df.formatTime(val.startDate.getInTimezone(this.calendarView.mTimezone));
>             label.setAttribute("time", "true");
>+            icon.hidden = true;
You already set the image to be hidden in the content, so there's probably no reason to repeat that here.

>           } else if (isToDo(val)) {
>-            // yeah, this should really be a little picture instead of a "*"
>-            str = "*";
>+            if (val.isCompleted)
>+              icon.setAttribute("type", "todo-completed");
>+            else
>+              icon.setAttribute("type", "todo");
>+            icon.hidden = false;
I suggest that you set the type attribute to the type of the item, event or todo. Additionally, you'd need a second attribute "completed" or something like that, which gets set if this todo is completed. This enables themes to introduce images for events and doesn't hardware this feature into the css rules.

>           } else {
>-            str = "";
>+            icon.hidden = true;
Same thing here, you can simply get away without doing anything here, since the image is already hidden.

>+.calendar-month-day-box-item-image[type="todo-completed"] {
>+  list-style-image: url("chrome://calendar/skin/day-box-todo-completed.png");
>+  margin: 2px;
>+  margin-right: 4px;
See my comment above, I suggest to use a secondary attribute to model the "completed" icon. Furthermore, you probably want to use moz-image-region here after having the icons combined in a single image.
Attachment #264413 - Flags: review?(michael.buettner) → review-
I would recommend to keep the icon box/element as flexible as possible. I can imagine that either we or themes authors might want to set also an icon for e.g. normal events, repeating events, all-day events, tasks. 

Would the following work: the icon is always shown and not hard-coded to be hidden in the xul. In the css we have a generic rule that hides the icon (e.g. display: none) if no special type is set. Further type-specific rules show the icon and set the image. In the code we just apply the type and the rest is done automatically be the css rules. We could even already set the type for e.g. all-day events without providing an icon now.
(In reply to comment #8)
> I would recommend to keep the icon box/element as flexible as possible. I can
> imagine that either we or themes authors might want to set also an icon for
> e.g. normal events, repeating events, all-day events, tasks. 
That's exactly the direction I tried to drive this patch towards :-)

> In the css we have a generic rule that hides the icon (e.g.
> display: none) if no special type is set.
That's a good idea. Markus, could you please take this into account before submitting a new iteration of this patch? So, we just don't add hidden="true" to the image tag and define the default rule "display: none" to the appropriate class. That should give us maximum flexibility.
Thanks for the comments to all - I'm learning... :-)

I have the new code ready but an updated patch will be here tomorrow when I'm at home again where I have my linux-machine with all the tools.
I combined the two images and changed the css to reflect all the above ideas. I also already set the type for all-day events as Stefan suggested.
The xul image tag just gets an anon-id and class. In the constructor "type" and "completed" are added.
I tried to come up with more self explanatory icons but failed due to my really bad art skills ;-)
Attached patch Patch V2 (obsolete) — Splinter Review
Comments see above.
Attachment #264413 - Attachment is obsolete: true
Attachment #264658 - Flags: ui-review+
Attachment #264658 - Flags: review?(michael.buettner)
Attached file Binaries V2
The updated Picture file
Attachment #264414 - Attachment is obsolete: true
I noticed there is a bug in the month view, that makes the height of the box about four times higher as wanted. (e.g. by dragging it to another box)
In testing a freshly downloaded build, I confirmed that this has nothing to do with this patch.
I will report this one...
Hmm, further searching revealed that this bug is already known as
Bug 375390
Comment on attachment 264658 [details] [diff] [review]
Patch V2

>+          var icon = document.getAnonymousElementByAttribute(this,
>+                                                             "anonid",
>+                                                             "item-icon");
>           var str = null;
>           // if val is an event and not an all day event, show start time in title
>           if (isEvent(val) && !val.startDate.isDate) {
>             var df = Cc["@mozilla.org/calendar/datetime-formatter;1"].
>                      getService(Ci.calIDateTimeFormatter);
>             str = df.formatTime(val.startDate.getInTimezone(this.calendarView.mTimezone));
>             label.setAttribute("time", "true");
>           } else if (isToDo(val)) {
>-            // yeah, this should really be a little picture instead of a "*"
>-            str = "*";
>+            icon.setAttribute("type", "todo");
>+            if (val.isCompleted)
>+              icon.setAttribute("completed", "true");
>           } else {
>-            str = "";
>+            icon.setAttribute("type", "all-day");
>           }
> 
>           label.value = str;
The variable 'str' is no longer necessary, since we're setting the 'item-label' only in case it's a non-all-day event. also getting the 'item-label' element in every case is also not necessary. it would be better to get this element only in the case when it's needed. one last comment, i think it would be a good idea to be a bit more verbose in terms of the attributes that could be picked up by new themes. in other words, it wouldn't hurt to add a "this is an event"-attribute. just putting what i said into a piece of code, it could look like this:

          var icon = document.getAnonymousElementByAttribute(this,"anonid","item-icon");                                                              
          if (isEvent(val)) {
            icon.setAttribute("type", "event");
            if (val.startDate.isDate) {
              icon.setAttribute("type", "all-day");
            } else {
              var label = document.getAnonymousElementByAttribute(this,"anonid","item-label");
              var df = Cc["@mozilla.org/calendar/datetime-formatter;1"].
                       getService(Ci.calIDateTimeFormatter);
              label.value = df.formatTime(
                val.startDate.getInTimezone(
                  this.calendarView.mTimezone));
              label.setAttribute("time", "true");
            }
          } else if (isToDo(val)) {
            icon.setAttribute("type", "todo");
            if (val.isCompleted) {
              icon.setAttribute("completed", "true");
            }
          }

>+.calendar-month-day-box-item-image[type="todo"] {
>+  -moz-image-region: rect(0px 11px 11px 0px);
>+  display: inline;
>+}
>+.calendar-month-day-box-item-image[type="todo"][completed="true"] {
>+  -moz-image-region: rect(0px 22px 11px 11px);
>+  display: inline;
The second 'display: inline' is not strictly necessary, since it's already set during evaluation of the previous rule.

>+.calendar-month-day-box-item-image[type="todo"] {
>+  -moz-image-region: rect(0px 11px 11px 0px);
>+  display: inline;
>+}
>+.calendar-month-day-box-item-image[type="todo"][completed="true"] {
>+  -moz-image-region: rect(0px 22px 11px 11px);
>+  display: inline;
Same comment applies here.

Since all of this doesn't really need another iteration, we just go ahead with the patch as it currently stands. I'll just apply the above mentioned bits and pieces. Thanks for the patch. r=mickey.
Attachment #264658 - Flags: review?(michael.buettner) → review+
Whiteboard: [checkin needed after 0.5]
Attached patch patch v3Splinter Review
patch with bits and pieces addressed.
Attachment #264658 - Attachment is obsolete: true
Attachment #266465 - Flags: ui-review+
Attachment #266465 - Flags: review+
With the upcoming alarm icon from bug 288496, wouldn't it be smarter to put this icon to the right? I'm sorry for coming so late, but I just noticed this while wanting to check it in. 
After having a look at the alarm Bell, I think it's better to keep the icon on the left.
Maybe one wants to set an alarm on a Task and that way you could display both without the right side getting too crowded...
We should be consistant over all views. I agree Philip that aligning the badge on the right would make more sense. Doing so would assure that the Event/Task Title does not "jump" so much. Sooner or late the events/tasks should also display badges like "Repeating", "Private", "Important"... Placing all these infront of the Title would look really strange.
Christian, the last comments made this bug stuck and we're holding it off. I would like to know what's the strategy in order to resolve it. Do we want to revoke the already given positive review or do we want to address the placement of the icon in a spin-off bug? I'd prefer the latter alternative.
We can fix the placement in a spin-off bug, but we should be fix for 0.7
checkin-needed for attachment 264659 [details] and attachment 266465 [details] [diff] [review] (TRUNK and MOZILLA_1_8_BRANCH)
Whiteboard: [checkin needed after 0.5]
Is here a reason to wait for checkin? 
patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 0.7
Verified in Lightning build 2007073103 (Linux) and Sunbird build 20070731 (Windows) -> task is fixed.
Status: RESOLVED → VERIFIED
It doesn't work for linux (in my case Suse 10.2). No difference between finished and unfinished tasks. See screenshot:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.7pre) Gecko/20070906 Calendar/0.7pre
(In reply to comment #26)
This is already filed as Bug 393843.
You need to log in before you can comment on or make changes to this bug.