Closed
Bug 288496
Opened 20 years ago
Closed 17 years ago
Alarm indicator icon on event box
Categories
(Calendar :: Calendar Frontend, enhancement)
Calendar
Calendar Frontend
Tracking
(Not tracked)
VERIFIED
FIXED
0.8
People
(Reporter: esullivan, Assigned: Fallen)
References
Details
Attachments
(17 files, 10 obsolete files)
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
|
chris.j.bugzilla
:
review+
|
Details |
2.72 KB,
image/png
|
Details | |
19.35 KB,
application/octet-stream
|
Details | |
34.49 KB,
patch
|
michael.buettner
:
review+
Fallen
:
ui-review+
|
Details | Diff | Splinter Review |
2.87 KB,
patch
|
ssitter
:
review+
|
Details | Diff | Splinter Review |
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•19 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•19 years ago
|
QA Contact: gurganbl → sunbird
Comment 2•19 years ago
|
||
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.
Comment 3•19 years ago
|
||
Comment 4•19 years ago
|
||
Comment 5•19 years ago
|
||
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)
Comment 6•19 years ago
|
||
(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•19 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 8•18 years ago
|
||
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 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•18 years ago
|
||
Comment 11•18 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.
Updated•18 years ago
|
Attachment #219487 -
Flags: second-review?(lilmatt)
Comment 12•18 years ago
|
||
This is the "static" icon used to indicate an event / alarm that has not yet passed.
Attachment #263667 -
Flags: review?(christian.jansen)
Comment 13•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
Attachment #263670 -
Flags: review?(christian.jansen)
Comment 15•18 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•18 years ago
|
||
This mock-up shows the yellow bells in 10x10px.
Comment 17•18 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•18 years ago
|
||
Attachment #263811 -
Flags: review?(christian.jansen)
Comment 19•18 years ago
|
||
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•18 years ago
|
||
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•18 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•18 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•18 years ago
|
||
Attachment #263821 -
Flags: review?(christian.jansen)
Comment 24•18 years ago
|
||
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•18 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•18 years ago
|
Attachment #263823 -
Flags: review?(christian.jansen) → review+
Comment 26•18 years ago
|
||
Thanks Chris. Glad to help.
Assignee | ||
Comment 27•18 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•18 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•18 years ago
|
Assignee: nobody → bugzilla
Status: ASSIGNED → NEW
Assignee | ||
Updated•18 years ago
|
Status: NEW → ASSIGNED
Comment 28•17 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•17 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•17 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•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
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)
Comment 34•17 years ago
|
||
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•17 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•17 years ago
|
||
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•17 years ago
|
||
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•17 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•17 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•17 years ago
|
||
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•17 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•17 years ago
|
||
Correction to the above "how do you create PNG images" - should be APNG images.
Assignee | ||
Comment 43•17 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•17 years ago
|
||
This was made with Jordan's help. I'm not sure if the size is ok - 26x21?
Assignee | ||
Comment 45•17 years ago
|
||
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•17 years ago
|
||
Are you sure you attached the right file Philipp? It looks like the same thing.
Assignee | ||
Comment 47•17 years ago
|
||
Oops, I did. Sorry.
Attachment #267412 -
Attachment is obsolete: true
Comment 48•17 years ago
|
||
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•17 years ago
|
Attachment #267420 -
Attachment is obsolete: true
Comment 49•17 years ago
|
||
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.
Updated•17 years ago
|
Attachment #267534 -
Attachment mime type: image/png → application/octet-stream
Comment 50•17 years ago
|
||
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•17 years ago
|
||
(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 52•17 years ago
|
||
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•17 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.
>
Comment 54•17 years ago
|
||
(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).
Comment 55•17 years ago
|
||
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.
Updated•17 years ago
|
Attachment #263821 -
Flags: review?(christian.jansen)
Updated•17 years ago
|
Attachment #263813 -
Flags: review?(christian.jansen)
Updated•17 years ago
|
Attachment #263667 -
Flags: review?(christian.jansen)
Updated•17 years ago
|
Attachment #263668 -
Flags: review?(christian.jansen)
Updated•17 years ago
|
Attachment #263670 -
Flags: review?(christian.jansen)
Updated•17 years ago
|
Attachment #263811 -
Flags: review?(christian.jansen)
Updated•17 years ago
|
Attachment #263812 -
Flags: review?(christian.jansen)
Assignee | ||
Comment 56•17 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•17 years ago
|
Attachment #268929 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Depends on: warOnBoxes
Assignee | ||
Comment 57•17 years ago
|
||
unbitrot patch
Attachment #267355 -
Attachment is obsolete: true
Attachment #282773 -
Flags: ui-review+
Attachment #282773 -
Flags: review?(michael.buettner)
Comment 58•17 years ago
|
||
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+
Updated•17 years ago
|
Whiteboard: [checkin-needed after 0.7]
Assignee | ||
Comment 59•17 years ago
|
||
Checked in on HEAD and MOZILLA_1_8_BRANCH
-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed after 0.7]
Target Milestone: 1.0 → 0.8
Comment 60•17 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
Comment 61•17 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•17 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•17 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.
Assignee | ||
Comment 64•17 years ago
|
||
Thanks for catching this. Would you like a new bug or is it ok as is?
Attachment #287104 -
Flags: review?(ssitter)
Comment 65•17 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•17 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.
Description
•