Alarm indicator icon on event box

VERIFIED FIXED in 0.8

Status

Calendar
Calendar Views
--
enhancement
VERIFIED FIXED
13 years ago
10 years ago

People

(Reporter: Elroy Sullivan, PhD, Assigned: Fallen)

Tracking

unspecified
Dependency tree / graph

Details

Attachments

(17 attachments, 10 obsolete attachments)

12.92 KB, patch
cmtalbert
: first-review-
Details | Diff | Splinter Review
408 bytes, image/png
Details
383 bytes, image/png
Details
601 bytes, image/png
Details
2.99 KB, image/png
Details
3.92 KB, image/png
Details
1.44 KB, image/gif
Details
17.04 KB, image/png
Details
982 bytes, image/gif
Details
3.80 KB, image/png
Details
2.51 KB, image/gif
Details
1.04 KB, image/gif
Details
4.00 KB, image/x-png
Christian Jansen
: review+
Details
2.72 KB, image/png
Details
19.35 KB, application/octet-stream
Details
34.49 KB, patch
Michael Büttner (no reviews TFN)
: review+
Fallen
: ui-review+
Details | Diff | Splinter Review
2.87 KB, patch
Stefan Sitter
: review+
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050317 Firefox/1.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050317 Firefox/1.0.2

Someone, Please!!, add an icon on each event on the main calendar display
indicating whether or not an alarm is set.  I love the calendar, but I am always
paranoid that I have accidentally acknowledged an alarm for some meeting.  I am
constantly opening my events to make sure the alarms are set.  If there was an
icon (or anything) that showed whether or not the alarm was set for each event
on the calendar view, this would be immensely helpful.

A HUGE THANKS TO WHOMEVER TAKES THIS ONE.
Sincerely,
Elroy Sullivan, PhD

Reproducible: Always

Steps to Reproduce:
See Details
Actual Results:  
See Details

Expected Results:  
See Details

See Details

Comment 1

13 years ago
Surprisingly, I couldn't find a confirmed report for this enhancement.  I agree
it would probably be quite useful.  Worst case, a checkbox preference can be
created for whether or not the icon should be displayed.  Tweaking the summary a
bit, and a few other parameters, and then confirming.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Summary: Alarm Indicator → Alarm indicator icon on event box

Updated

13 years ago
QA Contact: gurganbl → sunbird

Comment 2

12 years ago
Created attachment 219487 [details] [diff] [review]
add icon and flashing support

Patch adds an icon to all the views for when an item has an alarm.  When bug 335129 is fixed, these icons will even flash colors when the alarm is firing.
Assignee: mostafah → jminta
Status: NEW → ASSIGNED
Attachment #219487 - Flags: first-review?(dmose)

Comment 3

12 years ago
Created attachment 219488 [details]
alarm_normal.png

Comment 4

12 years ago
Created attachment 219489 [details]
alarm_red.png
This should be optionial.
The usual thing you want to know when looking at the calendar view is: what do i have to do today?, or: when do i have time to do this? I really don't care what event-with-alarm i have today. And when i'm planning, i also don't care about alarms.
I can see the use-case for the icons in comment 0, but i think that's so minor that it should be an option (eitehr a pref, an extension or a user-chrome.css rule)
(In reply to comment #5)
> ...it should be an option (eitehr a pref, an extension or a user-chrome.css
> rule)

I suggest it being a pref, once we have a real prefs.js, and as small an icon as is feasible, so as to not take up as much space in the views. 

Nit: It took me a while to figure out that the icon was an alarm clock. Perhaps a simple bell would be a better icon, and perhaps both states could be in a single .png, similar to the toolbars.

Just some thoughts.

Comment 7

12 years ago
(In reply to comment #5)
> This should be optionial.
Given that all the calendar programs I've used (Evolution/Google) do this, I think it should at least be a non-hidden pref.

(In reply to comment #6)
> Nit: It took me a while to figure out that the icon was an alarm clock. Perhaps
> a simple bell would be a better icon, and perhaps both states could be in a
> single .png, similar to the toolbars.
I'm definitely not an artist, and would never claim to be.  The icons were mostly so I had something semi-reasonable to work with.  I'd be more than happy to see a better icon provided.
Comment on attachment 219487 [details] [diff] [review]
add icon and flashing support

Moving reviews to ctalbert and lilmatt per dmose
Attachment #219487 - Flags: second-review?(lilmatt)
Attachment #219487 - Flags: first-review?(dmose)
Attachment #219487 - Flags: first-review?(cmtalbert)

Comment 9

12 years ago
Comment on attachment 219487 [details] [diff] [review]
add icon and flashing support

I'm sorry, but I think the patch is a bit incomplete. The code that is in this patch looks good, and should work. However, I found a few problems.
1. There is much talk of making this a preference in the comments, but there is no code here to handle a preference. You can treat this as a nit if you want to just open a follow-up bug for the preference.
2. This is the serious issue. OnAlarm does not seem to be called. I have the patch for bug 335129 in my tree and still onAlarm is not called in either the multiday view or the month view. Might this be a recent regression?
3. Nit: There are no icons for pinstripe in this patch (in lightning/jar.mn or resources/jar.mn)
4. Nit: I also had a difficult time understanding the image was an alarm. Although I'm also not an artist, I  think that putting hands on the alarm icon will help. I will upload an image of this as a suggestion.

I applied this patch to a debug sunbird build on widnows based on 20061023 (but updated since then), in order to perform the above testing.
Attachment #219487 - Flags: first-review?(cmtalbert) → first-review-

Comment 10

12 years ago
Created attachment 243846 [details]
Adding hands to the icon

Comment 11

12 years ago
I have one other thought about the icon. The old analog alarm clock, which is what this looks like, will only be an intuitive image for about half of the target users of Sunbird/Lightning. Our target users are "small business folk and college students." And I'd argue that if you show the icon to most college students these days, "alarm clock" is probably not the first thing they'd say.

The bell icon is widely used in other calendar apps, which I think is both a good and a bad thing. Unfortunately, I can't draw a bell that looks like a bell, so I can't suggest anything better. 

I vote that we get somebody with a good measure of artistic talent to help with the icon.

However, please don't confuse the icon with the primary issue of my review, which is still the fact that "onAlarm" is not being called. 
Attachment #219487 - Flags: second-review?(lilmatt)

Comment 12

11 years ago
Created attachment 263667 [details]
This is the alarm indicator in "default" state - before the event has passed

This is the "static" icon used to indicate an event / alarm that has not yet passed.
Attachment #263667 - Flags: review?(christian.jansen)

Comment 13

11 years ago
Created attachment 263668 [details]
This is the alarm indicator in "animated" state - this plays after the event has passed in the same place as the "static" icon

This is a frame set in 16x64 pixel format. Each icon is aligned and set into a 16x16 space. A unique frame starts at each 16 pixels horizontal position.
Attachment #263668 - Flags: review?(christian.jansen)

Comment 14

11 years ago
Created attachment 263670 [details]
This is the a sample gif (animated) showing how the frames of the "16x64" PNG should look when composed into an animation

This GIF does not have alpha transparency. It is for demonstration only. Please use the multiple frame (16x64) PNG for development since it has alpha transparency and will anti-alias better on different colored backgrounds.

Updated

11 years ago
Attachment #263670 - Flags: review?(christian.jansen)

Comment 15

11 years ago
Some feedback on the new icons:

1. The size should be reduced to 10x10px. From my point of view they consume too much space.
2. Additionally the icon should offer a 1px drop shadow. This assures a better contrast to event box colors with the same saturation.
3. The icon should be aligned right of the event box

Comment 16

11 years ago
Created attachment 263718 [details]
This mock-up shows the yellow bells in 10x10px.

This mock-up shows the yellow bells in 10x10px.

Comment 17

11 years ago
(In reply to comment #15)
> Some feedback on the new icons:
> 1. The size should be reduced to 10x10px. From my point of view they consume
> too much space.
> 2. Additionally the icon should offer a 1px drop shadow. This assures a better
> contrast to event box colors with the same saturation.
> 3. The icon should be aligned right of the event box

I will revise the icon to address this. Animating this within the 10x10 px area may be limiting since there was previously a little room for the "swing" in the 16x16 area. To overcome this limitation...

...Instead of actually doing a drop shadow I propose we darken the inside edges of the bell - trying to make it look like its shading on the bell rather than a drop shadow. This would be preferrable for two reasons; it wont add extra pixels to the outside - allowing for at least 2 more horizontal (and vertical) pixels for animation and secondly I would prefer not to use a drop shadow simply for the purpose of "blocking it off" from a simular colored background. This would also look sharper and more professional and drop shadows can make very small icons appear "muddy".

On your sample, including your drop shadow, the image is actually about 11x11. I'd like to use the 10x10 desired size as *a guideline rather than a restriction*... since I can compose it to appear and look approximately the same size as what you show in your sample...

1 to 2 pixels width and height breathing room (11x11 or 12x12) is significant, that is 21 or 44 "square pixels" more repectively, when we are working on a such a small canvas... I will compose it so that it appears the same relative size as your mock-up.

In my revision I'll compose it above various boxes (including the yellow) to be sure that it looks good. I'll provide that as a sample with the attachment for your review.

Comment 18

11 years ago
Created attachment 263811 [details]
Revised alarm indicator "animated" state - preview GIF
Attachment #263811 - Flags: review?(christian.jansen)

Comment 19

11 years ago
Created attachment 263812 [details]
Revised alarm indicator - PNG frames

This PNG for production use contains a sequence of frames a new frame exists at every 14th horizontal pixel.
Attachment #263812 - Flags: review?(christian.jansen)

Comment 20

11 years ago
Created attachment 263813 [details]
Revised alarm indicator - comparison

This image shows a comparison of the revised alarm (rightmost column) across different background colors.

It also includes the original (16x16 bell for comparison and Christian's mock-up bell with drop-shadow (far left).

The revision addresses the issue of laying it above different backgrounds without making it "muddy" with a drop-shadow. The animation also looks just as clear above each color.
Attachment #263813 - Flags: review?(christian.jansen)

Comment 21

11 years ago
Look at both animated versions side-by-side

Old - https://bugzilla.mozilla.org/attachment.cgi?id=263670

New - https://bugzilla.mozilla.org/attachment.cgi?id=263811

As well as providing a new smaller size (relative to Christian's sample) I have also darked the edges of the bell to provide better contrast on top of lighter backgrounds.

Comment 22

11 years ago
The actual size here is 12x11 for the static (non animated) version. For the "swing" animation (left and right) 2 more horizontal pixels are required so this is a total of 14x11.

This icon appears sigificantly smaller than that in the original 16x16 version and has a relative visual size to Christian's 11x11 sample.

12x11 is about the smallest I can make the static version while keeping a nice shape and look for the bell.

Comment 23

11 years ago
Created attachment 263821 [details]
Revised animation to appear more natural (gif sample)
Attachment #263821 - Flags: review?(christian.jansen)

Comment 24

11 years ago
Created attachment 263823 [details]
Revised animation to appear more natural (png frames)

The area of the icon is 14x11. A new frame of the animation exists every 14 pixels horizontal. 6 frames in all - from 3 frames for the previous version.
Attachment #263823 - Flags: review?(christian.jansen)

Comment 25

11 years ago
Thanks for doing all the work. This bell looks really great.
Would be great if we can get this in with the (0.7) release.

Updated

11 years ago
Attachment #263823 - Flags: review?(christian.jansen) → review+

Comment 26

11 years ago
Thanks Chris. Glad to help.
(Assignee)

Comment 27

11 years ago
I have a patch that works for the month view, but I still have some issues to work out with the multiday views. Too bad there is no APNG support on branch, that would make showing the image a bit less hacky.
Assignee: jminta → nobody
Status: ASSIGNED → NEW
Component: Sunbird Only → Calendar Views
QA Contact: sunbird → views
Target Milestone: --- → 1.0
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Assignee: nobody → bugzilla
Status: ASSIGNED → NEW
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED

Comment 28

11 years ago
If you need an apng version of the animation please send me the frames and the desired timeout and i'll make one.

Comment 29

11 years ago
(In reply to comment #28)
> If you need an apng version of the animation please send me the frames and the
> desired timeout and i'll make one.

I think the issue may be that there isn't actually APNG support - not that the file wasn't supplied that way. If you do produce an APNG please use the most recent gif sample attached to this bug as reference for frame timing.
(Assignee)

Comment 30

11 years ago
APNG support is only in the latest trunk and as of now not totally fixed. See bug 257197. I am currently working around this issue, but as soon as APNG goes into branch, I'll be happy to update the code.
(Assignee)

Comment 31

11 years ago
Created attachment 266074 [details] [diff] [review]
Work in Progress

I'm not requesting review yet since there is still an open issue: Alarms will not appear moving if the view has not been previously initialized. This is most notable at startup. The alarms fires before the views are initialized, so the view never recieves the alarm notification and the bell dosen't ring.

My plans are to move the timer and the array of "ringing" elements into either a component or the containing xul elements and then only make the bell ring in the currently visible view. Or should I the views just be initialized at startup?

Any other comments on things I should do fundamentally different?
(Assignee)

Comment 32

11 years ago
Created attachment 266248 [details] [diff] [review]
Adds flashing support to all views

There we go. This patch also takes care of flashing when the app is started. It was less trouble than I thought. This version also only uses one timeout for alarms, instead of a timeout for each alarm.

There is one problem, but this problem also existed before, so I think its worth a spin-off (if it has not already been filed): In the multiday views, the alarm image will not be visible if the event title is very long. The image is still there, but its to the right of the text. In month views, the text is cropped and a ... is appended.

Tested on recent lightning/sunbird cvs branch builds.

I have introduced the following prefs. I am not sure if I should add them to the default prefs.js file though. All prefs have the calendar.alarms.indicator prefix:

.show (BOOL): should the icon be shown? (observed)
.totaltime (INT): The total time the bell should be shown (interesting for user)
.frames (INT): How many frames does the image have (for theme devs)
.timeout (INT): The time a frame should be shown (for theme devs)

The default values for those prefs are ones that seem sensible for our default alarm image. With APNG support we can get rid of both .frames and .timeout
Attachment #266074 - Attachment is obsolete: true
Attachment #266248 - Flags: review?(michael.buettner)
(Assignee)

Comment 33

11 years ago
Created attachment 266369 [details] [diff] [review]
Adds flashing support to all views - v7

Added a few comments to make code more understandable. Also, do not animate when there is only 1 frame or the indicator should not be shown. No need to animate a hidden image :)
Attachment #266248 - Attachment is obsolete: true
Attachment #266369 - Flags: review?(michael.buettner)
Attachment #266248 - Flags: review?(michael.buettner)
I don't like using preferences to configure the icon animation. First of all, I'm not sure that a theme can override preferences. But even if that is the case, how are the preference values restored in case the theme is deactivated? But even if I'm wrong, and all of this is possible, I still have the strong impression that this is not the right way to go. I'd prefer to stick to css rules as much as possible, and use something different if there's absolutely no other way to achieve what we're trying to do.

Yesterday I spend a whole lot of bunch of time thinking about this problem and
I came up with a (more or less) elegant solution. The problem we have is "how
do we detect the total number of frame for the icon animation?". To be more precise, we need to do that in a way so that theme developers can easily provide their own version of this animation. Basically, it's not strictly necessary to answer that question. All we want to do is to somehow detect the end of the animation. Knowing the total number of frames is just one way to do that. Another way goes like this:

construct css rules like that:

alarm-icon {
  display: none;
  list-style-image: url("chrome://...");
}

alarm-icon[frame="1"] {
  -moz-image-region: rect(...);
  display: inline;
}

alarm-icon[frame="2"] {
  -moz-image-region: rect(...);
  display: inline;
}

...

The animation code now just needs to increment the 'frame' attribute. After stepping one frame forward, we need to inspect whether or not the icon is hidden. In case it appears to be hidden, we conclude that there's no rule that picked up our new frame number and resetting the current frame number back to zero (or one). If we set/check in one fellow swoop, we avoid the repaint event and just don't introduce visual glitches. That way the animation itself is completely done by css rules and the logic doesn't need to care about the details, it just needs to take care of incrementing the frame attribute at a constant rate. This also works with animation delay, loops and repeats. The css rules just need to be set accordingly, as long as the rules continuously pick up the frame numbers, they can set the image as they like.

Feel free to tell me that I'm totally on crack and this just doesn't work... ;-)

Comment 35

11 years ago
Thanks for adding the alarm indicator icon to events, that will be very nice.  However, I'm not convinced that having an *animated* icon is worthwhile.  It seems that it's adding complexity (and thus potential bugs) with not much added benefit.

In order to see the animated icon, we would have to be looking at our calendar and the particular event would have to be on our screen when the alarm fires.  It seems to me that that situation would not happen very often.  For me at least, most of the time I would either be working in an unrelated application or away from my computer.  Because the icon only flashes for a limited number of seconds, I would not see it most of the time.

Besides, why have a flashing icon when the "alarm reminder" dialog is already used to announce events?  Surely the alarm dialog will catch our attention before the animated icon, and by the time that we dismiss the dialog, the icon will probably have stopped its animation.  I realize that the alarm dialog doesn't currently grab focus to come to the foreground, but my opinion is that fixing that focus bug is much more important than having an animated icon on the event.

My goal is not to deny this feature to people who want it.  I just thought that someone should mention these points.
(Assignee)

Comment 36

11 years ago
Created attachment 267207 [details] [diff] [review]
Adds flashing support to all views - v8

After some discussion we noticed that themes cannot override prefs. They can change xbl bindings though. This approach uses a such binding.

I agree that the animation is not very important, but I liked the challenge of making the image themeable.
Attachment #266369 - Attachment is obsolete: true
Attachment #267207 - Flags: review?(michael.buettner)
Attachment #266369 - Flags: review?(michael.buettner)
(Assignee)

Comment 37

11 years ago
Created attachment 267249 [details] [diff] [review]
Adds flashing support to all views - v9

Minor style fixes. I guess I was a bit tired yesterday evening ;)
Attachment #267207 - Attachment is obsolete: true
Attachment #267249 - Flags: review?(michael.buettner)
Attachment #267207 - Flags: review?(michael.buettner)

Comment 38

11 years ago
It's nice that you got it working but personally I don't like adding unnecessary code just to show an animated image. 
Why not just use an animated GIF file? Or an APNG file? In case of the APNG users would see a different alarm icon (if the first frame is different than the default icon) and once we switch back to Trunk users would see the animation too.

Comment 39

11 years ago
I made this animated PNG and I tend to agree that the animation is superfluous here. If in the future Sunbird supports a tray icon, like Outlook, this animation may be more useful as an indicator. The icon can be cropped to 16x16 or less (with animation) so this would conform to the tray icon size.

Regardless of if a static icon or animated version is used I would certainly recommend against GIF since it cannot provide real alpha blended anti-aliasing and this tends to look poor especially on different coloured background surfaces such as the calendar. APNG or PNG seems appropriate in this case.
(Assignee)

Comment 40

11 years ago
Created attachment 267355 [details] [diff] [review]
Adds flashing support to all views - v10

Since we are still pre-1.0 and there are not so many themes for sunbird, I (now) also think its better to just use an APNG, resulting in a static image on branch and an animated image on trunk. Thankfully, APNG is downward compatible to png. This approach uses two images, one for non-flashing events and one for flashing events. The code also looks much cleaner :)

I tried to make an APNG from the png frames but it looks strange, mostly because I didn't want to waste too much time on that. It would be nice if one of you could attach the compiled APNG image.
Attachment #267249 - Attachment is obsolete: true
Attachment #267355 - Flags: review?(michael.buettner)
Attachment #267249 - Flags: review?(michael.buettner)

Comment 41

11 years ago
How do you create PNG images? I'd be happy to do one for you if you can suggest a tool / plug-in compatible with Photoshop or ImageReady. I have seen GIF>APNG converters but this wouldn't be useful since the advantage of PNG is to retain the alpha blending / transparency and this would be lost in outputting it to GIF first. If you have any suggestions or can point me in the right direction then I may be able to produce an APNG for you. I will also make it so the first frame is the "middle state" of the bell so the backwards compatible feature of APNG will show the middle state if this APNG is loaded in something with only PNG support.

Comment 42

11 years ago
Correction to the above "how do you create PNG images" - should be APNG images.
(Assignee)

Comment 43

11 years ago
Andrew (who is also the author of the APNG patch) volunteered to create the image for us yesterday in IRC. Otherwise, the trial version of this tool [1] is able to create APNG images. I managed to create the image correctly, but in some states some clipping occurred which made the image look strange from a close up view.


[1] http://www.gamani.com/apng.htm

Comment 44

11 years ago
Created attachment 267409 [details]
alarm APNG 26x21

This was made with Jordan's help. I'm not sure if the size is ok - 26x21?
(Assignee)

Comment 45

11 years ago
Created attachment 267412 [details]
alarm APNG 14x11

The image I used for testing was 14x11. I cropped your version to be just as big as there are pixels. I think there is no need to have extra transparent space. If someone objects, feel free to change obsoletion status.
Attachment #267409 - Attachment is obsolete: true

Comment 46

11 years ago
Are you sure you attached the right file Philipp? It looks like the same thing.
(Assignee)

Comment 47

11 years ago
Created attachment 267420 [details]
alarm APNG 14x11

Oops, I did. Sorry.
Attachment #267412 - Attachment is obsolete: true

Comment 48

11 years ago
Created attachment 267421 [details]
alarm APNG 14x11 with 'background' dispose mode

The one you just attached has the dispose mode 'None' I think, It looks rather strange. I remade it with dispose 'Background'.

Not obsoleting the other one cause I'm not sure whether you did that intentionally or not.
(Assignee)

Updated

11 years ago
Attachment #267420 - Attachment is obsolete: true

Comment 49

11 years ago
Created attachment 267534 [details]
PNG frames cropped to minimum

Attached are the PNG frames in seperate files. These are cropped to 14x11 to minimize any wasted space. The crop is exactly the amount (and not more) than that required for the animation. Since the bell "swings" the animation itself covers a 14px length and so this is the length of all frames to allow correct alignment of each in the animation.

Please keep in mind this is a PNG with alpha channel so that the transparency has true anti-aliasing so that it works best over various coloured backgrounds which vary in the calendar view. I'm not certain that the APNG converter preserves this but I assume that it does.

Andrew - you may want to use this version of the frames to recompile the APNG file since these are now using the minimum crop required.
Attachment #267534 - Attachment mime type: image/png → application/octet-stream
Comment on attachment 267355 [details] [diff] [review]
Adds flashing support to all views - v10

>+    get hashKey() {
>+        // Use id multiple times to avoid ambiguity
>+        var hashKey = this.id + this.recurrenceId + this.id;
>+
>+        if (this.calendar) {
>+            hashKey += this.calendar.id + this.id;
>+        }
>+        return hashKey;
>+    },
I just talked with daniel about this particular piece of code. We both feel that this should be a helper function inside the views instead of being available at calIItemBase. The reason for this is that basically it's a flaw of the views that makes this necessary. So, we should probably keep this inside of their scope instead of exposing this flaw to the outside world. Basically, once the got rid of the cloning and moving to the transaction model we're free to directly compare references to items. This would also make the need for something like a hashkey unnecessary.

Something else that came up is the question why we need to concatenate id twice in the above piece of code? What's the possible ambiguity you're referring to in the comment?

This issue aside, the patch seems to look good, much cleaner than the last iteration. I think it's a good idea to let the backend handle the animation instead of trying to fake this.
Attachment #267355 - Flags: review?(michael.buettner) → review-
(Assignee)

Comment 51

11 years ago
Created attachment 268929 [details] [diff] [review]
Adds flashing support to all views - v11

(In reply to comment #50)
> I just talked with daniel about this particular piece of code. We both feel
> that this should be a helper function inside the views instead of being
> available at calIItemBase.
Fixed. Moved the hashKey attribute to a hashItem() function in calendar-views.js

> Something else that came up is the question why we need to concatenate id twice
> in the above piece of code? What's the possible ambiguity you're referring to
> in the comment?
I included a small example in the comments. Since we are not sure what kind of ids other providers may use, I just wanted to make sure there were no ambiguities. I can take it out, but I don't feel it really hurts.
Attachment #267355 - Attachment is obsolete: true
Attachment #268929 - Flags: review?(michael.buettner)
Comment on attachment 268929 [details] [diff] [review]
Adds flashing support to all views - v11

Unfortunately I found one more flaw with this patch. Just add an alarm to an event, the alarm icon appears as it should. Now change to title of the event to some large string and notice that the description makes the icons disappear as it shifts it out of sight.

I also didn't like that the description and the icon get drawn on top of each other if the string happens to have just the right length. Wouldn't it be a good idea to just not put the image-tag on a separate layer in the <xul:stack> but put it in front of the description? This would solve both problems, wouldn't it? Of course, this works only if the image is placed to the left with respect to the description.

Otherwise the patch looks good to me, I didn't find anything else to complain about.
Attachment #268929 - Flags: review?(michael.buettner) → review-

Comment 53

11 years ago
(In reply to comment #52)
> (From update of attachment 268929 [details] [diff] [review])
> Unfortunately I found one more flaw with this patch. Just add an alarm to an
> event, the alarm icon appears as it should. Now change to title of the event to
> some large string and notice that the description makes the icons disappear as
> it shifts it out of sight.

We should get this one fixed. Long stringa should be shortened to assure that no icon gets pushed out. I can imagine that sooner or later events also indicate if they are an recurring, or private event. which means theat events display more than one icon.

In generell these icons should be right alingned. The event title should be shortend by using ellipses or having a word wrap.


> 
> I also didn't like that the description and the icon get drawn on top of each
> other if the string happens to have just the right length. Wouldn't it be a
> good idea to just not put the image-tag on a separate layer in the <xul:stack>
> but put it in front of the description? This would solve both problems,
> wouldn't it? Of course, this works only if the image is placed to the left with
> respect to the description.
> 
> Otherwise the patch looks good to me, I didn't find anything else to complain
> about.
> 

(In reply to comment #51)
> Created an attachment (id=268929) [details]
> Adds flashing support to all views - v11
> 
> (In reply to comment #50)
> > I just talked with daniel about this particular piece of code. We both feel
> > that this should be a helper function inside the views instead of being
> > available at calIItemBase.
> Fixed. Moved the hashKey attribute to a hashItem() function in
> calendar-views.js
However, I changed my mind on this; makes sense to have a general hashId for items since it's often used (attached to bug 363441).
Just as a reminder, the issue with the pushed icon could well be solved with bug #304741 landing. Since this is already in your review queue, you could try whether or not it solves the problem.
Attachment #263821 - Flags: review?(christian.jansen)
Attachment #263813 - Flags: review?(christian.jansen)
Attachment #263667 - Flags: review?(christian.jansen)
Attachment #263668 - Flags: review?(christian.jansen)
Attachment #263670 - Flags: review?(christian.jansen)
Attachment #263811 - Flags: review?(christian.jansen)
Attachment #263812 - Flags: review?(christian.jansen)
(Assignee)

Comment 56

11 years ago
Comment on attachment 267355 [details] [diff] [review]
Adds flashing support to all views - v10

After some testing, I found out that indeed bug 304741 fixes the remaining issue.

I have a non-bitrot version waiting that uses calIItemBase's hashId that is most similar to v10 and also makes sure the icon is in the right layer of the stack.
Attachment #267355 - Attachment is obsolete: false
(Assignee)

Updated

11 years ago
Attachment #268929 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Depends on: 304741
(Assignee)

Comment 57

11 years ago
Created attachment 282773 [details] [diff] [review]
Adds flashing support to all views - v13

unbitrot patch
Attachment #267355 - Attachment is obsolete: true
Attachment #282773 - Flags: ui-review+
Attachment #282773 - Flags: review?(michael.buettner)
Comment on attachment 282773 [details] [diff] [review]
Adds flashing support to all views - v13

Looks good -> r=mickey.
Attachment #282773 - Flags: review?(michael.buettner) → review+
Whiteboard: [checkin-needed after 0.7]
(Assignee)

Comment 59

11 years ago
Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed after 0.7]
Target Milestone: 1.0 → 0.8

Comment 60

11 years ago
Verified with Lt 2007102823 and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.9pre) Gecko/20071029 Calendar/0.8pre
Status: RESOLVED → VERIFIED

Updated

11 years ago
Blocks: 401546

Comment 61

10 years ago
The alarm icon looks good, but it doesn't flash on my computer (when the reminder window opens).

(Lightning 0.8pre 2007-10-29-05 / TB 2.0.0.6 / WinXP (classic theme))
(Assignee)

Comment 62

10 years ago
Pete, to have the image flash you need trunk, since an APNG image is used. APNG support is only available on trunk.

Comment 63

10 years ago
This bug added two new user preferences "calendar.alarms.indicator.show" and "calendar.alarms.indicator.totaltime". But I don't see them added to the default preferences file.

Updated

10 years ago
Depends on: 401905

Updated

10 years ago
Depends on: 401878
(Assignee)

Comment 64

10 years ago
Created attachment 287104 [details] [diff] [review]
[checked in] Add default preferences

Thanks for catching this. Would you like a new bug or is it ok as is?
Attachment #287104 - Flags: review?(ssitter)

Comment 65

10 years ago
Comment on attachment 287104 [details] [diff] [review]
[checked in] Add default preferences

r=ssitter
I'm fine with this.
Attachment #287104 - Flags: review?(ssitter) → review+
(Assignee)

Comment 66

10 years ago
Comment on attachment 287104 [details] [diff] [review]
[checked in] Add default preferences

Additional patch checked in on both branches.
Attachment #287104 - Attachment description: Add default preferences → [checked in] Add default preferences
You need to log in before you can comment on or make changes to this bug.