flathub / com.slack.Slack

https://flathub.org/apps/details/com.slack.Slack
35 stars 36 forks source link

Enable `WaylandWindowDecorations` feature when on Wayland. #162

Closed DeedleFake closed 1 year ago

flathubbot commented 2 years ago

Started test build 83273

flathubbot commented 2 years ago

Build 83273 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/81145/com.slack.Slack.flatpakref
WhyNotHugo commented 2 years ago

Crashes for me:

➜ flatpak run com.slack.Slack
F: Filesystem suffix "ro" is not applicable for --nofilesystem
F: Filesystem suffix "ro" is not applicable for --nofilesystem
F: Filesystem suffix "ro" is not applicable for --nofilesystem
F: Filesystem suffix "ro" is not applicable for --nofilesystem
[2 preload-host-spawn-strategy] Running: /app/bin/zypak-helper child - /app/extra/lib/slack/slack --type=zygote --enable-crashpad
F: Filesystem suffix "ro" is not applicable for --nofilesystem
F: Filesystem suffix "ro" is not applicable for --nofilesystem
F: Filesystem suffix "ro" is not applicable for --nofilesystem
F: Filesystem suffix "ro" is not applicable for --nofilesystem
Initializing local storage instance
(electron) Sending uncompressed crash reports is deprecated and will be removed in a future version of Electron. Set { compress: true } to opt-in to the new behavior. Crash reports will be uploaded gzipped, which most crash reporting servers support.
[2:0325/132946.502694:ERROR:bus.cc(397)] Failed to connect to the bus: Failed to connect to socket /run/dbus/system_bus_socket: No such file or directory
[2:0325/132946.502740:ERROR:bus.cc(397)] Failed to connect to the bus: Failed to connect to socket /run/dbus/system_bus_socket: No such file or directory
LaunchProcess: failed to execvp:
xdg-settings
[53:0325/132946.617069:ERROR:gpu_init.cc(454)] Passthrough is not supported, GL is egl, ANGLE is
[2:0325/132946.633987:ERROR:bus.cc(397)] Failed to connect to the bus: Failed to connect to socket /run/dbus/system_bus_socket: No such file or directory
[2:0325/132946.634043:ERROR:bus.cc(397)] Failed to connect to the bus: Failed to connect to socket /run/dbus/system_bus_socket: No such file or directory
[2:0325/132946.634084:ERROR:bus.cc(397)] Failed to connect to the bus: Failed to connect to socket /run/dbus/system_bus_socket: No such file or directory
[53:0325/132946.638328:ERROR:sandbox_linux.cc(377)] InitializeSandbox() called with multiple threads in process gpu-process.
[2:0325/132946.667341:ERROR:browser_main_loop.cc(267)] Gtk: gtk_widget_add_accelerator: assertion 'GTK_IS_ACCEL_GROUP (accel_group)' failed
[2:0325/132946.679871:ERROR:cursor_loader.cc(116)] Failed to load a platform cursor of type kNull
[0325/132946.680449:ERROR:scoped_ptrace_attach.cc(27)] ptrace: Operation not permitted (1)

This seems to be mostly a cosmetic change, I'd rather wait until it's stabilized rather than enable it early like this.

DeedleFake commented 2 years ago

I get that crash with a native installation of Slack, but not with the Flatpak version, and I get the crash regardless of the window decorations. It's a weird one. It seems to affect a lot of Electron apps at the moment. See electron/electron#32436.

This seems to be mostly a cosmetic change

It is not purely cosmetic if you're on a compositor that doesn't support server-side window decorations, such as Mutter or Enlightenment. In that case, running it with the Wayland backend produces a window with no decorations at all, which makes it kind of annoying to use, though not impossible.

tdgroot commented 2 years ago

On my wayland environment the DISPLAY environment variable is present, so I think the [ -z \"$DISPLAY\" ] check can be disregarded. The app is currently not being executed with the ozone flags

WhyNotHugo commented 2 years ago

On my wayland environment the DISPLAY environment variable is present, so I think the [ -z \"$DISPLAY\" ] check can be disregarded.

This is intentional: Slack on Wayland is not officially supported. If you want to force it to run with Wayland, you need to restrict it from accessing X11 (e.g.: using Flatseal).

WhyNotHugo commented 2 years ago

I can't test this because Ozone support is broken on current versions of Wayland. I believe it's due to this?

https://bugs.chromium.org/p/chromium/issues/detail?id=1246834

If another maintainer wants to merge this, I've no objection, but given that I can't test it, I won't do it myself.

y0ast commented 2 years ago
flatpak run --socket=wayland com.slack.Slack --ozone-platform=wayland --enable-features=WaylandWindowDecorations 

Works well for me with the newest update. The decoration is definitely necessary to enable window resizing in Gnome 42.

DeedleFake commented 2 years ago

You can also resize windows by holding super and middle mouse and dragging.

y0ast commented 2 years ago

Ah you're right! And by repeatedly resizing I can even move it wherever I want. I prefer the CSD though.

DeedleFake commented 2 years ago

No need for resizing repeatedly for that. Super and left mouse moves the window. But yeah, actual window decorations would be much nicer.

r2binx commented 2 years ago

Should be merged with #178 IMO. It seems they've upgraded the Electron version which finally fixed some bugs regarding window decorations (mainly https://github.com/electron/electron/pull/34955) and works well now. Has been working fine for me for a while on Arch & Fedora 36.

LorbusChris commented 1 year ago

This works for me today:

flatpak run --socket=wayland com.slack.Slack --ozone-platform-hint=auto --enable-features=WaylandWindowDecorations
Queuecumber commented 1 year ago

When I tested this it crashed anytime I tried to open the window from the tray icon. I think there is still an outstanding electron bug for fixing it: https://github.com/electron/electron/issues/35657

LorbusChris commented 1 year ago

It works for me on Fedora Silverblue 37 preview with the latest Flatpak

LorbusChris commented 1 year ago
$ cat ~/.local/share/applications/com.slack.Slack.desktop 
[Desktop Entry]
Name=Slack
Comment=Slack Desktop
GenericName=Slack Client for Linux
Exec=/usr/bin/flatpak run --socket=wayland --branch=stable --arch=x86_64 --command=slack --file-forwarding com.slack.Slack --ozone-platform-hint=auto --enable-features=WaylandWindowDecorations @@u %U @@
Icon=com.slack.Slack
Type=Application
StartupNotify=true
Categories=GNOME;GTK;Network;InstantMessaging;
MimeType=x-scheme-handler/slack;
X-Desktop-File-Install-Version=0.26
StartupWMClass=Slack
X-Flatpak-RenamedFrom=slack.desktop;
X-Flatpak=com.slack.Slack
Queuecumber commented 1 year ago

Right and if you close slack to the tray and then open it again it doesn't crash?

LorbusChris commented 1 year ago

you're right it crashes out when re-opening from tray.

Queuecumber commented 1 year ago

yeah so I think once the electron devs address that it will be the final issue on wayland

DeedleFake commented 1 year ago

yeah so I think once the electron devs address that it will be the final issue on wayland

They've fixed the maximized decoration issue?

Queuecumber commented 1 year ago

Which issue is that?

DeedleFake commented 1 year ago

The CSDs don't recognize the maximized state correctly and still show the shadow, causing a weird gap between the window and the edge of the area it's maximized into.

r2binx commented 1 year ago

yeah so I think once the electron devs address that it will be the final issue on wayland

They've fixed the maximized decoration issue?

Yes, the PR I mentioned in my earlier comment here https://github.com/flathub/com.slack.Slack/pull/162#issuecomment-1243696442 included a fix for that.

When I tested this it crashed anytime I tried to open the window from the tray icon. I think there is still an outstanding electron bug for fixing it: https://github.com/electron/electron/issues/35657

This is unrelated to the WaylandWindowDecorations flag though.

Queuecumber commented 1 year ago

That's true, I guess if people opt in to Wayland then the decorations should be turned on, and at least that part does work with the latest release

Good luck getting anyone to merge this though

LorbusChris commented 1 year ago

@WhyNotHugo @barthalion @druizz90 since the crash upon reopening is not related to this, could this be merged now?

barthalion commented 1 year ago

bot, build

flathubbot commented 1 year ago

Queued test build for com.slack.Slack.

flathubbot commented 1 year ago

Started test build 57776

flathubbot commented 1 year ago

Build 57776 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/40401/com.slack.Slack.flatpakref