Support for GNOME 45

[mwilck]

We need to add support for GNOME 45.

I've pushed pushing my code with GNOME 45 support to the 362-support-for-gnome-45 branch branch in this repository.

The branch seems to work. I'm using it right now. Please test and please post any issues here.

[hedayat]

Too bad...

@hedayat, to my understanding the replacement of .add_actor() by .add_child() should be well backward compatible. But you also replaced some .add() instances by .add_child(). Did you find this necessary? @matthijskooijman, can you try reverting just the .add() hunks and see if that fixes the issue?

Yes, I actually tried to run the extension, received errors and tried to fix errors. I don't think any of the changes can be reverted. But, replacing add() with add_child() was just a guess, so there might be another replacement which is compatible with both 45 & 46. But, IIRC, add() certainly doesn't work under 46.

[matthijskooijman]

But, replacing add() with add_child() was just a guess

It seems to be correct to me, assuming that I've found the right C code here: https://gitlab.gnome.org/GNOME/mutter/-/blob/45.5/clutter/clutter/clutter-container.c?ref_type=tags#L301

But, IIRC, add() certainly doesn't work under 46. That entire file was removed in gnome 46.0, so that would make sense.

If I apply this patch, it works again on gnome 45:

--- a/extension/widgets/todaysFactsWidget.js
+++ b/extension/widgets/todaysFactsWidget.js
@@ -48,7 +48,7 @@ class TodaysFactsWidget extends St.ScrollView {
             reactive: true
         });
         this.factsBox.add_child(this.facts_widget);
-        this.add_child(this.factsBox);
+        this.add_actor(this.factsBox);

     }

Look at the code for gnome 45.2 (which I am running), it seems that add_actor just calls add_child here: https://gitlab.gnome.org/GNOME/mutter/-/blob/45.2/clutter/clutter/clutter-container.c?ref_type=tags#L220 through an indirect add call but that should always be pointing container_real_add which calls clutter_actor_add_child here: https://gitlab.gnome.org/GNOME/mutter/-/blob/45.2/clutter/clutter/clutter-container.c?ref_type=tags#L106

One thing that is different, is that add_actor also calls clutter_container_create_child_meta beforehand: https://gitlab.gnome.org/GNOME/mutter/-/blob/45.2/clutter/clutter/clutter-container.c?ref_type=tags#L205, but clutter_actor_add_child essentially does the same (it calls the internal version here: https://gitlab.gnome.org/GNOME/mutter/-/blob/45.2/clutter/clutter/clutter-actor.c#L11221 with flags that include ADD_CHILD_CREATE_META which causes clutter_container_create_child_meta to be called here as well: https://gitlab.gnome.org/GNOME/mutter/-/blob/45.2/clutter/clutter/clutter-actor.c#L11099 The only difference I can see is that g_object_freeze_notify is called before that, maybe that messes things up by delaying allocations or something?

[matthijskooijman]

I tried simplifying things (e.g. removing all facts and just putting a label there) and making the BoxLayout more similar to the one in factsBox.js, but even then I could not make it work with add_child. It seems a bit like using a ScrollView like we do is not compatible with add_child somehow? Here's the diff I ended up with, still renders incorrectly in the same way:

--- a/extension/widgets/todaysFactsWidget.js
+++ b/extension/widgets/todaysFactsWidget.js
@@ -40,16 +40,12 @@ class TodaysFactsWidget extends St.ScrollView {
         this._panelWidget = panelWidget;

-        this.factsBox = new St.BoxLayout({});
+        this.factsBox = new St.BoxLayout({style_class: 'hamster_box'});
         this.factsBox.set_vertical(true);
-        this.facts_widget = new St.Widget({
-            style_class: 'hamster-activities',
-            layout_manager: new Clutter.GridLayout(),
-            reactive: true
-        });
-        this.factsBox.add_child(this.facts_widget);
-        this.add_child(this.factsBox);
-
+        let fact_list_label = new St.Label({style_class: 'hamster-box-label'});
+        fact_list_label.set_text(_("XXX"));
+        this.factsBox.add_child(fact_list_label);
+        this.actor.add_child(this.factsBox);
     }

     /**
@@ -187,8 +183,8 @@ class TodaysFactsWidget extends St.ScrollView {
      * Clear the widget and populate it anew.
      */
     refresh(facts, ongoingFact) {
-        this.facts_widget.remove_all_children();
-        this.populateFactsWidget(facts, ongoingFact);
+        //this.facts_widget.remove_all_children();
+        //this.populateFactsWidget(facts, ongoingFact);
     }
 });

[matthijskooijman]

Hm, my last diff above was not so useful: If I try that with add_actor instead of add_child, it still does not work, so I guess putting labels directly in a ScrollView (I also tried that after the above diff) or in a BoxLayout is not the way to go. I'm just guessing here, though...

[mwilck]

@matthijskooijman , can you try this?

diff --git a/extension/widgets/todaysFactsWidget.js b/extension/widgets/todaysFactsWidget.js
index 1734cde..85f1653 100644
--- a/extension/widgets/todaysFactsWidget.js
+++ b/extension/widgets/todaysFactsWidget.js
@@ -47,7 +47,7 @@ class TodaysFactsWidget extends St.ScrollView {
             layout_manager: new Clutter.GridLayout(),
             reactive: true
         });
-        this.factsBox.add_child(this.facts_widget);
+        this.factsBox.actor.add_child(this.facts_widget);
         this.add_child(this.factsBox);

     }

[matthijskooijman]

I can try, but I'm not very hopeful. I tried this.actor.add_child() yesterday, which got me:

apr 05 01:07:03 dottie gnome-shell[3453979]: Usage of object.actor is deprecated for TodaysFactsWidget
                                             get@resource:///org/gnome/shell/ui/environment.js:320:23
                                             _init@file:///home/matthijs/.local/share/gnome-shell/extensions/contact@projecthamster.org/widgets/todaysFactsWidget.js:56:9
                                             TodaysFactsWidget@file:///home/matthijs/.local/share/gnome-shell/extensions/contact@projecthamster.org/widgets/todaysFactsWidget.js:36:1
                                             _init@file:///home/matthijs/.local/share/gnome-shell/extensions/contact@projecthamster.org/widgets/factsBox.js:70:34
                                             PopupBaseMenuItem@resource:///org/gnome/shell/ui/popupMenu.js:80:4
                                             FactsBox@file:///home/matthijs/.local/share/gnome-shell/extensions/contact@projecthamster.org/widgets/factsBox.js:43:1
                                             _init@file:///home/matthijs/.local/share/gnome-shell/extensions/contact@projecthamster.org/widgets/panelWidget.js:99:25
                                             ButtonBox@resource:///org/gnome/shell/ui/panelMenu.js:13:1
                                             PanelMenuButton@resource:///org/gnome/shell/ui/panelMenu.js:99:4
                                             PanelWidget@file:///home/matthijs/.local/share/gnome-shell/extensions/contact@projecthamster.org/widgets/panelWidget.js:56:1
                                             deferred_enable@file:///home/matthijs/.local/share/gnome-shell/extensions/contact@projecthamster.org/extension.js:140:28
                                             enable/<@file:///home/matthijs/.local/share/gnome-shell/extensions/contact@projecthamster.org/extension.js:129:14
                                             wrapper/<@resource:///org/gnome/gjs/modules/core/overrides/Gio.js:251:36
                                             @resource:///org/gnome/shell/ui/init.js:21:20

So I suspect that using .actor in general is no longer needed, but maybe that deprecation does not apply to the factsbox.

[matthijskooijman]

Tried it, did not help unfortunately (ongoing facts box still empty).

And also a deprecation working:

apr 05 13:29:10 dottie gnome-shell[732106]: Usage of object.actor is deprecated for St_BoxLayout

[mwilck]

It works under GNOME 45 with this admittedly clumsy workaround:

diff --git a/extension/widgets/todaysFactsWidget.js b/extension/widgets/todaysFactsWidget.js
index 1734cde..29b9bbd 100644
--- a/extension/widgets/todaysFactsWidget.js
+++ b/extension/widgets/todaysFactsWidget.js
@@ -28,6 +28,7 @@ import GObject from 'gi://GObject';

 import { gettext as _ } from 'resource:///org/gnome/shell/extensions/extension.js';
 import * as Stuff from '../stuff.js';
+import * as Config from 'resource:///org/gnome/shell/misc/config.js';

 /**
  * A widget that lists all facts for *today*.
@@ -48,7 +49,10 @@ class TodaysFactsWidget extends St.ScrollView {
             reactive: true
         });
         this.factsBox.add_child(this.facts_widget);
-        this.add_child(this.factsBox);
+        if (Config.PACKAGE_VERSION.substring(0, 2) == "45")
+            this.add_actor(this.factsBox);
+        else
+            this.add_child(this.factsBox);

     }

I'll test this under 46 too, and push this until we find a more elegant solution.

[matthijskooijman]

It works under GNOME 45 with this admittedly clumsy workaround:

I do not think this warrants investing more time, clumsy workaround seems fine to me (maybe with add a link to this comment thread). I've applied your patch, seems to work as expected now.

[mwilck]

It might actually be a bug in GNOME shell. I think I'll open an issue there.

[mwilck]

OTOH, they probably won't care. If they did, they wouldn't be doing this kind of thing to extension maintainers all the time.

[matthijskooijman]

Yeah, I would not bother, especially if the issue is not present in 46, then it might be a bug that they already fixed.

[mwilck]

There is a simpler solution. We can just continue using add_actor() for St.ScrollView in GNOME 46. I expect this to break in GNOME 47, but for now, it works.

[mwilck]

I've pushed a trivial one-line revert to develop now that uses ScrollView.add_actor(). @matthijskooijman please verify it under GNOME 45 if you have time (I need to do another btrfs snapshot rollback to test it, which is very time consuming).

[matthijskooijman]

I've pushed a trivial one-line revert to develop now that uses ScrollView.add_actor(). @matthijskooijman please verify it under GNOME 45 if you have time (I need to do another btrfs snapshot rollback to test it, which is very time consuming).

Heh, and I thought having to log out and in was a lot of work to test this every time ;-p (btw, the nested gnome-shell trick documented in the README for quick testing without relogging seems to have broken at some point - gnome-shell starts up but cannot find hamster, which I guess makes sense since the nested instance has its own dbus bus...).

Anyway, I'm now running develop, seems to work as expected.

[mwilck]

I've created https://gitlab.gnome.org/GNOME/gnome-shell-extensions/-/issues/502 to make the GNOME devs aware of this.

[mwilck]

Heh, and I thought having to log out and in was a lot of work to test this every time

snapshot rollback is neccessary to go back from GNOME 46 to 45. Otherwise I just type Alt-F2 and "r" (yes, still on X11).

[mwilck]

Lesson learned: I waited 1/2y before pushing the GNOME 45 update (which was large) to develop, but for the apparently "simple" GNOME 46 update, I decided to move forward quickly, which played out badly for GNOME 45. Next time I'll be more careful.

[mwilck]

I've been told I should have opened the GNOME issue against gjs-guide. Here it is: https://gitlab.gnome.org/ewlsh/gjs-guide/-/issues/91

[hedayat]

the nested gnome-shell trick documented in the README for quick testing without relogging seems to have broken at some point -

Interesting... it still works for me. :thinking:

[hedayat]

I've pushed a trivial one-line revert to develop now that uses ScrollView.add_actor(). @matthijskooijman please verify it under GNOME 45 if you have time (I need to do another btrfs snapshot rollback to test it, which is very time consuming).

Well, Gnome 46 compatibility is broken now: https://github.com/projecthamster/hamster-shell-extension/pull/368#issuecomment-2041428358

I think let's just accept to have a separate branch for 45! Well, its certainly a possibility, but since this is just one line, we might do something to have a shared branch also. :P Anyway, whatever works is good!