JasonLG1979 / gnome-shell-extension-mpris-indicator-button

A full featured MPRIS indicator button extension for GNOME Shell 3.38+
https://extensions.gnome.org/extension/1379/mpris-indicator-button/
GNU General Public License v3.0
159 stars 21 forks source link

Add support for GNOME 43 #69

Open Moon-0xff opened 1 year ago

Moon-0xff commented 1 year ago

It simply adds the removed code for AggregateLayout to widgets.js

This is a quick and simple fix for my own testing purposes, so don't expect much of it :P

JasonLG1979 commented 1 year ago

Sorry I missed this. Must have got lost in the noise. I will test this out after work today and if all goes well merge it. Thanks.

ChrisLauinger77 commented 1 year ago

Bildschirmfoto vom 2022-12-06 18-05-17 thats how it looks like for me the complete command to pack the extension was "gnome-extensions pack --podir=../po --extra-source=dbus.js --extra-source=indicatorToolTip.js --extra-source=translations.js --extra-source=widgets.js --force"

JasonLG1979 commented 1 year ago

That's what I was afraid of. We may be missing a style class or something? The goal ofc is to match the width of the menu on the far right (whatever they're calling it now?) as it did in previous releases.

ChrisLauinger77 commented 1 year ago

its called quicksettings https://gjs.guide/extensions/upgrading/gnome-shell-43.html#quick-settings see also https://gjs.guide/extensions/topics/quick-settings.html#example-usage

ChrisLauinger77 commented 1 year ago

I just found they have QuickSettingsLayout as replacement. https://gitlab.gnome.org/GNOME/gnome-shell/-/blob/main/js/ui/quickSettings.js I think I can make another proposal how to solve this tomrrow

Moon-0xff commented 1 year ago

Using QuickSettingsLayout it looks like this: Screenshot from 2022-12-06 17-04-08

I just copied the entire thing to widgets.js, from line 381 to 579, added GLib, and replaced AggregateLayout to QuickSettingsLayout

This isn't a clean way of doing this but probably maintains compatibility with older versions of GNOME.

JasonLG1979 commented 1 year ago

That doesn't look right either though.

ChrisLauinger77 commented 1 year ago

https://gitlab.gnome.org/GNOME/gnome-shell/-/blob/gnome-42/js/ui/panel.js how about using the old AggregateLayout from panel.js inside the extension ?

Moon-0xff commented 1 year ago

That's the commit of this PR

JasonLG1979 commented 1 year ago

I'm pretty sure it depends on a style class to set the width.

Moon-0xff commented 1 year ago

AggregateLayout gets the width from vfunc_get_preferred_width. You can set the height creating a function named vfunc_get_preferred_height too.

Moon-0xff commented 1 year ago

Both functions are present in QuickSettings, however they are a lot more complicated.

JasonLG1979 commented 1 year ago

The layout doesn't set the width it just makes sure everything stays the proper size. The Aggregate menu style class sets the width. I'm looking for a replacement for that if it exists.

JasonLG1979 commented 1 year ago

If a suitable replacement does not exist we can create our own. I'd rather not do that though. I'd rather it be a built-in style class so the extension matched whatever theme more.

giinuu commented 1 year ago

I might be totally missing something here, messing around locally I noticed that commenting out line #1427 fixes the width issue.

This part could be totally essential and I'm missing something, tried Rhythmbox, YT Music and Youtube - all seem to work well.

doronbehar commented 1 year ago

I tested this patch along with commenting out line 1427 as @giinuu suggested and it's working great for me. Thanks for everyone who tested and contributing :) Would be great for others if this got a bit of attention.

petar-v commented 1 year ago

Hi! Thanks for that extension, it's great! I have missed having it! :)

I just tested it on my end as well. Looks alright IMHO, and it does the job. image

Moreover, this is tested on Gnome v44. It works perfectly as long as the metadata.json includes that version. :)

Moon-0xff commented 1 year ago

Commenting 1427 negates this PR, indeed is the only change the extension needs to run on 43/44

Mershl commented 1 year ago

Commenting 1427 negates this PR, indeed is the only change the extension needs to run on 43/44

Is it still working for you on 44 @Moon-0xff ? Fedora 38 throws an JS error for me:

JS ERROR: TypeError: Gtk.IconTheme.get_default is not a function

Location: https://github.com/JasonLG1979/gnome-shell-extension-mpris-indicator-button/blob/master/mprisindicatorbutton%40JasonLG1979.github.io/dbus.js#L1915

~Someone know an alternative to Gtk.IconTheme.get_default() or is it just gone with Gtk4?~ -> https://gjs.guide/extensions/upgrading/gnome-shell-44.html#gtk-icontheme

Moon-0xff commented 1 year ago

@Mershl I can't test right now but probably you are commenting the wrong line. If you are starting from JasonLG1979:master the line number is 1428. The line to comment is this one: this.menu.box.set_layout_manager(new AggregateLayout());

Mershl commented 1 year ago

@Mershl I can't test right now but probably you are commenting the wrong line. If you are starting from JasonLG1979:master the line number is 1428. The line to comment is this one: this.menu.box.set_layout_manager(new AggregateLayout());

I'm seeing a seperate issue on 44 regarding the default IconTheme, that is also mentioned in the porting guide of 44 (linked in my previous post). I was just curious why it was working for you on 44, but maybe this interface change was introduced late.

Moon-0xff commented 1 year ago

I tested this when 44 was still in beta so yeah, might be a change introduced after my tests. Have you tried to replace the Gtk.IconTheme statements to St.IconTheme?

Mershl commented 1 year ago
        const iconTheme = new St.IconTheme();
        return this.app_id && (iconTheme ? iconTheme.has_icon(iconName) : false) ? Gio.ThemedIcon.new(iconName) : null;

seems to work nicely on 44. I'm not sure when St12, the first version of this API supporting .IconTheme.has_icon was released though. This could likely break for 43 and lower.