Closed Bug 458368 Opened 16 years ago Closed 16 years ago

Move and integrate calendar/prototypes to calendar/base

Categories

(Calendar :: General, defect)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mschroeder, Assigned: mschroeder)

Details

Attachments

(2 files, 4 obsolete files)

      No description provided.
Assignee: nobody → mschroeder
Status: NEW → ASSIGNED
Attached patch WIP patch (obsolete) — Splinter Review
Just the code changes. Merging, renaming, and moving of files still missing.
Attached patch Patch v1a (obsolete) — Splinter Review
This patch contains all code changes (also the merge of sun-calendar-event-dialog.dtd and calendar-event-dialog.dtd), but no (real) renames and moves.
Attachment #341700 - Attachment is obsolete: true
Attachment #341840 - Attachment is patch: true
Attachment #341840 - Attachment mime type: application/octet-stream → text/plain
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #341840 - Attachment is obsolete: true
Attachment #342782 - Flags: review?(daniel.boelzle)
Comment on attachment 342782 [details] [diff] [review]
Patch v1

>diff --git a/calendar/prototypes/themes/pinstripe/timezone_0h.png b/calendar/base/themes/pinstripe/pinstripe/timezone_0h.png
>rename from calendar/prototypes/themes/pinstripe/timezone_0h.png
>rename to calendar/base/themes/pinstripe/pinstripe/timezone_0h.png

I can't test the patch (the jar script complains about missing files when building): The timezone chooser files have been moved into .../pinstripe/pinstripe/...

r- for now
Attachment #342782 - Flags: review?(daniel.boelzle) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #342782 - Attachment is obsolete: true
Attachment #342803 - Flags: review?(daniel.boelzle)
It's good to move all the prototypes stuff away in one move but I think we should create a base/content/dialogs/ and and respectively a base/themes/pinstripe|winstriype/dialogs folder for all our dialogs. calendar/base /content has become a container for nearly everything. While you are at it you could then also move the recurrence dialog files to that new destination.
I also find that the naming of our files could be shortened: If a dialog is located in calendar/base/content/... it is redundant to have a "calendar-" substring in the filename.
(In reply to comment #6)
> It's good to move all the prototypes stuff away in one move but I think we
> should create a base/content/dialogs/ and and respectively a
> base/themes/pinstripe|winstripe/dialogs folder for all our dialogs.

IMO this is something we should handle in a second bug or patch.
Comment on attachment 342803 [details] [diff] [review]
Patch v2

>diff --git a/calendar/locales/en-US/chrome/calendar/calendar-event-dialog.dtd b/calendar/locales/en-US/chrome/calendar/calendar-event-dialog.dtd
>--- a/calendar/locales/en-US/chrome/calendar/calendar-event-dialog.dtd
>+++ b/calendar/locales/en-US/chrome/calendar/calendar-event-dialog.dtd
>@@ -1,46 +1,54 @@
>
> [...]
>
>-   - The Original Code is OEone Calendar Code, released October 31st, 2001.
>+   - The Original Code is Sun Microsystems code.
>    -
>-   - The Initial Developer of the Original Code is
>-   - OEone Corporation.
>-   - Portions created by the Initial Developer are Copyright (C) 2001
>+   - The Initial Developer of the Original Code is Sun Microsystems.
>+   - Portions created by the Initial Developer are Copyright (C) 2006
>    - the Initial Developer. All Rights Reserved.
>    -
>    - Contributor(s):
>-   -   Cédric Corazza <cedric.corazza@wanadoo.fr>
>+   -   Michael Buettner <michael.buettner@sun.com>
>+   -   Philipp Kewisch <mozilla@kewis.ch>
>+   -   Stefan Sitter <ssitter@gmail.com>
>+   -   Martin Schroeder <mschroeder@mozilla.x-home.org>
>+   -   Hubert Gajewski <hubert@hubertgajewski.com>, Aviary.pl
>+   -   Fred Jendrzejewski <fred.jen@web.de>
>+   -   Cedric Corazza <cedric.corazza@wanadoo.fr>

While it is correct that you bring over the contributors to sun-calendar-event-dialog.dtd, you should not change the "Original Code" and "Initial Developer" sections. This file was originally created by OEone developers and that still remains the case. Please remove the Sun Microsystems attributions.

>diff --git a/calendar/locales/jar.mn b/calendar/locales/jar.mn
>--- a/calendar/locales/jar.mn
>+++ b/calendar/locales/jar.mn
>@@ -18,12 +18,12 @@ calendar-@AB_CD@.jar:
>
> [...]
>
>-    locale/@AB_CD@/calendar/sun-calendar-event-dialog.dtd	(%chrome/prototypes/sun-calendar-event-dialog.dtd)
>-    locale/@AB_CD@/calendar/sun-calendar-event-dialog.properties  (%chrome/prototypes/sun-calendar-event-dialog.properties)
>-    locale/@AB_CD@/calendar/calendar-invitations-dialog.dtd (%chrome/prototypes/calendar-invitations-dialog.dtd)
>-    locale/@AB_CD@/calendar/calendar-subscriptions-dialog.dtd  (%chrome/calendar/calendar-subscriptions-dialog.dtd)
>+    locale/@AB_CD@/calendar/calendar-event-dialog.dtd (%chrome/calendar/calendar-event-dialog.dtd)
>+    locale/@AB_CD@/calendar/calendar-event-dialog.properties (%chrome/calendar/calendar-event-dialog.properties)
>+    locale/@AB_CD@/calendar/calendar-invitations-dialog.dtd (%chrome/calendar/calendar-invitations-dialog.dtd)
>+    locale/@AB_CD@/calendar/calendar-subscriptions-dialog.dtd (%chrome/calendar/calendar-subscriptions-dialog.dtd)

We already include calendar-event-dialog.dtd in line 8 of this file. So there's no need to include it a second time.
Oh, and IMO it would be great if you could move resources/content/mouseoverPreviews.js to base/content/ as well, while you are touching it.
Personally I'd like to see files in resources/ stay there until they are overworked. There are a couple of things that could be improved, w.r.t style and coding constructs. In the case of mouseoverPreviews.js, this file is going away in bug 366680 as soon as I have time to resurrect those patches.
(In reply to comment #7)
> (In reply to comment #6)
> > It's good to move all the prototypes stuff away in one move but I think we
> > should create a base/content/dialogs/ and and respectively a
> > base/themes/pinstripe|winstripe/dialogs folder for all our dialogs.
> 
> IMO this is something we should handle in a second bug or patch.

Since base/content is already quite crowded, I second Berend's proposal. We should move the prototype dialogs to their final destination in this bug, all others be left out resp. subject to follow-up bugs, of course.
Attached patch Patch v3Splinter Review
* incorporated Simon's comments
* created dialog directories in base/themes/*instripe & base/content
Attachment #342803 - Attachment is obsolete: true
Attachment #342907 - Flags: review?(daniel.boelzle)
Attachment #342803 - Flags: review?(daniel.boelzle)
Attachment #342907 - Flags: review?(philipp)
Attachment #342907 - Flags: review?(daniel.boelzle)
Attachment #342907 - Flags: review+
Comment on attachment 342907 [details] [diff] [review]
Patch v3

You should put calendar-invitations-manager.js into base/content/ instead of base/content/dialogs/. Apart from that the patch looks good and works for me.

r=dbo with that fixed

Requesting 2nd review from Philipp.
Comment on attachment 342907 [details] [diff] [review]
Patch v3

Looks good, r=philipp

I didn't have a chance to do real testing, but from looking at the applied patch all is good.

I would appreciate if you could go ahead and align the jar files you are changing to have the "(" at column 60 and make the file use #filter substitution and @THEME@, since my sunbird.jar patch will hopelessly bitrot when this is checked in.
Attachment #342907 - Flags: review?(philipp) → review+
Addressed Daniel's and Philipp's comments.
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/62876a69cb35>

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.