Closed Bug 352543 Opened 13 years ago Closed 3 years ago

Package all architectures in a single lightning.xpi [cross-platform]

Categories

(Calendar :: Build Config, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mattwillis, Assigned: Paenglab)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

As discussed in the 2006-09-13 calendar meeting, we're tenatively planning to package Mac (PPC and x86), Linux and Windows architectures in one xpi for ease of installation.

This should block 0.3, but may be cut if we get down to the wire and are without resources or run into trouble.
Assignee: mattwillis → nobody
Flags: blocking0.3+
Status: NEW → ASSIGNED
Some relevant points that we didn't think of during the phone meeting were made in IRC while discussing this:

[10:48] 	shaver	I'm saying that cutting the decision from the blocker list now does buy you something
[10:48] 	shaver	which is time when you most need it

[...]

[10:48] 	shaver	if you do it now you are _guaranteed_ of the cost
[10:48] 	shaver	and you'll have more data after 0.3
[10:49] 	shaver	(how do people feel about the size of the extension?  how common is wrong-OS downloading? etc.)

[...]

[10:50] 	ssitter	packaging in one xpi also needs a lot of additional QA, because this xpi must be tested in thunderbird build for each local x 3 platforms x 2 thunderbird versions (1.5 and 2.0)
[10:51] 	dmose	true, and end-loading that qa sounds painful

I think these are convincing arguments; removing the blocking nomination for now.  If we have extra time in the endgame while waiting for respins, we can consider this again.
Flags: blocking0.3+
Target Milestone: Lightning 0.3 → Lightning 0.5
Not going to make the 0.5 train.  Moving forward.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: Lightning 0.5 → Lightning 0.7
Oops.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Milestone 0.7 Consolidation. Filter on PinkyANDBrain to get rid of bugspam.
Target Milestone: Lightning 0.7 → Sunbird 0.7
Status: REOPENED → NEW
Component: General → Lightning Only
QA Contact: general → lightning
Summary: Package all architectures in lightning xpi → Package all architectures in lightning.xpi
Target Milestone: 0.7 → ---
Version: Trunk → unspecified
please beware we have platform specific preprocessing in the scripts, e.g. <http://mxr.mozilla.org/mozilla1.8/source/calendar/resources/content/printDialog.js#227>
Summary: Package all architectures in lightning.xpi → Package all architectures in a single lightning.xpi [cross-platform]
Duplicate of this bug: 417925
Component: Lightning Only → Build Config
QA Contact: lightning → build
Duplicate of this bug: 599706
I think a lot of this will be solved if bug #677982 lands.
Duplicate of this bug: 1344655
Given the binary component is now shipped with Thunderbird instead of Lightning, we could actually be fairly close to resolving this bug and making Lightning an all-OS xpi file. I haven't had time to look into it though, if someone else does I would appreciate.
Yesterday I tried a Windows Lightning on Linux and it worked. i also tried version 5.6a1 on TB 52 and it worked too.

This where only quick tests, not tested all functions.
Attached patch uniLightning.patch (obsolete) — Splinter Review
This patch removes all preprocessing through using appConstants. For the iconsize="small" I removed preprocessing as all platforms use now small icons.

The systemcolors groupbox in the preferences I hidden for macOS through CSS.

The only file I can't convert is calendar-common-sets.xul. Here the modifiers are defined platform depending. Could this move to calendar-common-sets.js with appConstants? If yes, how?

Not yet tested on Linux and macOS but this should work.
Assignee: nobody → richard.marti
Attachment #8849554 - Flags: feedback?(philipp)
Attached patch uniLightning.patch (obsolete) — Splinter Review
Fixed a too much removed " in lightning-toolbar.xul.
Attachment #8849554 - Attachment is obsolete: true
Attachment #8849554 - Flags: feedback?(philipp)
Attachment #8850722 - Flags: feedback?(philipp)
Comment on attachment 8850722 [details] [diff] [review]
uniLightning.patch

Review of attachment 8850722 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. For the common sets file:


let keys = document.querySelector("#calendar-keys > key");
for (let key of keys) {
  key.setAttribute("modifiers", key.getAttribute("modifiers-" + AppConstants.platform));
}

Probably stick this into calendar-chrome-startup.js or similar. Needs testing if the modifiers can be changed after the fact.
Attachment #8850722 - Flags: feedback?(philipp) → feedback+
Attached patch uniLightning.patch (obsolete) — Splinter Review
(In reply to Philipp Kewisch [:Fallen] from comment #15)
> let keys = document.querySelector("#calendar-keys > key");
> for (let key of keys) {
>   key.setAttribute("modifiers", key.getAttribute("modifiers-" +
> AppConstants.platform));
> }

This didn't worked, also with setting directly the modifier as second attribute. As I understand it it would also set or change modifiers to the other keys in this keyset like openLightningKey, todaypanekey, calendar-new-event-key etc.

I implemented it now with direct ids:

*/**
+ * For linux tab switching reservers alt+number, where on windows that's ctrl.
+ * Use the available modifiers for each platform.
+ * Can't use the OPTION key on OSX, so we will use SHIFT+OPTION on the Mac.
+ */
+function setCalendarModifiers() {
+    let modifier = ""
+    switch (AppConstants.platform) {
+        case "linux":
+            modifier = "accel";
+            break;
+        case "macosx":
+            modifier = "shift alt";
+            break;
+        case "win":
+            modifier = "alt";
+            break;
+    }
+
+    document.getElementById("calendar-day-view-key").setAttribute("modifiers", modifier);
+    document.getElementById("calendar-week-view-key").setAttribute("modifiers", modifier);
+    document.getElementById("calendar-multiweek-view-key").setAttribute("modifiers", modifier);
+    document.getElementById("calendar-month-view-key").setAttribute("modifiers", modifier);
+};

Would this be okay or do you have a more elegant way?

I also removed the platform name from XPI name.
Attachment #8850722 - Attachment is obsolete: true
Attachment #8860470 - Flags: review?(philipp)
Ah ok, I was missing some details.

I guess it should be this:

> let keys = document.querySelector("#calendar-keys > key");
> for (let key of keys) {
>   if (key.hasAttribute("modifiers-" + AppConstants.platform)) {
>     key.setAttribute("modifiers", key.getAttribute("modifiers-" + AppConstants.platform));
>   }
> }

Then in the key do this:

> <key id="calendar-day-view-key" key="1"
>      observes="calendar_day-view_command"
>      modifiers-mac="shift alt"
>      modifiers-linux="accel"
>      modifiers-windows="alt"/>

That way only modifiers that have per-platform values are changed. Make sure the AppContants values are correct, I don't know if it is mac or osx and windows or win32.
Attached patch uniLightning.patch (obsolete) — Splinter Review
This works now with your approach. I needed to add All to querySelector.

I tried the XPI I created on Windows with Linux and it worked.
Attachment #8860470 - Attachment is obsolete: true
Attachment #8860470 - Flags: review?(philipp)
Attachment #8860602 - Flags: review?(philipp)
Comment on attachment 8860602 [details] [diff] [review]
uniLightning.patch

Review of attachment 8860602 [details] [diff] [review]:
-----------------------------------------------------------------

r+wc:

::: calendar/base/content/calendar-chrome-startup.js
@@ +49,5 @@
>      calendarWindowPrefs.init();
>  
> +    // Set up the available modifiers for each platform.
> +    let keys = document.querySelectorAll("#calendar-keys > key");
> +        for (let key of keys) {

Something is wrong with the indent here, can you check?
Also, while you are at it, since AppConstants.platform is used in a loop, maybe you could use a local variable instead, e.g. let platform = AppConstants.platform.
Attachment #8860602 - Flags: review?(philipp) → review+
Fixed indentation and added platform variable.
Attachment #8860602 - Attachment is obsolete: true
Attachment #8862571 - Flags: review+
https://hg.mozilla.org/comm-central/rev/7d9af75e6b67effcbc74c7466242673fa48ae995
Status: NEW → RESOLVED
Closed: 13 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 5.7
You need to log in before you can comment on or make changes to this bug.