position popup slider properly in wayland and make wayland optional

lukefromdc commented 1 year ago

Position the slider correctly in wayland (all four panel orientations tested) and allow passing --disable-wayland and --disable-in-process together for non-wayland, out of process builds

lukefromdc commented 1 year ago

Note that all attempts to permit an in-process build without wayland support failed as efforts to passHAVE_WAYLAND twice to gvc-stream-applet-icon.c repeated failed. For some reason passing IN_PROCESS worked in its place but this couples the options together. An attempt to build in-process without wayland support fails I an whole day of work I simply was NOT able to couple these two options together to be set by a single selector

The autotools documentation simply does not cover enough info on passing variables to preprocessing selectors in C files, and I don't have the understanding of autotools some of the others here do. I would much prefer to either have these options work independently or have one option control them both but will need help to get that done.

The reason for the second commit is complaints that the in-process applet has too many bugs elsewhere in its code that when in-process take down the whole panel. Since I don't get those bugs locally (the applet is very reliable here), I cannot fix them. This way the applet can be built in-process to support both x11 and wayland, or essentially the same as it always was for x11 only.

cwendling commented 1 year ago

I won't be able to test the Wayland part beyond compiling, but I'll try and make a good configure check. I think I'll go like that:

if --enable-wayland is passed, require in-process (automatic)
if --enable-in-process is passed, enable Wayland (automatic if available)
if --disable-in-process is passed, disable Wayland (automatic)
if there is an unsupported combination (like --enable-wayland --disable-in-process), fail with an error

The question might be what to default with: out-of-process like before (so no Wayland by default), or in-process? Or in-process if Wayland is detected, but that might be a tad unexpected that having the Wayland deps changes the in-process status automatically… opinions?

raveit65 commented 1 year ago

IHMO, out-of-process should be default until we state the MATE-Wayland desktop as stable and ready for production. Anyway, it's a small change to switch the default, so i don't mind very much. I would prefer enable-in-process and disable-in-process without depending on wayland, if it's possible. This might be better for testing bugs with in-process. Not everyone has a wayland VM running. Sadly i deleted my wayfire VM 1 or 2 yearys ago and forgot how to set it up.

cwendling commented 1 year ago

Here is a follow-up with the configure changes + a couple cleanups (code is better than a thousand words :smile:): https://github.com/cwendling/mate-media/commits/wayland-slider You or I can push to this PR if you're happy with the changes. If you're not, take what you like and leave the rest, I'm fine with anything

[edit]: beware: despite the fact I changed a couple things, I did not test this beyond the compilation phase. I also didn't review anything very carefully beside the configure script; e.g. neither the Makefile nor the code itself.

[edit2]: BTW, as I don't enable Wayland by default (c.f. @raveit65's comment as it depends on in-process), you'll need a flag for the CI to build it (either --enable-wayland or --enable-in-process will work if there's the right dependencies installed)

cwendling commented 1 year ago

IHMO, out-of-process should be default until we state the MATE-Wayland desktop as stable and ready for production.

Makes sense to me, so that's what I did

Anyway, it's a small change to switch the default, so i don't mind very much.

Yep, we can change the default fairly easily when we want to.

I would prefer enable-in-process and disable-in-process without depending on wayland, if it's possible. This might be better for testing bugs with in-process. Not everyone has a wayland VM running.

Agreed, these should be independent.

raveit65 commented 1 year ago

I tested this PR plus @cwendling improvements on top. Building with out-of-process/in-process/enable-wayland works fine. I will use in-process plus enable-wayland for better testing the applet on x11. Now testing those options with distcheck.

lukefromdc commented 1 year ago

I will get to work on those changes here, this sort of help is exactly what I need and I appreciate it

lukefromdc commented 1 year ago

NICE work! I tested all four sets of possible options:--disable-in-processwith --enable-waylanderrored out with the message that in-process is required for wayland. *A default build w no options built the traditional out of process applet, which won't run in wayland but won't take the panel down if it crashes in an x11 session

A build with --enable-in-process set picked up my installed wayland libraries and built wayland support too, positions the popup correctly in wayland and (on my machine) runs fine on x11
A build with --enable-in-process and--disable-wayland set (what would be built in-process if wayland libraries were not detected, I did not test removing them) runs same on x11 as above, will run on wayland but the slider popup appears in the center of the screen. This behavior allows confirming that the gtk-layer-shell library was not used and not needed for that build.

*A build with just --enable-wayland set automatically forced in-process as well, and again worked fine on both x11 and wayland. All the wayland builds put the slider over, under, or next to the applet depending on panel position.

There is one wayland-only bug: If you put the applet on a panel and then move that panel, both the old and new anchors are set, stretching the slider across the screen. I need to find a way to unset and reset the anchors when the panel is moved. Moving the applet within the panel does not change anchors only margins and works fine

cwendling commented 1 year ago

I added a couple more fixes and improvements to https://github.com/cwendling/mate-media/commits/wayland-slider (one being distcheck here as well)

cwendling commented 1 year ago

There is one wayland-only bug: If you put the applet on a panel and then move that panel, both the old and new anchors are set, stretching the slider across the screen. I need to find a way to unset and reset the anchors when the panel is moved.

I don't know the least bit about this, so I might be completely off, but I see you only ever call gtk_layer_set_anchor(…, TRUE), maybe you should call gtk_layer_set_anchor(…, FALSE) on the anchors you don't want?

lukefromdc commented 1 year ago

Setting unused anchors FALSE, and also zeroing out unused margins used in other orientations worked. Now the dock renders correctly in wayland after moving the panel. Thanks again

raveit65 commented 1 year ago

I squashed all improvement from @cwendling into your commit yesterday. This is currently running here.

offtopic on 2 days ago i got a panel crash during testing my new loudspeakers with my Asus Strix Raid DLX card and switching to mobo sound solution. I got 2 stacktraces. One from the panel and one from mate-settings-daemon. Looking in the traces it seems that pulseaudio was crashing both apps. I can post the traces if you are interested? Sadly pulseaudio or better pipewire-pulse in fedora becomes more unstable since pipewire is the new linux sound system. It would be better if mate-media would use pipewire directly instead of using pulseaudio which is a bit eol. Well, that's is a future task. What i want to say is that an external software can crash our applications including the panel when build in-process. In this case we can nothing do but i had other issues directly related to the applet in the past. I will take a look on them. So i am very happy about this PR which makes it possible to use in-process when any wayland session is needed. offtopic off

Btw. Is there any guide to set up a wayfire session with Mate?

lukefromdc commented 1 year ago

I don't know jack about pipewire (so to speak...) and I am probably not going to be able to diagnose your crash unless the stacktrace shows a source file line number within mate-media or (more likely) in libmate-mixer. If one of those comes up it should be possible to write code to catch and handle that case without needing to worry about the source of it.

Here's how my wayland session works:

1: install wayfire, which at least Debian Unstable now offers a prebuilt binary of. I did have to use some leftovers from my old wayfire build to get the desktop background to show up, and initially to use wayfire's panel. This was because not all the wayfire packages were offered, possibly due to a build failure. At any rate, you can test this with wayfire's default background color and no wf-panel just fine.

2: You will need these entries in ~/config/wayfire.ini

To launch mate-terminal when the desktop is empty

[command]

# Start a terminal
# https://github.com/alacritty/alacritty
binding_terminal = <super> KEY_ENTER
command_terminal = mate-terminal

To use mate-panel by default and start it with wayfire:


[autostart]

# Automatically start background and panel.
# Set to false if you want to override the default clients.
autostart_wf_shell = false

# Set the wallpaper, start a panel and dock if you want one.
# https://github.com/WayfireWM/wf-shell
#
# These are started by the autostart_wf_shell option above.
#
background = wf-background
panel = mate-panel

3: launch wayfire with dbus-launch wayfire.

4: you now can use KEY_ENTER to run some but not all commands. In about a minute all wayfire's settings will load and you will be able to run caja.

Current git master mate-panel is rather bare but will give you a window list. With my latest git branch and the same for mate-applets (in-process) you can get a full feature mate-panel. R

5: run nm-applet --indicatorto put the network manager (indicator mode) in the SNI-only wayland tray

6:(if using wayland-desktop branch) run caja --force-desktop

lukefromdc commented 1 year ago

Of course that can be changed. I am on the road now for today but can get to that tonight or tomorrow

On 7/2/2023 at 6:35 AM, "raveit65" @.***> wrote:

@raveit65 commented on this pull request.

@@ -181,5 +238,7 @@ if test "x$enable_panelapplet" = "xno"; then else echo " Building panel applet ......: yes" fi +echo " Building in-process ......: $enable_in_process" +echo " Wayland support ..........: $enable_wayland"

Only cosmetic, can the configure result (yes) under the other?

-- Reply to this email directly or view it on GitHub: https://github.com/mate-desktop/mate- media/pull/196#pullrequestreview-1509533537 You are receiving this because you authored the thread.

Message ID: <mate-desktop/mate- @.***>

cwendling commented 1 year ago

@raveit65 @lukefromdc the alignment should be fixed in one of the commits from my follow-up branch: https://github.com/cwendling/mate-media/commits/wayland-slider. It also has more substantial improvements, like fixing distcheck.

[edit] I just rebased the branch against the current state of this PR

raveit65 commented 1 year ago

Should we squash all commits in one with squash and merge option?

lukefromdc commented 1 year ago

Yes, that's fine with me and cleans up the overall commit history

mate-desktop / mate-media

position popup slider properly in wayland and make wayland optional #196