491207 - Icon files for event/task dialog windows

Status: VERIFIED FIXED

(Calendar :: General, defect)

Description

[gekacheka]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b5pre) Gecko/20090502 Shiretoko/3.5b5pre
Build Identifier: 

The event dialogs are no longer modal (bug 356833), so they appear in
window selections such as the task bar or the alt-tab icon list
(windows).  They need an icon.  (Similar to how Thunderbird message
compose window has an icon). 


Reproducible: Always

Steps to Reproduce:
1. Open event dialog window (e.g., new event)

Actual Results:  
1. alt-tab icon for dialog is default windows OS flag icon (pixelated on w2k),
   so hard to recognize it is part of sunbird or lightning,
   misleadingly associates it with operating system
2. no icon in task bar
3. no icon in title bar of event dialog

Expected Results:  
1. alt-tab popup shows event icon, or at least app icon (sunbird or thunderbird)
2. task bar shows event icon, or at least app icon
3. title bar of event dialog shows event icon, or at least app icon

Simplest fix is to use app icon since it is already made, and same icon can be used regardless of whether it is an event dialog or task dialog.  (The icon is associated with the window id, so separating event and task dialog might be accomplished by creating separate xul files for each window element and an overlay for the actual window content.  But that can wait until after 1.0 unless someone creates the appropriate icon files.)

Workaround:

In the install, non-default window-icons are located at
installdir/chrome/icons/default/ named by the id of the <window>.
[See "Window Icons" https://developer.mozilla.org/en/Window_icons]
(.ico file for windows, .xpm file for Linux)

A. For Sunbird, copy the application icon file
http://hg.mozilla.org/comm-central/file/790cf8238238/calendar/sunbird/app/sunbird.ico (windows)
to a place under the sunbird install dir:
sunbird/chrome/icons/default/calendar-event-dialog.ico (windows)
sunbird/chrome/icons/default/calendar-summary-dialog.ico (windows)

B. For Thunderbird/Lightning, copy the application icon file
http://hg.mozilla.org/comm-central/file/790cf8238238/mail/branding/nightly/thunderbird.ico (windows)
to a place under your TB user profile in the lightning extension install dir:
TBprofile/extensions/{e2f...}/chrome/icons/default/calendar-event-dialog.ico (windows)
TBprofile/extensions/{e2f...}/chrome/icons/default/calendar-summary-dialog.ico (windows)

Once the new icon files are in place, opening the event or task
dialog should now create a window with the icon that appears in its title bar, in the task bar, and in the alt-tab popup.  (Tested on w2k.)

Repository

In the comm-central repository, non-default window-icons are stored at
http://hg.mozilla.org/comm-central/file/790cf8238238/calendar/base/themes/winstripe/icons/

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I'd like to see this for 1.0. I think we should use the new task/new event icons for the dialog, but I'm open to using different icons.

In any case, we need the icons as xpm and ico to cover windows and unix.

Comment 2

[Markus Adrario [:Taraman]]

Attachment: Icons for the event-dialog

Like so?

Comment 3

[Markus Adrario [:Taraman]]

Attachment: .ico and .xpm Files for the event-dialog

And here are the corresponding Binary Files

Comment 4

[Stefan Sitter [:ssitter]]

If the New Event icon is reused there should be a different one for win32/linux and macosx that matches the theme:
http://mxr.mozilla.org/comm-central/source/calendar/base/themes/pinstripe/toolbar-large.png
http://mxr.mozilla.org/comm-central/source/calendar/base/themes/winstripe/toolbar-large.png

Maybe win32 only: The ico-file should contain both small and large resolutions to ensure that it looks good in dialog title (as shown on the screenshot) and that it looks good in the dialog shown when using Alt+Tab to switch applications.

Comment 5

[Markus Adrario [:Taraman]]

Attachment: Event-Dialog Icons V2

Thx for the comment Stefan,

I checked how the Alarm Dialog Icon is made.
In the Pinstripe theme there is no icon for that one. Since I don't own a mac, I don't know how the Icons should look there and if the Window-Icon works the same as in Windows. Perhaps any Mac-User could help me there.

The Windows-Icon for the Alarm-Dialog contains 4 Images, but two of them are just the same but with a white line at the bottom. So I decided to include a large and a small one with 24 bpp each.

Comment 6

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 376287 [details]
Event-Dialog Icons V2

looks good

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 376287 [details]
Event-Dialog Icons V2

I was wondering this too back when I made the alarm icon. Now here's the clue: Mac doesn't have window icons ;-)

Next time, please create a hg patch with the binaries. if you make sure your .hgrc has:

[defaults]
diff= -U 8 -p
qdiff=--git -U 8

[diff]
git = 1

And then hg add the files as usual, you'll get a git-style patch that contains the binary files, which I just need to apply to my tree when checking in :-)

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Need to run, checkin later.

Comment 9

[Stefan Sitter [:ssitter]]

> The Windows-Icon for the Alarm-Dialog contains 4 Images, but two of
> them are just the same but with a white line at the bottom. So I decided
> to include a large and a small one with 24 bpp each.

Most probably one image is for systems that support alpha transparency (WinXP and newer) and one for system that don't support alpha transparency (Win2000 and older)

Don't you need to package the new files in .jar or packages-static too?

Comment 10

[Markus Adrario [:Taraman]]

The Makefile states:

# Copy the window icons into the correct directory
79 libs:: $(addprefix themes/winstripe/icons/, $(addsuffix $(ICON_SUFFIX), $(WINDOW_ICONS)))
80         $(INSTALL) $^ $(FINAL_TARGET)/chrome/icons/default

That should package all the icons, right?

Comment 11

[Markus Adrario [:Taraman]]

Attachment: The patch for the above icons

Comment 12

[Stefan Sitter [:ssitter]]

From a quick search I'd say that at least the following places needs to be updated too:
http://mxr.mozilla.org/comm-central/source/calendar/base/Makefile.in#62
http://mxr.mozilla.org/comm-central/source/calendar/installer/windows/packages-static#283

Comment 13

[gekacheka]

Q1. Do you have the ability to remove the green '+' sign from the icon?  '+'
seems to mean "create a new one", so the '+' seems inappropriate for editing
previously created events, or viewing read-only events.

Q2. Should the same icon be used for the event-summary-dialog?  
[One way to see the event-summary-dialog is to make calendar a read-only (in
calendar list, edit the calendar's properties via context menu) and open or
edit or view properties of an event in the read-only calendar.]
If so, need to copy it to calendar-summary-dialog.{ico,xpm}

Q3. Should a different icon be used for the task dialog?  The new-task toolbar
icons are different from the new-event icons, so it seems like for consistency
the dialog icons should also be different.  If so, need to extract the task
icon also, creating calendar-task-dialog.{ico.xpm}

Comment 14

[Stefan Sitter [:ssitter]]

(In reply to comment #13)
> Q3. Should a different icon be used for the task dialog?

I don't think it's possible. There is only one dialog that acts as New/Edit Event/Task dialog by showing or hiding its content.

Comment 15

[gekacheka]

(In reply to comment #14)
> I don't think it's possible. There is only one dialog that acts as New/Edit
> Event/Task dialog by showing or hiding its content.

As I mentioned in the description (comment 0) it may be possible by having two xul dialog files, one for each dialog, and a common xul overlay that provides their content.  I was thinking that could wait until later if dialogs all use the app icon, but if they use event/task specific icons then maybe it would be better to do it now.

Comment 16

[Markus Adrario [:Taraman]]

(In reply to comment #13)
Q1: Should be no problem. I had somehow in mind its only the "new Event/Task
Dialog" I was obviously misled on this.

Q2: I think its a goog Idea.
    Should we include any other dialogs in this Bug, or open new bugs for that?

Q3: I agree with Stefan here. See
https://developer.mozilla.org/en/Window_icons.

to comment #12:
I will check and submit a changed patch if needed (and I think you are right.)

Comment 17

[Markus Adrario [:Taraman]]

(In reply to comment #15)
Taking the App-Icon is then the better solution for now.
I could work on splitting the two dialogs, but since this is set blocking for 1.0 and I'm not sure how fast I could manage this task.

If anyone else can split this quite fast, and has time to do that this would be the more complete solution and I would be happy to provide the icon-files.

Comment 18

[Stefan Sitter [:ssitter]]

> Taking the App-Icon is then the better solution for now.

This means one for Lightning, one for Sunbird (official branding), and one for Sunbird (common branding)? In case of Lightning create a new one or take the one from the parent application at runtime (Thunderbird, SeaMonkey, IceDove)?

> it may be possible by having two xul dialog files, 
> one for each dialog, and a common xul overlay

I don't think that splitting the dialog into multiple files is worth the effort just to get a icon. Drawback is that all source code must be updated to decide on opening either the task or event dialog.

My opinion: Keep It Simple, Stupid.

Comment 19

[Markus Adrario [:Taraman]]

> In case of Lightning create a new one or take the
> one from the parent application at runtime (Thunderbird, SeaMonkey, IceDove)?
I think we cannot change during runtime. See description of this bug. So if we don't want to have specific builds for all the parent Applications we would have to create a lightning icon.
Since Lightning is integrated into the Application and does not have a window itself this would be somewhat confusing.

So the nicest solution would be to have an own icon for the dialogs.
I also think that Splitting the event-Task Dialog would make no sense. The code is so similar, that it would create nearly identical files.
So maybe we can find an Icon which suits both cases.

Comment 20

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to comment #10)
> The Makefile states:
> 
> # Copy the window icons into the correct directory
> 79 libs:: $(addprefix themes/winstripe/icons/, $(addsuffix $(ICON_SUFFIX),
> $(WINDOW_ICONS)))
> 80         $(INSTALL) $^ $(FINAL_TARGET)/chrome/icons/default
> 
> That should package all the icons, right?

Not quite, this copies all icons in the variable WINDOW_ICONS, so you need to add the filenames (without extension) to that variable. Please separate the names with newlines, i.e

WINDOW_ICONS = a \
               b \
               c \
               $(NULL)

(In reply to comment #14)
> (In reply to comment #13)
> > Q3. Should a different icon be used for the task dialog?
> 
> I don't think it's possible. There is only one dialog that acts as New/Edit
> Event/Task dialog by showing or hiding its content.

There may be only one dialog, but please try changing the id using document.documentElement.setAttribute("id", "calendar-" + itemType + "-dialog"); or similar. The same needs to be done for the summary dialog, so you might want to create a common function in calendar-dialog-utils.js.

I'm not sure if the icon is updated automatically. If this fails we could go with gekachecka's idea in comment #15, but I agree that should happen in a new bug if possible.

Markus, please update packages-static as noted in comment #12.


(In reply to comment #19)
> > In case of Lightning create a new one or take the
> > one from the parent application at runtime (Thunderbird, SeaMonkey, IceDove)?
> I think we cannot change during runtime. See description of this bug. So if we
> don't want to have specific builds for all the parent Applications we would
> have to create a lightning icon.
> Since Lightning is integrated into the Application and does not have a window
> itself this would be somewhat confusing.
If we decide to use the app icon, I'd suggest to just use the nightly app icon for all cases.


> 
> So the nicest solution would be to have an own icon for the dialogs.
> I also think that Splitting the event-Task Dialog would make no sense. The code
> is so similar, that it would create nearly identical files.
> So maybe we can find an Icon which suits both cases.
Just to explain: We don't need nearly identical files. The idea was to create two minimal files with the id being item dependant:

<window id="calendar-event-dialog"><box flex="1" id="calendar-item-dialog-content"/></window>

and then using an overlay to insert the event dialog content:

<overlay id="item-dialog-overlay">
  <box id="calendar-item-dialog-content">
     <!-- insert current event dialog content here -->
  </box>
</overlay>

As said, I don't think we should go this route for now.


So to summarize things:
* Try to remove the + from the icon
  (ping me if this doesn't work out, I'll contact the original author)
* Make the summary dialog also use the icon
* Try to modify the dialog ID to change the icon for a task dialog.
 - If fails, just use the nightly calendar app icon for all dialogs and
   file a new bug for using app specific icons somehow.
* Update packages-static and the Makefile

Comment 21

[Markus Adrario [:Taraman]]

Attachment: Icons V3

(In reply to comment #20)
> So to summarize things:
> * Try to remove the + from the icon
Done. I have attached the new icons as a preview.

> * Make the summary dialog also use the icon
> * Try to modify the dialog ID to change the icon for a task dialog.
I wouldn't have thought it's this easy, but it works.
Since its just 3 additional lines of code of which only one could be placed in a seperate function, i will keep it in the respective .js files of the dialogs.

> * Update packages-static and the Makefile
Done.

Comment 22

[Stefan Sitter [:ssitter]]

(In reply to comment #20)
> There may be only one dialog, but please try changing the id
> using document.documentElement.setAttribute("id", "calendar-" +
> itemType + "-dialog"); or similar.

Some concerns: What is the consequence for extensions/themes that overlay/style the dialog? Does overlaying works correctly if the ID is changed at runtime? Does this effect ID related toolkit functions, e.g. restoring of dialog size and position? Do calendar or toolkit functions rely on the windowtype "Calendar:EventDialog" instead of the ID? And don't forget to update the source code locations that do something like |getElementById("calendar-event-dialog")|.

Comment 23

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to comment #22)
> (In reply to comment #20)
> > There may be only one dialog, but please try changing the id
> > using document.documentElement.setAttribute("id", "calendar-" +
> > itemType + "-dialog"); or similar.
> 
> Some concerns: What is the consequence for extensions/themes that overlay/style
> the dialog?
> Does overlaying works correctly if the ID is changed at runtime?
This definitely needs to be tested. Markus, could you look into this? I believe that we are fine, since the overlaying happens before the "load" event and at that point in time the id is still fixed. What will not work is using document.loadOverlay afterwards, but I think if we document that as a comment just above the document element, everything is fine.

> Does this effect ID related toolkit functions, e.g. restoring of dialog size
> and position?
Unfortunately, they rely on the id. But maybe its not so bad afterall, that task windows have a separate id. this would solve the problem that the task dialog is cut off because the event dialog has less width and the width is persisted.

>  Do calendar or toolkit functions rely on the windowtype
> "Calendar:EventDialog" instead of the ID?
Which functions do you have in mind?

> And don't forget to update the source
> code locations that do something like
> |getElementById("calendar-event-dialog")|.
We should use document.documentElement instead, but I agree this should be taken care of.

Comment 24

[Markus Adrario [:Taraman]]

Concerning the concerns from previous posts:


> Does overlaying works correctly if the ID is changed at runtime?
Overlaying works with both dialogs (event and summary).

> What will not work is using document.loadOverlay afterwards, but I think if we
> document that as a comment just above the document element, everything is fine.
I agree, since there should be no need to call loadOverlay again. This comment would go just above the <dialog> tag in the .xul, right?

> Unfortunately, they rely on the id. But maybe its not so bad afterall, that
> task windows have a separate id. this would solve the problem that the task
> dialog is cut off because the event dialog has less width and the width is
> persisted.
unfortunately, the size is restored before onLoad, where I change the id, so the task-dialog is still too small. But maybe we could use the onLoad function to set a minimum size?

> > And don't forget to update the source
> > code locations that do something like
> > |getElementById("calendar-event-dialog")|.
> We should use document.documentElement instead, but I agree this should be
> taken care of.
Right now I'm using "document.activeElement". I would liked to have something more descriptive like "document.dialog", but this is not available. Adding a property with self reference for that is only possible in xml, or am I wrong about that?
Fortunately there are very few locations, where these are called. [1]
Same for the summary dialog, where its only 1 call.

> What is the consequence for extensions/themes that overlay/style
the dialog?
The only place where this id is referenced is in [2], where we are fine when adding #calendar-task-dialog.
None of the skin-.css files reference the id, so there should be no problem.

On my machine testing showed everything works well.
I will have a patch ready soon.

[1]:
http://mxr.mozilla.org/comm-central/search?string=getElementById(%22calendar-event-dialog%22)&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

[2]:
http://mxr.mozilla.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog.css#39

Comment 25

[Markus Adrario [:Taraman]]

One more question:
The Summary Dialog has the id "calendar-summary-dialog".
So to have it named descriptive I would change that to "calendar-task-summary-dialog" and "calendar-event-summary-dialog".

Since this would change the id in any case, I would prefer to use "calendar-task-dialog" and "calendar-event-dialog" since that way we don't have to have 2 copies of the same icons with different names.
Since the dialogs are independant objects/instances there should also not be any name-collisions.

Sure it would not be strict naming, but it saves resources.

Any concerns about that, I didn't come up with?

Comment 26

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to comment #24)
> > Does overlaying works correctly if the ID is changed at runtime?
> Overlaying works with both dialogs (event and summary).
I didn't mean the summary dialog.
* Create an overlay like:
  <overlay>
    <dialog id="calendar-event-dialog">
      <label>mything</label>
    </dialog>
  </overlay>
* let that overlay calendar-event-dialog.xul
* now open a task window. The id is changed to calendar-task-dialog
  -> Is the label also shown correctly?


> I agree, since there should be no need to call loadOverlay again. This comment
> would go just above the <dialog> tag in the .xul, right?
Right.

> unfortunately, the size is restored before onLoad, where I change the id, so
> the task-dialog is still too small. But maybe we could use the onLoad function
> to set a minimum size?
I've tried this in a different bug, but ran into many problems, including bug 357725.


> Right now I'm using "document.activeElement". 
document.activeElement is actually an IE property? I still think you should go with document.documentElement, which always references the root element of the document.

(In reply to comment #25)
> Sure it would not be strict naming, but it saves resources.
> 
> Any concerns about that, I didn't come up with?
I think it doesn't hurt to copy the icon twice. We should avoid using the same id to make it more transparent to extension authors. They may use a single css file for both dialogs and then run into problems since they only have one id to work with.

Comment 27

[Markus Adrario [:Taraman]]

Attachment: Patch V3

Overlaying is tested and works.

The id of the dialog is restored in the dispose() function, because else the persisted values won't be stored in case of dialog-id-renaming.

Comment 28

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 377487 [details] [diff] [review]
Patch V3

Just a general question: Why do we need to reset the dialog id?

>+function resetDialogId(aDialog) {
>+    var id = aDialog.getAttribute("originalId");
use let instead.

>+    if(id != "")
>+        aDialog.setAttribute("id", id);
Add {} even for one-line ifs.

>+    aDialog.removeAttribute("originalId");
> }


>+    if (isEvent(item)) {
Please use cal.isEvent(), we want to transition using calUtils.jsm instead of calUtils.js as time passes.

>+        setDialogId(document.documentElement, "calendar-event-summary-dialog");
>+    } else {
Please use cal.isToDo() here too. This will obviously change when bug 185537 is fixed, but in that bug we want to make the if/else blocks with isToDo/isEvent more explicit.

>+        setDialogId(document.documentElement, "calendar-task-summary-dialog");
>     }



>+    // set the dialog-id to enable the right window-icon to be loaded.
>+    if (!isEvent(item)) {
cal.isEvent().


r=philipp with that fixed, I'd appreciate a new patch ready for checkin.

Comment 29

[Markus Adrario [:Taraman]]

(In reply to comment #28)
> Just a general question: Why do we need to reset the dialog id?
Because the persist function uses the id to store position and size. If we don't reset, the values don't get stored.

> r=philipp with that fixed, I'd appreciate a new patch ready for checkin.
The patch is on the way...

Comment 30

[Markus Adrario [:Taraman]]

Attachment: Patch V4

Incorporated Comments form Comment #28
and added Comments in .xul-files as mentioned in Comment #23

Comment 31

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 378117 [details] [diff] [review]
Patch V4

looks good to me

Comment 32

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/b86ec2cc557a>

-> FIXED

Comment 33

[Damian Szczepanik]

verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.9pre) Gecko/20100305 Calendar/1.0b2pre

Comment 34

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".