515337 - Dialog specific files should be moved into /calendar/base/content/dialogs/ Part 2

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Markus Adrario [:Taraman]]

This is a followup bug of Bug 443752.

Description:
Not all files wich are related to dialogs are in the folder where the dialog-related files should be. (/calendar/base/content/dialogs/)

The related comment from the previos bug:
> 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 1

[Markus Adrario [:Taraman]]

Attachment: Patch V1

Only thing I'm not sure about is the placement of calendar-alarm-widget.xml
From the name it should be in "widgets", so I placed it there.

Comment 2

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

Comment on attachment 423673 [details] [diff] [review]
Patch V1

>diff --git a/calendar/base/content/calErrorPrompt.xul b/calendar/base/content/dialogs/calErrorPrompt.xul
>diff --git a/calendar/base/content/migration.js b/calendar/base/content/dialogs/migration.js
>diff --git a/calendar/base/content/migration.xul b/calendar/base/content/dialogs/migration.xul
I'd prefer following the usual scheme in base/content, i.e calendar-error-prompt.xul


>diff --git a/calendar/base/content/calendar-alarm-snooze-popup.js b/calendar/base/content/dialogs/calendar-alarm-snooze-popup.js
>diff --git a/calendar/base/content/calendar-alarm-snooze-popup.xul b/calendar/base/content/dialogs/calendar-alarm-snooze-popup.xul
>diff --git a/calendar/base/content/calendar-alarm-widget.xml b/calendar/base/content/widgets/calendar-alarm-widget.xml
Go ahead and leave these files where they are, they will be moved/deleted by bug 407915.


>diff --git a/calendar/base/content/calendar-subscriptions-list.xml b/calendar/base/content/dialogs/calendar-subscriptions-list.xml
These are rather widgets, please move them to base/content/widgets


>diff --git a/calendar/base/content/import-export.js b/calendar/base/content/dialogs/import-export.js
Strictly spoken, this file does open dialogs, but I think its better placed in base/content since its not the .js file for a xul dialog.


>diff --git a/calendar/locales/en-US/chrome/calendar/calendarCreation.dtd b/calendar/locales/en-US/chrome/calendar/calendarCreation.dtd
>deleted file mode 100644
Maybe I missed something, but why are you deleting this file? Don't we need it for the calendar creation wizard?


>diff --git a/calendar/locales/en-US/chrome/calendar/calendar-alarms.properties b/calendar/locales/en-US/chrome/calendar/dialogs/calendar-alarms.properties
>rename from calendar/locales/en-US/chrome/calendar/calendar-alarms.properties
>rename to calendar/locales/en-US/chrome/calendar/dialogs/calendar-alarms.properties

I'm not sure how to proceed on all these locale renamings. We might want to notify localizers before doing this, since without warning to them it might look like we have a bunch of new files (in case they miss the fact that the files were just renamed).

sipaq, any ideas on this?

Marking r+ if you leave out the locales/ changes and answer my calendarCreation question.

Comment 3

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

Sipaq, see previous comment

Comment 4

[Markus Adrario [:Taraman]]

Attachment: Patch V2

This patch takes care of the notes

>>diff --git a/calendar/base/content/migration.xul b/calendar/base/content/dialogs/migration.xul
>I'd prefer following the usual scheme in base/content, i.e
>calendar-error-prompt.xul
Since the rename requires some code-changes in other files, please check that I got them all.

>>diff --git a/calendar/locales/en-US/chrome/calendar/calendarCreation.dtd b/calendar/locales/en-US/chrome/calendar/calendarCreation.dtd
>>deleted file mode 100644
>Maybe I missed something, but why are you deleting this file? Don't we need it
>for the calendar creation wizard?
Oops, must have gotten accidentially deleted. Thanks for noticing...

Locales are out for now.

Comment 5

[Simon Paquet [:sipaq]]

(In reply to comment #2)
> (From update of attachment 423673 [details] [diff] [review])
> >diff --git a/calendar/locales/en-US/chrome/calendar/calendar-alarms.properties b/calendar/locales/en-US/chrome/calendar/dialogs/calendar-alarms.properties
> >rename from calendar/locales/en-US/chrome/calendar/calendar-alarms.properties
> >rename to calendar/locales/en-US/chrome/calendar/dialogs/calendar-alarms.properties
> 
> I'm not sure how to proceed on all these locale renamings. We might want to
> notify localizers before doing this, since without warning to them it might
> look like we have a bunch of new files (in case they miss the fact that the
> files were just renamed).
> 
> sipaq, any ideas on this?

I don't think that we should rename the locale files at all. I see the need 
for doing the renaming on the code side, but here the upside is way smaller than the downside IMO.

Comment 6

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

Comment on attachment 435416 [details] [diff] [review]
Patch V2

Looks good, r=philipp

Comment 7

[Markus Adrario [:Taraman]]

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

-> FIXED