Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for GNOME 45 #362

Closed
mwilck opened this issue Sep 22, 2023 · 71 comments · Fixed by #364
Closed

Support for GNOME 45 #362

mwilck opened this issue Sep 22, 2023 · 71 comments · Fixed by #364
Assignees
Labels
testing needed A fix for this issue is available and needs testing.

Comments

@mwilck
Copy link
Contributor

mwilck commented Sep 22, 2023

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.

@mwilck
Copy link
Contributor Author

mwilck commented Sep 22, 2023

@hedayat for information.

@hedayat
Copy link
Member

hedayat commented Sep 22, 2023

Thanks for starting the work. I didn't know about that, but I'll most likely upgrade to latest Fedora Beta tomorrow, which comes with gnome 45. So, I'll try to help as much as I can (might not be much though, at least for a few days).

@mwilck
Copy link
Contributor Author

mwilck commented Sep 24, 2023

I've just pushed an update.

Good news

The extension is displayed in the panel, and the pop-up shows and works as far as I've tested.
Screenshot from 2023-09-24 23-36-39a

The settings can be opened from the GNOME extensions tool and look ok-ish

image

Not-so-good news

The settings aren't applied correctly yet. The "global hotkey" entry doesn't work at all. (Note that I basically had to re-write the preferences dialog from scratch, copying code from the "window-list" standard extension).

@hedayat
Copy link
Member

hedayat commented Sep 25, 2023

Thanks a lot. Unfortunately, I've not yet started using Gnome 45, and cannot test the code till tomorrow. But I guess it's completely working till then 😅

@mwilck
Copy link
Contributor Author

mwilck commented Sep 25, 2023

The settings dialog works now, too. It's still necessary to logout and relogin to apply the settings, and one day we should fix that, but it's no different from the current behavior until GNOME 44 and earlier.

I've created #363 in preparation of a GNOME 45 PR. Anyway, even if testing by you and hopefully other people is successful, we need to discuss how to proceed further. GNOME 45 support is exclusive and means that no prior GNOME shell version can be supported. Therefore I suppose the develop branch will keep supporting the older versions for some time to come, and will create a gnome-shell-45 branch.

@mwilck
Copy link
Contributor Author

mwilck commented Sep 25, 2023

Re-pushed the branch to 6d31a9b, as I'd made a mistake rebasing it.

@mwilck mwilck added the testing needed A fix for this issue is available and needs testing. label Sep 25, 2023
@hedayat
Copy link
Member

hedayat commented Sep 25, 2023

Thanks a lot! :)

Therefore I suppose the develop branch will keep supporting the older versions for some time to come, and will create a gnome-shell-45 branch.

I suggest the other way around: create a branch for older versions (e.g. gjs-version or pre-gnome-shell-45) and let develop contain latest code.

@hedayat
Copy link
Member

hedayat commented Sep 25, 2023

Well, just so that we have a now almost meaningless master branch too. If we want to keep that around, I think it can contain current develop branch which is compatible with 44 and below, and then develop branch can contain gnome-shell 45 compatible code until it is considered release quality.

@mwilck
Copy link
Contributor Author

mwilck commented Sep 26, 2023

Right. I thought the original idea was to have master in line with e.g.o. But both have bitrotted so much now that it's kind of meaningless.

If we follow your suggestion, I could actually create a PR against develop, which would simplify the review process on GitHub. So yeah, I am in favor of it.

@mwilck
Copy link
Contributor Author

mwilck commented Sep 26, 2023

There are 2 problems: current master is not an ancestor of develop, so we can't fast-forward it (the differences are few and mostly related to commit messages, but they aren't zero). Also, IIRC, @elbenfreund has locked the master branch, so that nobody except himself can do merges or other updates to the branch (I haven't tried in a long time, though) 1.

Notifying all members of @projecthamster/gnome-shell-extension here for follow-up discussion.

Footnotes

  1. I found that our past team discussions are now only available in an awkward json format, which is very hard to read.

@hedayat
Copy link
Member

hedayat commented Sep 26, 2023

Well, maybe its time to create a main branch if @elbenfreund is not available. :D

@matthijskooijman
Copy link
Member

I've tried a few times over the past few years to send @elbenfreund e-mail asking for some changes or transferring some rights, but have not received any reply to those. I wonder if it might be worth considering migrating to a new and separate organization to work around such limitations, but that's probably something to discuss elsewhere (maybe organisation-wide, though organisations do not seem to have (public) discussion options).

@benjaoming
Copy link
Contributor

This is always a huge problem, not to mention that the extension doesn't even have a proper official package on Gnome. I've followed the conversation and all the issues over the years and I could totally get behind a reboot, mainly in terms of governance and distribution. I think renaming the project[1] would help to free up various namespaces in the distribution chain.

[1] for instance to a similar animal that likes running around in wheels, example: "chinchilla"

@mwilck
Copy link
Contributor Author

mwilck commented Sep 26, 2023

I wonder if it might be worth considering migrating to a new and separate organization to work around such limitations

I thought the same.

but that's probably something to discuss elsewhere (maybe organisation-wide, though organisations do not seem to have (public) discussion options).

This project doesn't have GitHub discussions, because @elbenfreund would need to enable it, and we're back at square one.1

I guess we agree that if we move to a different organization, we should move the entire project (at least the currently active repos, i.e. "hamster" and "hamster-shell-extension").

About changing the name: I'm unsure if that's a good idea. People will be looking for "hamster", both on e.g.o and elsewhere. AFAICS, nobody owns the name "hamster", and if anyone did, it would probably be @tstriker. IMO we could simply fork the active repos and keep using the name "hamster", as long as we keep true to the spirit of the project (which I think we have done so far).

Footnotes

  1. We used to have "team discussions" but GitHub has dropped the feature in favor of the new GitHub discussions, that's where the "awkward json" mentioned above comes from.

@tstriker
Copy link
Contributor

the name 'hamster' ('project hamster', specifically) is not copyrighted and nobody owns it so please keep using it if you want - the name was a reference to the hamster wheel and is a tongue-in-cheek reference to 9-to-5 work.

if elbenfreund has gone missing, you might want to reach out to github support - they might allow transferring the project ownership to a new team. until then you can also just abandon 'master' as github deprecated those in favor to 'main' a while ago anyway.

alternatively, of course, you could make a fork org and leave a message somewhere pointing to it but that might be messier.

Note: i unfortunately don't have extra cycles so i won't be monitoring this nor any other conversations relating to project hamster; i've now set up an auto-filter to keep these out of my inbox.

Good luck!

@mwilck
Copy link
Contributor Author

mwilck commented Sep 26, 2023

Thanks a lot, Tom!

Wrt reaching out to github support, I see that as a last resort. Even if @elbenfreund has turned silent, his contributions are massive and still very valuable, and I wouldn't want to be rude to him. I'd strongly appreciate if he voluntarily assigned someone else (one or more of the currently active people) a maintainer role in this repository though. If he can't or doesn't want to do that, I'd rather move to a fork. That's my personal PoV.

@matthijskooijman
Copy link
Member

To prevent further digressing this issue, I've created projecthamster/hamster#730 for further discussion about regaining access to the repositories and organization.

I suggest moving this issue back to supporting gnome 45, and what branches to use (within the current limitations we have wrt repo access).

@mwilck
Copy link
Contributor Author

mwilck commented Sep 27, 2023

Thanks, @matthijskooijman. I suppose the general discussion about the future of the project will take some more time.

Meanwhile, once @hedayat has ultimately approved #363, I will create a PR for merging into master (as we'll certainly won't be able to force-push to master, this will require some manual, local merging on my part). Then we'll see if we can merge into master at all. If not, we can still try to create a "main" branch, but we won't be able to change the fact that "develop" is currently the default branch.

@mwilck
Copy link
Contributor Author

mwilck commented Sep 27, 2023

I pushed another commit to the 362-support-for-gnome-45 branch, fixing an issue that occured when disabling the extension.

@hedayat
Copy link
Member

hedayat commented Oct 1, 2023

Well, I finally got to test this, but I need to see if I can fix hamster python 3.12 compatibility first...

@hedayat
Copy link
Member

hedayat commented Oct 1, 2023

OK, I'm finally able to use the extension under Gnome 45, and so far so good :) At least all functionality I used before are working fine. Thanks!

@hedayat
Copy link
Member

hedayat commented Oct 5, 2023

Well, there is one thing. When I select "Add earlier activity", it opens the dialog with the current time selected as the start time, which is an inconvenience sine you almost always want to change it. It's better to remain like previous version: with no start time so that we can write the desired number right away (for me, usually start with time diff; e.g. -120)

@hedayat
Copy link
Member

hedayat commented Apr 4, 2024

Oops, sorry if I wasn't clear enough. I mean, does the latest code, which works on 46, also work on 45 correctly if 45 is added to metadata.json? The new code might not be compatible with gnome shell 45.

@mwilck
Copy link
Contributor Author

mwilck commented Apr 4, 2024

My bad, I overlooked the fact that you put 46 instead of 45 rather than just adding it.

I'll push an update pretending to support both.
Because I had a similar change in another extension, I'm rather confident it will work with both GNOME 45 and 46. But it would be very nice if someone could test that.

@mwilck
Copy link
Contributor Author

mwilck commented Apr 4, 2024

I just pushed this to the "develop" branch to fix my previous mistake. Please test under GNOME 45. If any issues appear, we'll need to branch once more.

@hedayat
Copy link
Member

hedayat commented Apr 4, 2024

Thanks! Hope it works in 45 too.

@matthijskooijman
Copy link
Member

Please test under GNOME 45

I'm still on 45, so I tested develop (I was previously running the branch from #364 without problems). Seems to mostly work, but the list of todays activities is displayed empty in the popup:

image

In the journal I get these when I open the popup, which I think are related:

apr 04 23:28:33 dottie gnome-shell[3036550]: ../clutter/clutter/clutter-actor.c:8802: Actor '<unnamed>[<StScrollBar>:0x5fa74a918780]' tried to allocate a size of 8,00 x -8,00
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<StBoxLayout>:0x5fa74a91b960] is on because it needs an allocation. 
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<StWidget>:0x5fa74a91c880] is on because it needs an allocation.    
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<StLabel>:0x5fa746695b50] is on because it needs an allocation.     
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<ClutterText>:0x5fa748136950] is on because it needs an allocation.
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<StLabel>:0x5fa74851f9a0] is on because it needs an allocation.    
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<ClutterText>:0x5fa74aa9f590] is on because it needs an allocation.
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<StLabel>:0x5fa74603d990] is on because it needs an allocation.    
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<ClutterText>:0x5fa7485ced50] is on because it needs an allocation.
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<StButton>:0x5fa7471e9ad0] is on because it needs an allocation.   
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<StIcon>:0x5fa749c6f9f0] is on because it needs an allocation.      
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<ClutterActor>:0x5fa745893850] is on because it needs an allocation.
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<StLabel>:0x5fa74ac94300] is on because it needs an allocation.     
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<ClutterText>:0x5fa74baa0b10] is on because it needs an allocation. 
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<StLabel>:0x5fa745760f10] is on because it needs an allocation.     
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<ClutterText>:0x5fa7453d6660] is on because it needs an allocation.
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<StLabel>:0x5fa747a378e0] is on because it needs an allocation.    
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<ClutterText>:0x5fa748070750] is on because it needs an allocation.
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<StButton>:0x5fa74aad03d0] is on because it needs an allocation.   
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<StIcon>:0x5fa746091cd0] is on because it needs an allocation.     
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<ClutterActor>:0x5fa746af3ee0] is on because it needs an allocation.
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<StButton>:0x5fa747e06360] is on because it needs an allocation.    
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<StIcon>:0x5fa74aaa1df0] is on because it needs an allocation.      
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<ClutterActor>:0x5fa74561fb80] is on because it needs an allocation.
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<StLabel>:0x5fa748a880b0] is on because it needs an allocation.     
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<ClutterText>:0x5fa748063400] is on because it needs an allocation. 
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<StLabel>:0x5fa7481489b0] is on because it needs an allocation.    
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<ClutterText>:0x5fa74548c680] is on because it needs an allocation.
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<StLabel>:0x5fa74804eae0] is on because it needs an allocation.    
apr 04 23:28:33 dottie gnome-shell[3036550]: Can't update stage views actor <unnamed>[<ClutterText>:0x5fa74593ace0] is on because it needs an allocation.

I looked around the mutter 45.0 source code a bit and it seems like add and add_child are pretty much equivalent really (add is defined on in clutter-container.c, but pretty much only calls add_child defined in clutter-actor.c), but maybe I'm looking at entirely the wrong code...

I might see if I can pinpoint what line in the 46-fix breaks this, but I probably won't have time now to get to the bottom of this...

@mwilck
Copy link
Contributor Author

mwilck commented Apr 4, 2024

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?

@hedayat
Copy link
Member

hedayat commented Apr 4, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

mwilck commented Apr 5, 2024

@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
Copy link
Member

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/[email protected]/widgets/todaysFactsWidget.js:56:9
                                             TodaysFactsWidget@file:///home/matthijs/.local/share/gnome-shell/extensions/[email protected]/widgets/todaysFactsWidget.js:36:1
                                             _init@file:///home/matthijs/.local/share/gnome-shell/extensions/[email protected]/widgets/factsBox.js:70:34
                                             PopupBaseMenuItem@resource:///org/gnome/shell/ui/popupMenu.js:80:4
                                             FactsBox@file:///home/matthijs/.local/share/gnome-shell/extensions/[email protected]/widgets/factsBox.js:43:1
                                             _init@file:///home/matthijs/.local/share/gnome-shell/extensions/[email protected]/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/[email protected]/widgets/panelWidget.js:56:1
                                             deferred_enable@file:///home/matthijs/.local/share/gnome-shell/extensions/[email protected]/extension.js:140:28
                                             enable/<@file:///home/matthijs/.local/share/gnome-shell/extensions/[email protected]/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
Copy link
Member

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
Copy link
Contributor Author

mwilck commented Apr 5, 2024

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
Copy link
Member

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
Copy link
Contributor Author

mwilck commented Apr 5, 2024

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

@mwilck
Copy link
Contributor Author

mwilck commented Apr 5, 2024

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
Copy link
Member

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
Copy link
Contributor Author

mwilck commented Apr 5, 2024

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 added a commit that referenced this issue Apr 5, 2024
Using St.ScrollView.add_child() instead of St.ScrollView.add_actor()
leads to broken layout in the ScrollView widget (zero vertical extension
of rows). See
#362 (comment)
and follow-up comments.

But unlike other St Widgets, ScrollView still supports the add_actor() method
in GNOME 46. So keep using it.
@mwilck
Copy link
Contributor Author

mwilck commented Apr 5, 2024

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
Copy link
Member

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
Copy link
Contributor Author

mwilck commented Apr 5, 2024

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

@mwilck
Copy link
Contributor Author

mwilck commented Apr 5, 2024

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
Copy link
Contributor Author

mwilck commented Apr 5, 2024

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
Copy link
Contributor Author

mwilck commented Apr 5, 2024

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
Copy link
Member

hedayat commented Apr 5, 2024

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

@hedayat
Copy link
Member

hedayat commented Apr 7, 2024

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: #368 (comment)

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing needed A fix for this issue is available and needs testing.
Projects
None yet