Closed Bug 386432 Opened 17 years ago Closed 16 years ago

Unify front-end code of Lightning and Sunbird

Categories

(Calendar :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sipaq, Assigned: sipaq)

References

Details

Attachments

(4 files)

With the patch of bug 371916 going in, we should try to unify the front-end code of Lightning (currently living in various overlays) and Sunbird (currently living in calendar.xul and various .inc files) as much as possible to avoid code duplication and higher maintenance work.
Totally agreed. This should also include e.g. the task list. The existing task list in Sunbird and the task list proposal from [http://wiki.mozilla.org/Calendar:Mail_View_Integration] look very similar. Instead of writing something new for Lightning the task list from Sunbird should be used and extended as necessary. See also Bug 372830.
This patch consolidates all the scripts used by messenger-overlay-sidebar.xul and calendar.xul in calendar/base/content/calendar-scripts.inc

Lightning-specific scripts are moved into calendar/lightning/content/lightning-scripts.inc, while Sunbird-specific scripts are moved into calendar/sunbird/base/content/sunbird-scripts.inc
Attachment #294865 - Flags: review?(michael.buettner)
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
Comment on attachment 294865 [details] [diff] [review]
[checked in] Consolidate scripts - v1

Perfect step towards a better organized code base, r=mickey.
Attachment #294865 - Flags: review?(michael.buettner) → review+
Patch checked in into HEAD and MOZILLA_1_8_BRANCH. I took to the liberty to reflect the latest script-related changes (addition of calendar-ui-utils.js and calendar-offline.js) in calendar-scripts.inc and sunbird-scripts.inc when checking in.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Ugh, this is obviously not totally fixed. Reopening
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: bugzilla → nobody
Status: REOPENED → NEW
I've checked in a fix for the bustage from this patch.  The tinderboxen will reset soon.

The issue here is that the #include statement for calendar-scripts.inc should be a relative path not an absolute one.

I'll attach a diff for that just so you can see it easier.
 
Comment on attachment 294865 [details] [diff] [review]
[checked in] Consolidate scripts - v1

Couldn't diff since I already checked in the fix - Doh. 
Here is the change:

>Index: calendar/lightning/content/messenger-overlay-sidebar.xul
>===================================================================
>+#include /calendar/base/content/calendar-scripts.inc
This should be:
#include ../../base/content/calendar-scripts.inc

>Index: calendar/sunbird/base/content/calendar.xul
>===================================================================
>+#include /calendar/base/content/calendar-scripts.inc
And this one should be:
#include ../../../base/content/calendar-scripts.inc

Like I said, I checked in this change to fix the bustage.
The original patch did not address the calendar-scripts.inc line inside of calendar/sunbird/base/content/hiddenWindow.xul.  

I've made that change,and also assumed that the #include sunbird-scripts.inc line is also necessary there as well.  

As I've been fighting this bustage all night, and this omission seems (right now) to only affect mac sunbird builds, I'm going to wait and let Mickey have a chance to review this in the morning.  I've already made a number of unreviewed checkins to try to fix this bustage, and that always makes me uncomfortable.  So, in the case there is some special reason why hiddenWindow was not addressed, or in case it needs special handling of some sort; I'm not going to check this in until it gets reviewed.  That means that the mac tinderboxen are going to burn until morning in Germany.

Mickey, if you R+ this, please check it in for me as I will (hopefully) be asleep.

I'll put a comment on the tinderboxen pointing to this bug in case people are wondering why they're on fire.
Attachment #297483 - Flags: review?(michael.buettner)
(In reply to comment #5)
> Ugh, this is obviously not totally fixed. Reopening

Because of the bustage or because of additional things that should be done?
Target Milestone: --- → 0.8
There are a lot of additional things that can be done:
- context-menu unification
- toolbar unification
- basic UI unification (e.g. the XUL code for displaying the views in the UI)
Attachment #294865 - Attachment description: Consolidate scripts - v1 → [checked in] Consolidate scripts - v1
Comment on attachment 297483 [details] [diff] [review]
[checked in] Original patch didn't address hiddenWindow.xul 

Sorry for forgetting about hiddenWindow.xul in the first place. r=mickey.
Attachment #297483 - Flags: review?(michael.buettner) → review+
patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: NEW → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #297483 - Attachment description: Original patch didn't address hiddenWindow.xul → [checked in] Original patch didn't address hiddenWindow.xul
Status: REOPENED → ASSIGNED
Attachment #297614 - Flags: review?(michael.buettner)
Comment on attachment 297614 [details] [diff] [review]
[checked in] Consolidate toolbar buttons - v1

>+++ calendar/base/content/calendar-toolbar.inc	17 Jan 2008 21:39:17 -0000
>+<?xml version="1.0"?>
This tag identifies xml-files, it's not allowed in this context. Simply get rid of it.

>+<!-- ***** BEGIN LICENSE BLOCK *****
>+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
All other *.inc files don't use xml-comments for the license header.
ile under

>+# the terms of any one of the MPL, the GPL or the LGPL.
>+#
>+# ***** END LICENSE BLOCK ***** -->
The last line swallows the closing comment tag.

The same comments apply to the other *.inc files as well. Without these modifications the relevant xul-files will be invalid. Please just use '#' for the license header, just as all other *.inc files. Apart from that, the patch looks just fine. r=mickey with these comments addressed.
Attachment #297614 - Flags: review?(michael.buettner) → review+
Comment on attachment 297614 [details] [diff] [review]
[checked in] Consolidate toolbar buttons - v1

Patch checked in on HEAD and MOZILLA_1_8_BRANCH with Mickey's review comments addressed.
Attachment #297614 - Attachment description: Consolidate toolbar buttons - v1 → [checked in] Consolidate toolbar buttons - v1
Comment on attachment 297614 [details] [diff] [review]
[checked in] Consolidate toolbar buttons - v1

calendar-toolbar.inc and calendar-sunbird-inc are included twice in calendar.xul now: http://mxr.mozilla.org/seamonkey/search?string=sunbird-toolbar.inc&find=calendar%2F&findi=&filter=&tree=seamonkey
(In reply to comment #16)
> (From update of attachment 297614 [details] [diff] [review])
> calendar-toolbar.inc and calendar-sunbird-inc are included twice in

That should read "calendar-toolbar.inc and sunbird-toolbar.inc are included twice in".
This regressed Bug 414135 making Sunbird unusable.
Depends on: 414135
I checked in a bustage fix for the issue raised in comment 16.
No longer depends on: 414135
Blocks: 414135
Status: ASSIGNED → NEW
Does this bug report need to stay open? If there are still options to unify frontend code, new bugs should be filed.
Resolving FIXED. If there are still options to unify frontend code, new bugs should be filed.
Assignee: nobody → bugzilla
Status: NEW → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.