Add Gnome Shell 46 support. #64

glerroo · 2024-03-08T00:12:36Z

This close issue #61.

This change is

mgalgs

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @glerroo)

[email protected]/extension.js line 265 at r1 (raw file):

            let mainContentBox = new St.BoxLayout({style_class: 'prompt-dialog-main-layout',
                                                   vertical: false});
            this.contentLayout.add_child(mainContentBox,

I'm still trying to understand this change. The GS 46 extension porting guide mentions Clutter.Container (Clutter.Container.add_actor being of particular interest to us), but I'm not seeing anything about an .add() method going away... And I'm actually not even sure which .add() method is being called here. I can't find an .add() in the St.BoxLayout inheritance chain in the gjs docs but I might just be missing it. 😬

[email protected]/extension.js line 353 at r1 (raw file):

            Clutter.cairo_set_source_color(cr, this.extension._Background);
        else
            cr.setSourceColor(this.extension._Background);

I think this pattern is repeated enough to warrant a helper function. Something like:

// Gnome 45 Clutter.cairo_set_color compatibility
function sm_cairo_set_source_color(cr, bg_color) {
    if (Clutter.cairo_set_source_color)
        Clutter.cairo_set_source_color(cr, bg_color);
    else
       cr.setSourceColor(bc_color);
}

[email protected]/metadata.json line 2 at r1 (raw file):

{
    "shell-version": ["45","46"],

nit: space after comma

mgalgs

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @glerroo)

a discussion (no related file):
Thanks for the patch!!!

Could you please also share which versions of GS you tested this on?

marxin · 2024-03-08T07:50:25Z

Thanks for the pull request. I noticed the following error when running (8832b56) under Gnome 46:

Mar 08 08:48:29 kettlebell gnome-shell[2642]: Extension [email protected]: TypeError: Main.panel._centerBox.remove_actor is not a function
                                              
                                              Stack trace:
                                                enable@file:///home/marxin/.local/share/gnome-shell/extensions/[email protected]/extension.js:2451:39
                                                _callExtensionEnable@resource:///org/gnome/shell/ui/extensionSystem.js:266:38
                                                loadExtension@resource:///org/gnome/shell/ui/extensionSystem.js:478:32
                                                async*_loadExtensions@resource:///org/gnome/shell/ui/extensionSystem.js:786:24
                                                async*_enableAllExtensions@resource:///org/gnome/shell/ui/extensionSystem.js:792:48
                                                _sessionUpdated@resource:///org/gnome/shell/ui/extensionSystem.js:827:20
                                                async*init@resource:///org/gnome/shell/ui/extensionSystem.js:76:14
                                                _initializeUI@resource:///org/gnome/shell/ui/main.js:303:22
                                                start@resource:///org/gnome/shell/ui/main.js:175:11
                                                @resource:///org/gnome/shell/ui/init.js:12:47
                                                @resource:///org/gnome/shell/ui/init.js:21:20

with the commented-out function calls:

diff --git a/[email protected]/extension.js b/[email protected]/extension.js
index 7354ddc..8f5b363 100644
--- a/[email protected]/extension.js
+++ b/[email protected]/extension.js
@@ -2448,9 +2448,6 @@ export default class SystemMonitorExtension extends Extension {
 
             if (this._Schema.get_boolean('move-clock')) {
                 let dateMenu = Main.panel.statusArea.dateMenu;
-                Main.panel._centerBox.remove_actor(dateMenu.container);
-                Main.panel._addToPanelBox('dateMenu', dateMenu, -1, Main.panel._rightBox);
-                tray.clockMoved = true;
             }
 
             this._Schema.connect('changed::background', (schema, key) => {
@@ -2548,8 +2545,6 @@ export default class SystemMonitorExtension extends Extension {
         // restore clock
         if (this.__sm.tray.clockMoved) {
             let dateMenu = Main.panel.statusArea.dateMenu;
-            Main.panel._rightBox.remove_actor(dateMenu.container);
-            Main.panel._addToPanelBox('dateMenu', dateMenu, Main.sessionMode.panel.center.indexOf('dateMenu'), Main.panel._centerBox);
         }
         // restore system power icon if necessary
         // workaround bug introduced by multiple cpus init :

I've got a working extension, yay!

vibragiel · 2024-03-08T08:55:12Z

Hi. I tried the changes in this PR in Gnome-Shell 46 and it's working nicely (I didn't have to apply @marxin's changes).

marxin · 2024-03-08T09:05:57Z

You're likely going to hit the issue once you enable "Move the Clonk" in Extension settings.

NVieville · 2024-03-08T09:34:51Z

Hello,

According to:

https://gjs.guide/extensions/upgrading/gnome-shell-46.html#clutter-container

The function remove_actor seems to have been replaced by remove_child.
Maybe this is the missing last thing to modify and to get a functional extension for GS 46.

Hope this will help.

Cordially,

--
NVieville

marxin · 2024-03-08T09:57:00Z

Yep, thanks, the following patch works for me:

diff --git a/[email protected]/extension.js b/[email protected]/extension.js
index 7354ddc..058f334 100644
--- a/[email protected]/extension.js
+++ b/[email protected]/extension.js
@@ -2448,7 +2448,7 @@ export default class SystemMonitorExtension extends Extension {
 
             if (this._Schema.get_boolean('move-clock')) {
                 let dateMenu = Main.panel.statusArea.dateMenu;
-                Main.panel._centerBox.remove_actor(dateMenu.container);
+                Main.panel._centerBox.remove_child(dateMenu.container);
                 Main.panel._addToPanelBox('dateMenu', dateMenu, -1, Main.panel._rightBox);
                 tray.clockMoved = true;
             }
@@ -2548,7 +2548,7 @@ export default class SystemMonitorExtension extends Extension {
         // restore clock
         if (this.__sm.tray.clockMoved) {
             let dateMenu = Main.panel.statusArea.dateMenu;
-            Main.panel._rightBox.remove_actor(dateMenu.container);
+            Main.panel._rightBox.remove_child(dateMenu.container);
             Main.panel._addToPanelBox('dateMenu', dateMenu, Main.sessionMode.panel.center.indexOf('dateMenu'), Main.panel._centerBox);
         }
         // restore system power icon if necessary

and the Clock is properly moved:

glerroo · 2024-03-08T15:41:36Z

@marxin, @NVieville Bug resolved, can confirm that is ok.
@mgalgs i think i the solution of .add() is in this commit that remove a monkey patch from St.Layout environment: Stop patching StBoxLayout.

I tested this pr in Fedora 40.

mgalgs · 2024-03-08T16:51:14Z

@mgalgs i think i the solution of .add() is in this commit that remove a monkey patch from St.Layout environment: Stop patching StBoxLayout.

Got it, thanks.

I think this is mergeable once we get test results on GS 45, verifying no regressions.

mgalgs

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @glerroo)

[email protected]/extension.js line 265 at r1 (raw file):

Previously, mgalgs (Mitchel Humpherys) wrote…

I'm still trying to understand this change. The GS 46 extension porting guide mentions Clutter.Container (Clutter.Container.add_actor being of particular interest to us), but I'm not seeing anything about an .add() method going away... And I'm actually not even sure which .add() method is being called here. I can't find an .add() in the St.BoxLayout inheritance chain in the gjs docs but I might just be missing it. 😬

Hold on, what about the second argument? Looks like the old monkey-patched .add() method took a second argument of props to copy over, which clutter.Actor.add_child() doesn't seem to do.

glerroo · 2024-03-08T18:48:05Z

I think the second argument is optional, if present assign properties to actor.

// "monkey patch" in some varargs ClutterContainer methods; we need
// to do this per-container class since there is no representation
// of interfaces in Javascript
function _patchContainerClass(containerClass) {
    // This one is a straightforward mapping of the C method
    containerClass.prototype.child_set = function (actor, props) {
        let meta = this.get_child_meta(actor);
        for (let prop in props)
            meta[prop] = props[prop];
    };

    // clutter_container_add() actually is a an add-many-actors
    // method. We conveniently, but somewhat dubiously, take the
    // this opportunity to make it do something more useful.
    containerClass.prototype.add = function (actor, props) {
        this.add_actor(actor);
        
        if (props)
            this.child_set(actor, props);
    };
}

if props is present call this.child_set(actor, props);

mgalgs · 2024-03-08T20:06:08Z

Right, it's optional, but we are using it. So with your patch it seems like we're missing that part (copying the props in the second argument to the child).

glerroo · 2024-03-08T21:54:22Z

In this part there is some problem:

I think the smDialog is never opened because smDepsGtop and smDepsNM are always true.
I set smDepsGtop to false with the current GS45 version and

Mar 08 22:33:30 fedora gnome-shell[14007]: [system-monitor-next] applet enable from /home/fedora39/.local/share/gnome-shell/extensions/[email protected]
Mar 08 22:33:30 fedora gnome-shell[14007]: Extension [email protected]: TypeError: meta is null
                                           
                                           Stack trace:
                                             _patchContainerClass/containerClass.prototype.child_set@resource:///org/gnome/shell/ui/environment.js:45:13
                                             _patchContainerClass/containerClass.prototype.add@resource:///org/gnome/shell/ui/environment.js:54:18
                                             SystemMonitor_smDialog@file:///home/fedora39/.local/share/gnome-shell/extensions/[email protected]/extension.js:266:32
                                             enable@file:///home/fedora39/.local/share/gnome-shell/extensions/[email protected]/extension.js:2386:27
                                             _callExtensionEnable@resource:///org/gnome/shell/ui/extensionSystem.js:253:38
                                             loadExtension@resource:///org/gnome/shell/ui/extensionSystem.js:461:32
                                             async*_loadExtensions@resource:///org/gnome/shell/ui/extensionSystem.js:759:24
                                             async*_enableAllExtensions@resource:///org/gnome/shell/ui/extensionSystem.js:765:48
                                             _sessionUpdated@resource:///org/gnome/shell/ui/extensionSystem.js:800:20
                                             async*init@resource:///org/gnome/shell/ui/extensionSystem.js:72:14
                                             _initializeUI@resource:///org/gnome/shell/ui/main.js:303:22
                                             start@resource:///org/gnome/shell/ui/main.js:176:11
                                             @resource:///org/gnome/shell/ui/init.js:12:47
                                             @resource:///org/gnome/shell/ui/init.js:21:20

can you verify and make some test?

jonnedeprez · 2024-03-25T12:16:47Z

any progress on this last remaining issue?

mgalgs

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @glerroo)

[email protected]/extension.js line 265 at r1 (raw file):

Previously, mgalgs (Mitchel Humpherys) wrote…

Hold on, what about the second argument? Looks like the old monkey-patched .add() method took a second argument of props to copy over, which clutter.Actor.add_child() doesn't seem to do.

@glerroo just to re-iterate, it looks like by moving from .add() to add_child() we're now losing the second argument (the props to copy to the child). Maybe we need to make a separate call to copy the props (or add an internal helper to do so)? I haven't hunted everything down yet but clutter.Container.child_set_property looks promising?

glerroo · 2024-03-25T20:21:45Z

@glerroo just to re-iterate, it looks like by moving from .add() to add_child() we're now losing the second argument (the props to copy to the child). Maybe we need to make a separate call to copy the props (or add an internal helper to do so)? I haven't hunted everything down yet but clutter.Container.child_set_property looks promising?

@mgalgs Ok I make some test, but smDialog class is never used becase smDepsGtop and smDepsNM are always true. I think we can remove that class.

lukedupin · 2024-03-25T20:52:29Z

I just pulled this PR and everything worked great. I checked the logs for errors, saw none. I clicked through all the menus, flipping things on and off. No issues found.

glerroo · 2024-03-25T21:06:39Z

@mgalgs Clutter.Container was removed from GS46.
https://gjs.guide/extensions/upgrading/gnome-shell-46.html#clutter-container

The interface will be gone in GNOME 46. Prepare for that by switching from `add()` to Clutter.Actor's `add_child()`. The replacement has been around forever, so this does not affect compatibility with the current stable version.

If you are using Clutter.cairo_set_source_color() in St.DrawingArea, cairo.Context can use setSourceColor() instead (cr.setSourceColor()).

Use Clutter.Actor.remove_child() instead of Clutter.Container.remove_action().

…r().

Our system dependency pop-up dialog (smDialog) is currently using some private APIs which are being removed in Gnome Shell 46 [1]. Rather than fixing this let's just tie it up and throw it overboard. This is a reasonable course of action because: (1) It's not clear which replacement APIs we should be moving to. We could figure this out with a bit of research and testing, but it may not be worth it because: (2) The smDialog dependency checks have actually been hard-coded out since the port to Gnome Shell 45 [2] (since the old dependency check method relied on a try/catch of the library imports, which isn't supported in es6). So even if we figure out a new API for copying props to the child, we'd still never be using any of that new code anyway since we're hard-coding an assumption of the dependencies being present. Ultimately we *should* fix this for the best possible user experience, but in the interest of keeping things moving with Gnome Shell updates we'll defer that work to the back burner (#69). [1] #64 (comment) [2] (f838b97: "extension: Update for Gnome Shell 45")

mgalgs · 2024-03-25T21:44:08Z

Ok I make some test, but smDialog class is never used becase smDepsGtop and smDepsNM are always true

@glerroo aaah now I see what you're saying. You're 100% right. I tossed it out and pushed the result. Working fine on my Gnome Shell 46 box, I'll test on 45 a bit later but if anyone else could please test the latest from this PR that would be fantastic!

I don't think there's anything else blocking this other than test results, but please chime in if I'm missing anything.

mgalgs · 2024-03-25T22:04:09Z

Hmm my changes don't seem to be showing up... Currently mobile but I'll check my refs in an hour or so... Stand by...

Edit: ok, branch looks good now.

jonnedeprez · 2024-03-25T22:24:52Z

The PR from @glerroo works fine on my Arch Linux with Gnome 46

mgalgs

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @glerroo)

mgalgs · 2024-03-25T22:45:55Z

Thanks, @jonnedeprez. Unfortunately, as of the time of your comment the PR branch didn't contain all of the changes 😭 (that was my bad). Can you please re-fetch and try again?

Anyone who wants to test this PR can quickly check it out with:

git fetch https://github.com/mgalgs/gnome-shell-system-monitor-applet.git +refs/pull/64/head && git checkout FETCH_HEAD

mgalgs

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @glerroo)

glerroo · 2024-03-25T23:02:48Z

@mgalgs I'm sorry, but at this point I don't understand what I should do.

mgalgs · 2024-03-25T23:20:40Z

@glerroo I just need GS 46 and 45 testing results. I know people have been testing throughout the discussion but I want to make sure the latest changes get tested.

mgalgs · 2024-03-25T23:28:29Z

All right, I've tested on 45 and 46. If someone else could please confirm these test results on the latest version of this PR then we can ship this. 🚢

git fetch https://github.com/mgalgs/gnome-shell-system-monitor-applet.git +refs/pull/64/head && git checkout FETCH_HEAD

glerroo · 2024-03-25T23:44:15Z

Ok, i have also tested on Fedora 39 - GS45 and Fedora 40 - GS46 with no error.
I think we can remove also line 34 import * as ModalDialog from "resource:///org/gnome/shell/ui/modalDialog.js";

Our system dependency pop-up dialog (smDialog) is currently using some private APIs which are being removed in Gnome Shell 46 [1]. Rather than fixing this let's just tie it up and throw it overboard. This is a reasonable course of action because: (1) It's not clear which replacement APIs we should be moving to. We could figure this out with a bit of research and testing, but it may not be worth it because: (2) The smDialog dependency checks have actually been hard-coded out since the port to Gnome Shell 45 [2] (since the old dependency check method relied on a try/catch of the library imports, which isn't supported in es6). So even if we figure out a new API for copying props to the child, we'd still never be using any of that new code anyway since we're hard-coding an assumption of the dependencies being present. Ultimately we *should* fix this for the best possible user experience, but in the interest of keeping things moving with Gnome Shell updates we'll defer that work to the back burner (mgalgs#69). [1] mgalgs#64 (comment) [2] (f838b97: "extension: Update for Gnome Shell 45")

mgalgs

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @glerroo)

mgalgs · 2024-03-25T23:55:41Z

Thanks a bunch, @glerroo, and everyone else who provided feedback and test results! I'll get this tagged and shipped to extensions.gnome.org right now, they typically approve within a couple of hours.

glerroo mentioned this pull request Mar 8, 2024

Gnome 46 support #61

Closed

mgalgs requested changes Mar 8, 2024

View reviewed changes

mgalgs requested changes Mar 25, 2024

View reviewed changes

AlexWayfer mentioned this pull request Mar 25, 2024

Changes according to Gnome 46 #68

Closed

2 tasks

glerroo added 6 commits March 25, 2024 14:10

Stop using Clutter.Container methods.

75082a1

The interface will be gone in GNOME 46. Prepare for that by switching from `add()` to Clutter.Actor's `add_child()`. The replacement has been around forever, so this does not affect compatibility with the current stable version.

GNOME Shell 46 dropped Clutter.cairo helpers.

d0608f7

If you are using Clutter.cairo_set_source_color() in St.DrawingArea, cairo.Context can use setSourceColor() instead (cr.setSourceColor()).

Add Gnome Shell 46 support.

674eb71

More Clutter.Container deprecation.

a350945

Use Clutter.Actor.remove_child() instead of Clutter.Container.remove_action().

Add a space to separate GS version.

f2e6992

Add a compatibility helper function for Clutter.cairo_set_source_colo…

694f159

…r().

mgalgs mentioned this pull request Mar 25, 2024

System dependency checking needs a rework #69

Open

mgalgs force-pushed the gs46-dev branch from b7ba8a6 to b3630b1 Compare March 25, 2024 22:42

mgalgs approved these changes Mar 25, 2024

View reviewed changes

mgalgs force-pushed the gs46-dev branch from b3630b1 to 461475b Compare March 25, 2024 23:51

mgalgs approved these changes Mar 25, 2024

View reviewed changes

mgalgs merged commit 002f432 into mgalgs:master Mar 25, 2024
1 check was pending

mgalgs mentioned this pull request Mar 26, 2024

System-monitor-next applet no longer works #60

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Gnome Shell 46 support. #64

Add Gnome Shell 46 support. #64

glerroo commented Mar 8, 2024 •

edited by mgalgs

Loading

mgalgs left a comment

mgalgs left a comment

marxin commented Mar 8, 2024 •

edited

Loading

vibragiel commented Mar 8, 2024

marxin commented Mar 8, 2024

NVieville commented Mar 8, 2024

marxin commented Mar 8, 2024

glerroo commented Mar 8, 2024 •

edited

Loading

mgalgs commented Mar 8, 2024

mgalgs left a comment

glerroo commented Mar 8, 2024

mgalgs commented Mar 8, 2024

glerroo commented Mar 8, 2024

jonnedeprez commented Mar 25, 2024

mgalgs left a comment

glerroo commented Mar 25, 2024

lukedupin commented Mar 25, 2024

glerroo commented Mar 25, 2024

mgalgs commented Mar 25, 2024

mgalgs commented Mar 25, 2024 •

edited

Loading

jonnedeprez commented Mar 25, 2024

mgalgs left a comment

mgalgs commented Mar 25, 2024

mgalgs left a comment

glerroo commented Mar 25, 2024

mgalgs commented Mar 25, 2024

mgalgs commented Mar 25, 2024

glerroo commented Mar 25, 2024

mgalgs left a comment

mgalgs commented Mar 25, 2024

Add Gnome Shell 46 support. #64

Add Gnome Shell 46 support. #64

Conversation

glerroo commented Mar 8, 2024 • edited by mgalgs Loading

mgalgs left a comment

Choose a reason for hiding this comment

mgalgs left a comment

Choose a reason for hiding this comment

marxin commented Mar 8, 2024 • edited Loading

vibragiel commented Mar 8, 2024

marxin commented Mar 8, 2024

NVieville commented Mar 8, 2024

marxin commented Mar 8, 2024

glerroo commented Mar 8, 2024 • edited Loading

mgalgs commented Mar 8, 2024

mgalgs left a comment

Choose a reason for hiding this comment

glerroo commented Mar 8, 2024

mgalgs commented Mar 8, 2024

glerroo commented Mar 8, 2024

jonnedeprez commented Mar 25, 2024

mgalgs left a comment

Choose a reason for hiding this comment

glerroo commented Mar 25, 2024

lukedupin commented Mar 25, 2024

glerroo commented Mar 25, 2024

mgalgs commented Mar 25, 2024

mgalgs commented Mar 25, 2024 • edited Loading

jonnedeprez commented Mar 25, 2024

mgalgs left a comment

Choose a reason for hiding this comment

mgalgs commented Mar 25, 2024

mgalgs left a comment

Choose a reason for hiding this comment

glerroo commented Mar 25, 2024

mgalgs commented Mar 25, 2024

mgalgs commented Mar 25, 2024

glerroo commented Mar 25, 2024

mgalgs left a comment

Choose a reason for hiding this comment

mgalgs commented Mar 25, 2024

glerroo commented Mar 8, 2024 •

edited by mgalgs

Loading

marxin commented Mar 8, 2024 •

edited

Loading

glerroo commented Mar 8, 2024 •

edited

Loading

mgalgs commented Mar 25, 2024 •

edited

Loading