bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Add picture to Task in MonthView (instead of *)

VERIFIED FIXED in 0.7

Status

Calendar
Calendar Views
--
enhancement
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Taraman, Assigned: Taraman)

Tracking

unspecified

Details

Attachments

(4 attachments, 3 obsolete attachments)

7.50 KB, image/jpeg
Details
1.22 KB, application/zip
Details
9.58 KB, patch
Michael Büttner (no reviews TFN)
: review+
Michael Büttner (no reviews TFN)
: ui-review+
Details | Diff | Splinter Review
125.38 KB, image/jpeg
Details
(Assignee)

Description

11 years ago
In the MonthView in front of a Task there is a "*" which should be a picture instead.
(Assignee)

Comment 1

11 years ago
Created attachment 264413 [details] [diff] [review]
adding picture

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)
(Assignee)

Comment 2

11 years ago
Created attachment 264414 [details]
Picture Binaries 

The two .png-files for the patch
(Assignee)

Comment 3

11 years ago
Created attachment 264415 [details]
Screenshot for the patch

Comment 4

11 years ago
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 5

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

Comment 10

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

Comment 11

11 years ago
Created attachment 264658 [details] [diff] [review]
Patch V2

Comments see above.
Attachment #264413 - Attachment is obsolete: true
Attachment #264658 - Flags: ui-review+
Attachment #264658 - Flags: review?(michael.buettner)
(Assignee)

Comment 12

11 years ago
Created attachment 264659 [details]
Binaries V2

The updated Picture file
Attachment #264414 - Attachment is obsolete: true
(Assignee)

Comment 13

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

Comment 14

11 years ago
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]
Created attachment 266465 [details] [diff] [review]
patch v3

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

Comment 18

11 years ago
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...
Keywords: checkin-needed

Comment 19

11 years ago
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.

Comment 21

11 years ago
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]

Comment 23

11 years ago
Is here a reason to wait for checkin? 
patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 0.7

Comment 25

11 years ago
Verified in Lightning build 2007073103 (Linux) and Sunbird build 20070731 (Windows) -> task is fixed.
Status: RESOLVED → VERIFIED

Comment 26

11 years ago
Created attachment 279982 [details]
Broken task icon for linux

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.