mate-desktop / mate-applets

Applets for use with the MATE panel
http://www.mate-desktop.org
GNU General Public License v2.0
79 stars 67 forks source link

Port all wayland-usable applets to in-process #651

Closed lukefromdc closed 1 year ago

lukefromdc commented 1 year ago

Most mate-panel applets work in wayland the same or nearly the same as on x11 after porting to in-process.

*Accessx not ported, hard dependency on x11 for keyboard handling. The standalone xkb library could offer a future way to fix this

*The battstat applets works same in x11 and wayland, save that again notifications cannot yet be used

*The charpick applet itself will function in both x11 and wayland, but the primary paste preference does not work in wayland-and even resets the gsettings preference to disable it. Fixing this is probably the job of a wayland session manager. No change in x11 behavior

*Cpufreq and drivemount work same in x11 and wayland as they did in x11 before

*Geyes not ported as the applet cannot see the pointer in wayland once it moves outside the panel due to wayland restrictions on one app accessing another's windows. This is considered a security feature in wayland as it makes clickjacking much harder. Also when in-process, geyes improperly computes pointer position as it becomes relative to the mate-panel window not the GtkPlug window used for out of process.

*mateweather, multiload,netspeed, stickynotes all seem to work the same in wayland as in x11 and no behavioral differences were noticed in (ONE user) testing

*The timer applet must be used with the dialog not the notification in wayland, as we don't have a wayland-compatable notification daemon yet and the applet will freeze on a failed notification. No change x11 behavior.

*Trash applet works same in x11 and wayland if a wayland-compatable file manager is installed

cwendling commented 1 year ago

I can help with the Autotools side of things if I know what's wanted. I'll look at this probably this afternoon -- or maybe I'll still find time this morning, not sure.

lukefromdc commented 1 year ago

Thanks. What's needed is a way to on all of the applets except Accessx (only useful in X) and Geyes (cannot track pointer outside panel in wayland) to build in-process as a library in the selected libdir, or out of process as a standalone binary (as before) in libexec. I have a variable set in configure.ac (ENABLE_IN_PROCESS I think) and the timer applet can use it, but no matter what I tried with the Makefile.am files in battstat and trash I could not stop autotools from trying to use both sections of the relevent Makefile.am

Code that worked elsewhere failed there, with any variable I set somehow ignored

cwendling commented 1 year ago

Thanks. What's needed is a way to on all of the applets except Accessx (only useful in X) and Geyes (cannot track pointer outside panel in wayland) to build in-process as a library in the selected libdir, or out of process as a standalone binary (as before) in libexec.

Done. I also added support for AccessX and GEyes, as I think in/out of process is orthogonal to Wayland support, and having a --enable-in-process switch that doesn't make everything in-process felt weird.

I also took the liberty of pushing to this PR because it seemed simpler for everyone, but I can retire my commits if you want, and I can also squash some commits together if we're happy with the result (I'd rather keep some separated, like in/out process vs. Wayland, but that should be fairly easy).

DISCLAIMER: I didn't actually test this beyond compilation yet. I know it's not great, but this work was painstaking enough that I'm a bit fed up with it right now, and figured you guys could test it in parallel as well. I'll test it, but not right now.

I have a variable set in configure.ac (ENABLE_IN_PROCESS I think) and the timer applet can use it, but no matter what I tried with the Makefile.am files in battstat and trash I could not stop autotools from trying to use both sections of the relevent Makefile.am Code that worked elsewhere failed there, with any variable I set somehow ignored

Ah yeah Autotools can be tricky. I guess you encountered this? https://www.gnu.org/software/automake/manual/html_node/Objects-created-both-with-libtool-and-without.html Unfortunately Automake is not smart enough to realize the conditionals make the problem irrelevant here, but the fix is easy enough (just add a target-specific _CFLAGS). And generating separate objects also makes it slightly more resilient if changing configuration options without cleaning the builddir, as Autotools isn't the best at rebuilding objects when only the environment flags changed.

lukefromdc commented 1 year ago

Thank you very much, I was stumped by this and really needed the help. Commiting these directly to the PR saves me some work too, will test now and look it over. I had written another version of configure.ac to allow selecting individual applets but set it aside for now due to the problems with "Objects-created-both-with-libtool-and-without"

lukefromdc commented 1 year ago

This make all the applets successfully build in-process or out. EDIT: broken applets on initial test out of process may have been nothing more than forgetting to restart mate-panel

Most apples worked in-process except Netspeed (silently errored out)and Multiload(silently errored out). AccessX won't be useful in wayland it works fine in process or out in X11. One or more of the applets generated these non-crashing errors, again should not be hard to fix. I should be able to take it from here. Thanks and thanks again for the assist!


(mate-panel:154145): GLib-CRITICAL **: 13:08:51.565: g_variant_ref: assertion 'value != NULL' failed

** (mate-panel:154145): WARNING **: 13:08:51.565: GDBus.Error:org.freedesktop.DBus.Error.NoReply: Message recipient disconnected from message bus without replying

this non-crashing error in-process w all applets installed:

(mate-panel:238025): GLib-GObject-WARNING **: 13:18:03.684: g_object_connect: invalid signal spec "destroy"

(mate-panel:238025): Gtk-CRITICAL **: 13:18:03.690: gtk_widget_realize: assertion 'widget->priv->anchored || GTK_IS_INVISIBLE (widget)' failed

(mate-panel:238025): Gtk-CRITICAL **: 13:18:03.690: gtk_widget_realize: assertion 'widget->priv->anchored || GTK_IS_INVISIBLE (widget)' failed

(mate-panel:238025): Gtk-WARNING **: 13:18:04.337: Negative content width -1 (allocation 1, extents 1x1) while allocating gadget (node button, owner WnckButton)

(mate-panel:238025): Gtk-WARNING **: 13:18:04.337: Negative content width -1 (allocation 1, extents 1x1) while allocating gadget (node button, owner WnckButton)
lukefromdc commented 1 year ago

Looks like

(mate-panel:244415): Gtk-CRITICAL **: 13:29:31.160: gtk_widget_realize: assertion 'widget->priv->anchored || GTK_IS_INVISIBLE (widget)' failed

(mate-panel:244415): Gtk-CRITICAL **: 13:29:31.160: gtk_widget_realize: assertion 'widget->priv->anchored || GTK_IS_INVISIBLE (widget)' failed

in-process is geyes, which also has issues in-process correctly following the pointer unless put on the left side of the screen. I will need to add code to get the applet position along the panel to fix that. (mate-panel:245924): GLib-GObject-WARNING **: 13:32:17.166: g_object_connect: invalid signal spec "destroy" is AccessX, should not be hard to find

lukefromdc commented 1 year ago

Trash out of process didn't work because of a typo in /data/Makefile.am "libexecir" instead of "libexecdir" easy fix, pushing now. Nothing to worry about, this happens to all of us

lukefromdc commented 1 year ago

All applets now build and run out of process, now to fix some in-process runtime errors that cropped up

lukefromdc commented 1 year ago

Now everything basically works in-process too. Couple runtime warnings to deal with, and if possible get geyes to properly follow the pointer when out of process. If not we may have to be able to build it out of process with everything else in-process as an option, probably specified as --enable-geyes-in-process (experimental)

lukefromdc commented 1 year ago

There is an issue (at least in some themes) in AccessX out of process showing a rotated background on a panel with an image background if the panel is changed from vertical to horizontal or horizontal to vertical. This is inherited from master and not caused by any of these commits-and is fixed by building in-process. An in-process build eliminates the GtkPlug window on which the background is redrawn.

lukefromdc commented 1 year ago

We are now rid of the runtime warning from geyes, but it still does not properly track the pointer when built in-process. Note that in wayland it cannot track the pointer when out of the panel due to wayland restrictions on one app watching another app's windows

lukefromdc commented 1 year ago

Last force-push makes geyes track the pointer correctly when in-process by compensating for applet position on the panel, no change in out of process behavior.

Ready for testing

raveit65 commented 1 year ago

34 commits at the moment 0_o , can this be sorted and squashed a bit? With git rebase -i <last-commit-hash-before-this-PR> you can first sort and then squash/edit/reword the branch easily. Do that step by step to avoid conflicts. Personal i do always edit a commit instead of pushing a fix for a fix to branch, which avoid squashing commits. Sorry, if you know that already. I suggest to sort by applets and generally stuff and sub-sorting by in/out-process and wayland-only. Anyway, i will squash all in one commit to build my RPM for testing the actual state.

lukefromdc commented 1 year ago

Oh yeah this will need squashing. A per-applet squash won't be possible on making in-process builds possible directly, and such work absolutely requires a backup copy of the rep. Would be very easy to break it

lukefromdc commented 1 year ago

I just started working on squashing this

lukefromdc commented 1 year ago

Commit history squashed to something easier to work with.

raveit65 commented 1 year ago

My actual testing results: Multiload applet doesn't display anymore in x11 and wayland. brightness.applet - doesn't display under wayland dictionary - doesn't display under wayland trash-applet - no right-click menu under wayland, instead you will get the panel menu.

Those applets are there in add-to-pannel gui.

raveit65 commented 1 year ago

When building applets out-of-process the multiload applet displays well under X11.

lukefromdc commented 1 year ago

I fixed multiload in the last force-push, one line needed in it's data/Makefile.am (the one to write true or false for "in-process=" hadn't made it in with the transition to selectable in or out of process builfs

I will have to re-check trash to ensure my fix for clicking on it in wayland didn't get lost, i can easily bring it back if it did

lukefromdc commented 1 year ago

Ahh I see it now-the trashapplet's right-click context menu was lost in and only in wayland. Should be easy to fix

raveit65 commented 1 year ago

Ok, multiload do run after a rebuild. But trash-applet doesn't have a menu Also brightness and dictionary do not run under wayland. Not sure if they were OK in my first test.

lukefromdc commented 1 year ago

In trashapplet, it seems there's an issue with clicks being "eaten." In wayland, opening trash on button release doesn't work, the panel eats the click and worse yet responds by moving the panel to the top if it's on the bottom. Opening on button press works fine with left click. That however then causes the right click to be ignored, possibly the original reason for using left button release to open the trash. Thus we will need an explicit command to open the right click menu on button 3 in trash_applet_button_release which remains the function name at this time

lukefromdc commented 1 year ago

Using if ((event->button == 1) || (event->button == 3)) `` in trash_applet_button_release will open the trash on left or right click, proving we still have access to both clicks. Just need to redirect button 3 to opening the context menu now

raveit65 commented 1 year ago

Using if ((event->button == 1) || (event->button == 3)) in trash_applet_button_release `` will open the trash on left or right click, proving we still have access to both clicks. Just need to redirect button 3 to opening the context menu now

I recall that this was working in wayland during my first test.

lukefromdc commented 1 year ago

Got it working with code from cpufreq-applet. Seems we had to exclude some of the other cases. At first the right click worked but not the left click, then the reverse of that but never had a problem w this from cpufreq-applet so borrowed that code

lukefromdc commented 1 year ago

Squashed all the cleanups too

raveit65 commented 1 year ago

I just sent a mail to travis-ci for getting new credits again.

lukefromdc commented 1 year ago

Thanks

lukefromdc commented 1 year ago

Brightness and inhibit are provided by mate-power-manager, dictionary by mate-utils. Dictionary can certainly be ported in that package, brightness and inhibit may be using Xorg-specific functions but I will have to check.

raveit65 commented 1 year ago

Ahh ok, those applets are from different packages. I think also the kill-process applet use X functions to find the windows and should be disable for wayland.

lukefromdc commented 1 year ago

force-quit is provided by mate-panel and indeed we should disable it in wayland

lukefromdc commented 1 year ago

Speaking of applets provided by mate-panel, connect to server works well, drawer works but renders at the start of the panel so needs code to move it to the applet. Separator and search for files work fine in wayland. Do not disturb does not work (does it work by creating a status icon though?), logout works but right now doesn't have a session manager to connect to so does nothing. Shut down same behavior as logout (nothing to connect to) if present after adding in an Xorg session but for some reason is not shown in the add to panel dialog in wayland.

All of these are applets provided by mate-panel, I will have to iterate through the ones with issues

raveit65 commented 1 year ago

My actual results in wayfire session:

battstat loads fine, menus are there. charpick loads fine, menus are there. command loads fine, menus are there. CPUfreq loads fine, menus are there, cpu governor can't be selected, not related to this PR. Drivemount loads fine, menus are there. weather, loads fine, menus are there, but has problems to load selected location, not related to this PR, ihmo. multiload , loads fine, menus are there. netspeed , loads fine, menus are there. sticky notes ok, but gives runtime warning when removing the applet from the panel.

(mate-panel:195905): Wnck-WARNING **: 20:15:09.628: libwnck is designed to work in X11 only, no valid display found

(mate-panel:195905): Wnck-CRITICAL **: 20:15:09.629: wnck_screen_force_update: assertion 'WNCK_IS_SCREEN (screen)' failed

timer-applet OK but runtime warning when timer ends, not related to PR

(mate-panel:196085): Atk-CRITICAL **: 20:19:09.594: atk_object_set_description: assertion 'description != NULL' failed

trash-applet , loads fine, menus are there.

I think the warnings from sticky-notes-applet are from mate-panel code, but we should investigate why this happens. All other applets are fine now.

I will do a code-format review the next days.

raveit65 commented 1 year ago

Do not disturb does not work (does it work by creating a status icon though?)

I think it is out-of-process. From my spec file of mate-notification-daemon %{_datadir}/dbus-1/services/org.mate.panel.applet.MateNotificationAppletFactory.service

lukefromdc commented 1 year ago

I will look into the stickynotes warning, usually that's signals not getting disconnected. Warnings are invisible in terminal for out of process so these may or may not be new

lukefromdc commented 1 year ago

Last force-push fixed the stickynotes warnings, by limiting some x11 and wnck stuff to x11 only. Note that in wayland there is no easy way to exclude the note windows from the window list, and they are decorated. It would probably require a new function in libmate-panel-applet to let applets talk to the window list, and stickynotes would initially be the only user of it.

The decorated windows could be fixed and displaying notes on all four windows enabled by using gtk-layer-shell, but this would require adding that as a new conditional dependency for the entire package, plus another config option to enable wayland support for sticky notes just to undecorate them and allow showing notes on all desktops.

Also note that hiding notes (whether by clicking on the desktop or from the menu item) didn't work the last time I tested sticky notes while testing another PR quite a while ago. Doesn't in wayland but doesn't in x11 either so same behavior there and probably same as current git master in x11.

raveit65 commented 1 year ago

Last force-push fixed the stickynotes warnings, by limiting some x11 and wnck stuff to x11 only.

Confirmed, warning is gone, thanks.

IHMO, all applets should load/unload to panel, menus should be there and preferences windows should be open/close nicely with this PR. Specific applet functions which do not work at this moment can be fix in single PRs later. Other things like session-management, m-s-d and open caja issues are more important, ihmo.

Battstat gives a warning when removing from panel, but for me this can be fix later. 1 open battstatt preferences 2 close preferences 3 remove battstat from panel

(mate-panel:96778): Gtk-CRITICAL **: 12:13:29.975: gtk_widget_destroy: assertion 'GTK_IS_WIDGET (widget)' failed

All other applets load and unload fine under wayland.

@cwendling @lukefromdc With the interactive rebase command it's possible to split the cleanup commits into single commits for every applet, which can be squash in the main commit for every applet. Ie. for last commit:

  1. choose edit in vi rebase terminal. Now all changes are uncommented and you can add them again to git staging area.
  2. git add accessx-status/src/applet.c
  3. git commit -a -m "accessx cleanup"

Now the same procedure for trashapplet/src/trashapplet.c and at least for the remaining sticky-notes changes. I would do this splitting first for the first cleanup comment to avoid conflicts/problems. https://github.com/mate-desktop/mate-applets/pull/651/commits/d9e90ff16a0d4c0f4a8e2f7f7a94cc53e8dd1ed1

I can do that for you if you like.

lukefromdc commented 1 year ago

I have never attempted to split up a commit. If you can handle that it's greatly appreciated

On 7/16/2023 at 7:16 AM, "raveit65" @.***> wrote:

Last force-push fixed the stickynotes warnings, by limiting some x11 and wnck stuff to x11 only.

Confirmed, warning is gone, thanks.

IHMO, all applets should load/unload to panel and preferences windows should be open/close nicely with this PR. Specific applet functions which do not work at this moment can be fix in single PRs later. Other things like session-management, m-s-d and open caja issues are more important.

Battstat gives a warning when removing from panel, but for me this can be fix later. 1 open battstatt preferences 2 close preferences 3 remove battstat from panel

(mate-panel:96778): Gtk-CRITICAL **: 12:13:29.975: 
gtk_widget_destroy: assertion 'GTK_IS_WIDGET (widget)' failed

All other applets load and unload fine under wayland.

@cwendling @lukefromdc With the interactive rebase command it's possible to split the cleanup commits into single commits for every applet, which can be squashed in the main commit for every applet. Ie. for last commit:

  1. choose edit in vi rebase terminal. Now all changes are uncommented and you can add them again to git staging area.
  2. git add accessx-status/src/applet.c
  3. git commit -a -m "accessx cleanup"

Now the same procedure for trashapplet/src/trashapplet.c and at least for the remaining sticky-notes changes. I would do this splitting first for the first cleanup comment to avoid conflicts/problems. https://github.com/mate-desktop/mate- applets/pull/651/commits/d9e90ff16a0d4c0f4a8e2f7f7a94cc53e8dd1ed1

I can do that for you if you like.

-- Reply to this email directly or view it on GitHub: https://github.com/mate-desktop/mate-applets/pull/651#issuecomment- 1637056276 You are receiving this because you were mentioned.

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

raveit65 commented 1 year ago

Done, i split up all commits with changes from files from different applets and sorted the history. The final squashing i leave for you to avoid that all commits get my name. I hope i didn't break anything. Please review. In a test branch i squashed the commits and CI building was fine. https://github.com/mate-desktop/mate-applets/commits/in-process-test-commit-squashed

Edit: A tutorial to split commits https://internalpointers.com/post/split-commit-into-smaller-ones-git After git reset HEAD~1 command i used the gitg gui to create new commits.

lukefromdc commented 1 year ago

Testing now, both ways in x11 plus in-process on wayland

lukefromdc commented 1 year ago

Everything seemd to work in the test branch: all applets out of process x11, in-process x11, and in-process wayland where the dialog came up to bypass applets flagged as not supported in wayland. Even a test stickynote created on an x11 desktop came up on the wayfire desktop as well. Running mate-panel --replace did not bring up any warnings in terminal

lukefromdc commented 1 year ago

https://github.com/mate-desktop/mate-applets/tree/in-process-squashed-to-one Is the same commits squashed into one with a well-commented commit message preserving the history of how they came to be. Test builds coming in minutes.

if this meets your approval it can be renamed and force-pushed as in-process for merging

lukefromdc commented 1 year ago

https://github.com/mate-desktop/mate-applets/commit/675ee7474fedde6d0c28d60e9ddbd19587bf1634 is the final single commit (barring further edits) in https://github.com/mate-desktop/mate-applets/tree/in-process-squashed-to-one that can replace everything in the in-process branch via a rename and force-push for final merge, thus keeping it all connected to this history.

Both out of process and in-process builds finished without any apparent issues. Tested with two panels and every applet from this package added.

Both out of process and in-process builds ran in x11 without putting warnings in a terminal the panel was being test run from (via mate-panel --replace). In-process build also worked as expected in wayland, warnings to go with the dialogs for unsupported applets but nothing else./

raveit65 commented 1 year ago

Really, you squashed all commits in one.....!!! Sorry, i spend 2 hours of my life time to split commits of your work. This is really a killing joke and sucks. .... or your skills are sufficient to use the interactive rebase command. I am out.......sigh

lukefromdc commented 1 year ago

I kept a backup, so if you want one commit per applet I can do that too almost as easily, no problem

lukefromdc commented 1 year ago

I can rebase easily enough, splitting commits was something I have never done and would have to do manually w files.

I have reordered commits and squashed commits using git rebase -i over the years (though not much recently until now) but have never split a commit up. That was the part that was beyond me, though I could have done it file-wise rather than commit wise to a new branch, bringing files over for one applet at a time.

lukefromdc commented 1 year ago

Will switch this (again from your work) to one per applet. That's why I kept a backup. I didn't know what you wanted, sorry about that. Will force-push again, one per applet

lukefromdc commented 1 year ago

Done: one commit for each applet, and the final commit with the option to build in-process or out, plus the cleanups. Again I have backups if needed

lukefromdc commented 1 year ago

It was the words "The final squashing i leave for you to avoid that all commits get my name." that confused me. I thought you wanted all in one with a composite commit message from that line. Sorry about that but it's fixed now.

Final count is 13 commits, one for each applet and the final to make in-process optional and apply cleanups.

lukefromdc commented 1 year ago

Will test again, thanks