443752 - Dialog specific files should be moved into /calendar/base/content/dialogs/

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Berend Cornelius]

As Philipp proposed in Bug 396819 comment #11 (Event Summary Dialog doesn't show important information to user) we should move all source code related to dialogs in a special folder like calendar/base/content/dialogs.

Comment 1

[Markus Adrario [:Taraman]]

Some things I stumbled about:
The Publish Dialog is confusing me:
There is
calendar/base/content/calendar-publish-dialog.xul &.js

and
calendar/resources/content/publishDialog.xul & .js

The first one does not seem to be used anymore:
http://mxr.mozilla.org/comm-central/search?string=calendar-publish-dialog.xul

in calendar/resources/content/ there is also the PrintDialog.

Two questions arise:
1. What is the currently used PublishDialog and can the other files be deleted?
2. why is some code in the /resources/ directory rather than /base/ ?

Comment 2

[Martin Schröder [:mschroeder]]

(In reply to comment #1)
> 1. What is the currently used PublishDialog and can the other files be deleted?

The one in /resources/content! The other publish dialog can be removed imo. I work on an updated and cleaned up version of this dialog in bug 305526.


> 2. why is some code in the /resources/ directory rather than /base/ ?

Everything shared between Sunbird and Lightning should live in /base, other files should be moved into /lightning or /sunbird. The resources directory is still a relict from the times when there was the Mozilla Calendar extension.

Comment 3

[Markus Adrario [:Taraman]]

Attachment: Patch V1.0

Moving all dialog-related files from calendar/base/content to calendar/base/content/dialogs.

The obsolete calendar-Publish-Dialog files are removed.

The Lightning folder does not have any dialogs.
The Sunbird folder has an "About"-Dialog, but there are so few files, that another subfolder won't bring a better overview.

>The resources directory is still a relict from the times when
>there was the Mozilla Calendar extension.
This could be straightened out in a seperate bug.

Comment 4

[Martin Schröder [:mschroeder]]

Comment on attachment 386906 [details] [diff] [review]
Patch V1.0

You should also move the css files which are specific to dialogs. The patch looks good so far. I'll just r- it to signal it is not complete, and wait for the next iteration with css files considered. :)

Comment 5

[Markus Adrario [:Taraman]]

Attachment: Patch V2 including .css files

This is the patch including the .css-files in the Themes-directories.

calendar-event-dialog.css from the content directory is split up and incorporated into the calendar-event-dialog.css files in the themes dir.
The one in content/dialogs now only has -moz-binding entries.

Comment 6

[Martin Schröder [:mschroeder]]

Comment on attachment 390195 [details] [diff] [review]
Patch V2 including .css files

r=mschroeder with following remarks:

> --- a/calendar/base/content/dialogs/calendar-event-dialog.css
> +++ b/calendar/base/content/dialogs/calendar-event-dialog.css
[...]
>  daypicker-weekday {
>      -moz-binding: url(chrome://calendar/content/calendar-daypicker.xml#daypicker-weekday);
>      -moz-user-focus: normal;
>      margin-top: 2px;
>  }
>  
>  daypicker-monthday {

Could you move the margin declaration for those to the theme?


The following problems with the CSS changes also apply to the winstripe version:

> --- a/calendar/base/themes/pinstripe/calendar-event-dialog.css
> +++ b/calendar/base/themes/pinstripe/dialogs/calendar-event-dialog.css
[...]
> -.status-icon {
> -  margin: 0 3px;
> -  display: none;
> -  list-style-image: url("chrome://calendar/skin/calendar-event-dialog-attendees.png");

You should leave the common parts of the rules for the icons in this general one. Also have a look at https://developer.mozilla.org/en/Writing_Efficient_CSS#Rely_on_inheritance! regarding optimizing CSS and inheritance. The display property shouldn't be needed because "inline" is the default value, and the ".status-icon" rule is always overridden.

[...]
> +#addressingWidget {
> +    -moz-user-focus: none;
> +}
> +
> +
> +#typecol-addressingWidget {
> +    min-width: 9em;
> +    border-right: 1px solid #CACAFF;
> +}

Please, remove the empty line.


I would recommend to introduce a general rule for class ".role-icon" with properties "margin" and "list-style-image". Also, there is no difference between normal and disabled ".role-icon". They use the same image, but code-wise there is a difference, so maybe we need another set of icons, or we can remove the code setting/removing the 'disabled' attribute? I don't know what's the right solution here.


> +.role-icon[role="CHAIR"] {
> +    margin: 0 3px;
> +    list-style-image: url(chrome://calendar/skin/calendar-event-dialog-attendees.png);
> +    -moz-image-region: rect(0px 201px 16px 180px);
> +}
> +.role-icon[role="CHAIR"][disabled="true"] {
> +    margin: 0 3px;
> +    list-style-image: url(chrome://calendar/skin/calendar-event-dialog-attendees.png);
> +    -moz-image-region: rect(0px 201px 16px 180px);
> +}
> +
> +.role-icon[role="CHAIR"] {
> +    margin: 0 3px;
> +    list-style-image: url(chrome://calendar/skin/calendar-event-dialog-attendees.png);
> +    -moz-image-region: rect(0px 201px 16px 180px);
> +}

Please, remove the duplicate rule for ".role-icon[role="CHAIR"]".


Some additional remarks:
We seem to have a -moz-binding in the themes for calendar-alarm-dialog.css which should be moved back to content. There are also some probably missed dialogs: calendar-alarm-snooze-popup.xul/js, calendar-subscription-list.xml, calendar-occurrence-prompt.xul, calErrorPrompt.xul, migration.xul/js and calendar-alarm-widget.xml. The patch is large enough, so this can go into a new bug & patch. :)

Comment 7

[Markus Adrario [:Taraman]]

Attachment: Patch V2.1

>> --- a/calendar/base/themes/pinstripe/calendar-event-dialog.css
>> +++ b/calendar/base/themes/pinstripe/dialogs/calendar-event-dialog.css
[...]
>> -.status-icon {
>> -  margin: 0 3px;
>> -  display: none;
>> -  list-style-image: url("chrome://calendar/skin/calendar-event-dialog-attendees.png");

>You should leave the common parts of the rules for the icons in this general
>one. Also have a look at
These parts were duplicates in both .css files. combined and corrected them.

>I would recommend to introduce a general rule for class ".role-icon" with
>properties "margin" and "list-style-image".
Also fixed. 

Same applies to .left-icon & .right-icon.
I changed that too.

Since there have been quite some changes in the .css now, please have a look at it again.

For the additional comments I will file a spinoff-bug when this is checked in.

Comment 8

[Martin Schröder [:mschroeder]]

Comment on attachment 395668 [details] [diff] [review]
Patch V2.1

r=mschroeder with a followup bug filed for the missing dialogs and following change reverted:

>--- a/calendar/base/themes/pinstripe/calendar-alarm-dialog.css
>+++ b/calendar/base/themes/pinstripe/dialogs/calendar-alarm-dialog.css
>@@ -34,7 +34,7 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>-/* Bindings */
>+ /* Bindings */
> calendar-alarm-widget {
>     -moz-binding: url(chrome://calendar/content/calendar-alarm-widget.xml#calendar-alarm-widget);
> }

Comment 9

[Markus Adrario [:Taraman]]

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

-> FIXED

Comment 10

[Markus Adrario [:Taraman]]

Followup bug is Bug 515337.

Comment 11

[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".