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

Status: RESOLVED FIXED

(Calendar :: Build Config, defect)

Description

[Matthew (lilmatt) Willis]

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.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 2

[Matthew (lilmatt) Willis]

Not going to make the 0.5 train.  Moving forward.

Comment 3

[Matthew (lilmatt) Willis]

Oops.

Comment 4

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

Milestone 0.7 Consolidation. Filter on PinkyANDBrain to get rid of bugspam.

Comment 6

[Daniel Boelzle [:dbo]]

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>

Comment 9

[Felix Möller]

I think a lot of this will be solved if bug #677982 lands.

Comment 11

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

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.

Comment 12

[Richard Marti (:Paenglab)]

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.

Comment 13

[Richard Marti (:Paenglab)]

Attachment: uniLightning.patch

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.

Comment 14

[Richard Marti (:Paenglab)]

Attachment: uniLightning.patch

Fixed a too much removed " in lightning-toolbar.xul.

Comment 15

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

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.

Comment 16

[Richard Marti (:Paenglab)]

Attachment: uniLightning.patch

(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.

Comment 17

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

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.

Comment 18

[Richard Marti (:Paenglab)]

Attachment: uniLightning.patch

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.

Comment 19

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

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.

Comment 20

[Richard Marti (:Paenglab)]

Attachment: uniLightning.patch

Fixed indentation and added platform variable.

Comment 21

[Richard Marti (:Paenglab)]

https://hg.mozilla.org/comm-central/rev/7d9af75e6b67effcbc74c7466242673fa48ae995