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

RESOLVED FIXED in 5.7

Status

Calendar
Build Config
RESOLVED FIXED
12 years ago
9 months ago

People

(Reporter: Matthew (lilmatt) Willis, Assigned: Paenglab)

Tracking

(Depends on: 1 bug)

unspecified

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Updated

12 years ago
Assignee: mattwillis → nobody
Flags: blocking0.3+
(Reporter)

Updated

12 years ago
Status: NEW → ASSIGNED

Comment 1

12 years ago
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+

Updated

12 years ago
Target Milestone: Lightning 0.3 → Lightning 0.5
(Reporter)

Comment 2

11 years ago
Not going to make the 0.5 train.  Moving forward.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: Lightning 0.5 → Lightning 0.7
(Reporter)

Comment 3

11 years ago
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

Comment 6

10 years ago
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

Updated

7 years ago
Duplicate of this bug: 599706
Depends on: 673085

Comment 9

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

Updated

11 months ago
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.
(Assignee)

Comment 12

11 months ago
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.
(Assignee)

Comment 13

10 months ago
Created attachment 8849554 [details] [diff] [review]
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.
Assignee: nobody → richard.marti
Attachment #8849554 - Flags: feedback?(philipp)
(Assignee)

Comment 14

10 months ago
Created attachment 8850722 [details] [diff] [review]
uniLightning.patch

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+
(Assignee)

Comment 16

9 months ago
Created attachment 8860470 [details] [diff] [review]
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.
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.
(Assignee)

Comment 18

9 months ago
Created attachment 8860602 [details] [diff] [review]
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.
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+
(Assignee)

Comment 20

9 months ago
Created attachment 8862571 [details] [diff] [review]
uniLightning.patch

Fixed indentation and added platform variable.
Attachment #8860602 - Attachment is obsolete: true
Attachment #8862571 - Flags: review+
(Assignee)

Comment 21

9 months ago
https://hg.mozilla.org/comm-central/rev/7d9af75e6b67effcbc74c7466242673fa48ae995
Status: NEW → RESOLVED
Last Resolved: 11 years ago9 months ago
Resolution: --- → FIXED
Target Milestone: --- → 5.7
You need to log in before you can comment on or make changes to this bug.