mate-desktop / mate-terminal

The MATE Terminal Emulator
http://www.mate-desktop.org
GNU General Public License v3.0
133 stars 73 forks source link

Enable wayland support #342

Closed yetist closed 4 years ago

yetist commented 4 years ago

Now it can run both X11 and wayland.

yetist commented 4 years ago

As I understand it, startup_id only need a random string, and the slowly_and_stupidly_obtain_timestamp function can be removed if we modify code like this:

--- a/src/terminal.c
+++ b/src/terminal.c
@@ -602,12 +602,8 @@ main (int argc, char **argv)

        if (options->startup_id == NULL)
        {
-               /* Create a fake one containing a timestamp that we can use */
-               Time timestamp;
-
-               timestamp = slowly_and_stupidly_obtain_timestamp (GDK_DISPLAY_XDISPLAY (gdk_display_get_default ()));
-
-               options->startup_id = g_strdup_printf ("_TIME%lu", timestamp);
+               options->startup_id = g_uuid_string_random ();
+               g_print("startup id: %s\n", options->startup_id);
        }

        if (options->use_factory)

Is that my understanding right? @rbuj

rbuj commented 4 years ago

@yetist Yes, I think also that g_uuid_string_random is compatible with StartupNotify for getting valid IDs. Congratulations, I think it's a great improvement!

Edit: test

$ DESKTOP_STARTUP_ID=f81d4fae-7dec-11d0-a765-00a0c91e6bf6 /usr/bin/mate-terminal
raveit65 commented 4 years ago

@wmww Can you please do some testing and a review?

yetist commented 4 years ago

There are still some gdkx11* function calls which don't have a wayland alternative:

These files src/egg* seems a library, and terminal didn't call the functions. Several other repos in the MATE component use similar files, and I recommend maintaining them in one place.

terminal-utils.c and terminal-window.c has already use GDK_WINDOWING_X11 macro.

wmww commented 4 years ago

I don't know too much about those files, but I suspect they were originally split out into repos for a reason. Now's probably not the time to bring them together again. If a centralized utility library were made, I suspect one would want to rewrite a lot of these functions as they don't seem to have gotten an overhaul for a very very long time.

I recommend dropping any unused functions, especially unused functions relying on X11. Hopefully we can make it so all X11 calls are behind both runtime and compile-time checks. Even if we're not planning on compiling without X11 any time soon, aspiring to be able to is a good way to make sure there are no edge-cases that trigger X-specific calls (and will crash on Wayland).

rbuj commented 4 years ago

Maybe we can drop eggdesktopfile.c and use GDesktopAppInfo.

$ find . -name eggdesktopfile.c
./mate-session-manager/egg/eggdesktopfile.c
./engrampa/copy-n-paste/eggdesktopfile.c
./mate-terminal/src/eggdesktopfile.c
./caja/libegg/eggdesktopfile.c
./atril/cut-n-paste/smclient/eggdesktopfile.c
./mate-utils/gsearchtool/libeggsmclient/eggdesktopfile.c
./pluma/pluma/smclient/eggdesktopfile.c
./mate-panel/mate-panel/libegg/eggdesktopfile.c
rbuj commented 4 years ago

xsmp support was removed from gnome at https://github.com/GNOME/gnome-terminal/tree/364115335f4e43908da3dba936e5d42edad4d8d0

yetist commented 4 years ago

These files are now included in 8 repos, and there are very different, maintenance is a big trouble.

$ find . -name eggdesktopfile.c -exec md5sum {} \;
6e0535f5234a4f3285c218f29da2c9f0  ./engrampa/copy-n-paste/eggdesktopfile.c
43faf149c0f91b3533695943c2e83e88  ./mate-session-manager/egg/eggdesktopfile.c
874b539f5a488e8b15999b3cbceb60a7  ./mate-panel/mate-panel/libegg/eggdesktopfile.c
912b4ecce3b78361f4a433e33c105fdd  ./caja/libegg/eggdesktopfile.c
ce5c65864f6b0b33324a0e2561ffa2f6  ./pluma/pluma/smclient/eggdesktopfile.c
f902f35e65e35096959d19f69c8c5eef  ./atril/cut-n-paste/smclient/eggdesktopfile.c
2131e56167cbed03aa1f94d7f5498a3e  ./mate-terminal/src/eggdesktopfile.c
c2a21c28c505b0afbca45328a4136ef3  ./mate-utils/gsearchtool/libeggsmclient/eggdesktopfile.c
$ find . -name eggsmclient-xsmp.c -exec md5sum {} \;
9402650985fc08446ed2ee2d97ec5295  ./engrampa/copy-n-paste/eggsmclient-xsmp.c
a60e7a417ef9acf0116dd246b397e0fc  ./mate-session-manager/egg/eggsmclient-xsmp.c
6e3a4024624b381ce617e215f91d5c7b  ./mate-panel/mate-panel/libegg/eggsmclient-xsmp.c
d471856e6efb7c93a80fe2be71b2183e  ./caja/libegg/eggsmclient-xsmp.c
6ee5ae7465e8a2f56f4d9874e1c6de76  ./pluma/pluma/smclient/eggsmclient-xsmp.c
db550b33b81337766f7b844db1a36f57  ./atril/cut-n-paste/smclient/eggsmclient-xsmp.c
712a62614da15fe74af4ee2fedbee20c  ./mate-terminal/src/eggsmclient-xsmp.c
f8c7236582697c5cd8d2ba6d3aaf55af  ./mate-utils/gsearchtool/libeggsmclient/eggsmclient-xsmp.c
lukefromdc commented 4 years ago

When these files orginated, they were all supposed to be cut and pasted from what must have been some other project, or I'm thinking maybe Nautilus. Nautilus was Easel's free and open source browser they hoped to make money on by offering paid remote server access browsable by a good filebrowser, such as Nautilus. They had big ambitions thus the use of subdirectories inside /src for file-manager and they were figuring other functions. Their ambitions never panned out but Nautilus became part of GNOME. No idea if Easel orginated this code or cut and pasted it from elsewhere but such names as "libegg" containing "e" suggest Easel may have been the origin. In short, this was to be cut and pasted from that upstream and never modifed inside the repo, thus the name "cut-n-paste." We have not been able of course to do that, and when Easel died I would not be surprised if GNOME could not either. We can possibly sync them up, however, and then set one repo (possibly caja) as the master repo from which all others use the files unchanged,

On 4/24/2020 at 3:03 AM, "Wu Xiaotian" wrote: These files are now included in 8 repos, and there are very different, maintenance is a big trouble.

$ find . -name eggdesktopfile.c -exec md5sum {} ;

6e0535f5234a4f3285c218f29da2c9f0 ./engrampa/copy-n-paste/eggdesktopfile.c 43faf149c0f91b3533695943c2e83e88 ./mate-session-manager/egg/eggdesktopfile.c 874b539f5a488e8b15999b3cbceb60a7 ./mate-panel/mate-panel/libegg/eggdesktopfile.c 912b4ecce3b78361f4a433e33c105fdd ./caja/libegg/eggdesktopfile.c ce5c65864f6b0b33324a0e2561ffa2f6 ./pluma/pluma/smclient/eggdesktopfile.c f902f35e65e35096959d19f69c8c5eef ./atril/cut-n-paste/smclient/eggdesktopfile.c 2131e56167cbed03aa1f94d7f5498a3e ./mate-terminal/src/eggdesktopfile.c c2a21c28c505b0afbca45328a4136ef3 ./mate-utils/gsearchtool/libeggsmclient/eggdesktopfile.c $ find . -name eggsmclient-xsmp.c -exec md5sum {} ; 9402650985fc08446ed2ee2d97ec5295 ./engrampa/copy-n-paste/eggsmclient-xsmp.c a60e7a417ef9acf0116dd246b397e0fc ./mate-session-manager/egg/eggsmclient-xsmp.c 6e3a4024624b381ce617e215f91d5c7b ./mate-panel/mate-panel/libegg/eggsmclient-xsmp.c d471856e6efb7c93a80fe2be71b2183e ./caja/libegg/eggsmclient-xsmp.c 6ee5ae7465e8a2f56f4d9874e1c6de76 ./pluma/pluma/smclient/eggsmclient-xsmp.c db550b33b81337766f7b844db1a36f57 ./atril/cut-n-paste/smclient/eggsmclient-xsmp.c 712a62614da15fe74af4ee2fedbee20c ./mate-terminal/src/eggsmclient-xsmp.c f8c7236582697c5cd8d2ba6d3aaf55af ./mate-utils/gsearchtool/libeggsmclient/eggsmclient-xsmp.c

You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or unsubscribe.

raveit65 commented 4 years ago

We can possibly sync them up, however, and then set one repo (possibly caja) as the master repo from which all others use the files unchanged,

Or better mate-desktop-libs to avoid new dependency pain between repos?

yetist commented 4 years ago

I think to use git submodule is a way, in the future, we can use meson subproject.

I have created https://github.com/yetist/libegg and used some branches for different repos currently using it. I created a submodule test for caja, https://github.com/yetist/caja/tree/git-submodule It seems to work well, and no new dependencies are introduced.

raveit65 commented 4 years ago

Can someone other please review https://github.com/mate-desktop/mate-terminal/pull/344 This is needed to disable SMClient support here for building with wayland.

raveit65 commented 4 years ago

Maybe we can drop eggdesktopfile.c and use GDesktopAppInfo.

$ find . -name eggdesktopfile.c
./mate-session-manager/egg/eggdesktopfile.c
./engrampa/copy-n-paste/eggdesktopfile.c
./mate-terminal/src/eggdesktopfile.c
./caja/libegg/eggdesktopfile.c
./atril/cut-n-paste/smclient/eggdesktopfile.c
./mate-utils/gsearchtool/libeggsmclient/eggdesktopfile.c
./pluma/pluma/smclient/eggdesktopfile.c
./mate-panel/mate-panel/libegg/eggdesktopfile.c

I think in other repos like atril, eom, pluma, etc. SMClient support is also needed displaying last opened files in menus, or not? So finding a replacement which can be used for wayland too might be a good idea?

Edit: seems like GDesktopAppInfo doesn't support this function :/ Still wondering how this is handled in other wayland desktops?

rbuj commented 4 years ago

Maybe we can drop eggdesktopfile.c and use GDesktopAppInfo.

$ find . -name eggdesktopfile.c
./mate-session-manager/egg/eggdesktopfile.c
./engrampa/copy-n-paste/eggdesktopfile.c
./mate-terminal/src/eggdesktopfile.c
./caja/libegg/eggdesktopfile.c
./atril/cut-n-paste/smclient/eggdesktopfile.c
./mate-utils/gsearchtool/libeggsmclient/eggdesktopfile.c
./pluma/pluma/smclient/eggdesktopfile.c
./mate-panel/mate-panel/libegg/eggdesktopfile.c

I think in other repos like atril, eom, pluma, etc. SMClient support is also needed displaying last opened files in menus, or not? So finding a replacement which can be used for wayland too might be a good idea?

We can use GtkRecentManager like evince does.

raveit65 commented 4 years ago

Can someone other please review #344 This is needed to disable SMClient support here for building with wayland.

Ok, building with --disable-smclient has the disadvantage that we lost session-state support for mate-terminal. Can this part of libegg work under wayland? Or was this the reason that smclient support was dropped from gnome-terminal?

raveit65 commented 4 years ago

@yetist Can you please rebase to fix merge conflict after merging https://github.com/mate-desktop/mate-terminal/pull/344

raveit65 commented 4 years ago

Indeed, last patch fixes this warning.

[rave@f32 ~]$ mate-terminal
Unable to init server: Could not connect: Connection refused

So this seems OK. This build is with smclient support and it doesn't hurt in wayfire. Any concerns left or can this be merged? @yetist Can you please squash all commits?

raveit65 commented 4 years ago

I think the problem is here with gtk_menu_popup_at_pointer https://github.com/mate-desktop/mate-terminal/blob/master/src/terminal-window.c#L1563

raveit65 commented 4 years ago

Reverting partial https://github.com/mate-desktop/mate-terminal/pull/250 fixes the issue with context-menu, but we have deprecated code again. As a side note, gnome-terminal use deprecated gtk_menu_popup at this place. https://www.dropbox.com/s/kmx9ge1gexfj8en/mate-terminal_0001-restore-deprecated-gtk_menu_popup-for-context-menu.patch?dl=0

From e7152632ea2e4b5cf8b9ef729efa6572b590b452 Mon Sep 17 00:00:00 2001
From: raveit65 <mate@raveit.de>
Date: Tue, 2 Jun 2020 22:22:00 +0200
Subject: [PATCH] restore deprecated gtk_menu_popup for context-menu

fixing weird behaviour with wayland compositor
---
 src/terminal-window.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/src/terminal-window.c b/src/terminal-window.c
index b50378f..66af860 100644
--- a/src/terminal-window.c
+++ b/src/terminal-window.c
@@ -1477,9 +1477,6 @@ popup_clipboard_targets_received_cb (GtkClipboard *clipboard,
     GtkAction *action;
     gboolean can_paste, can_paste_uris, show_link, show_email_link, show_call_link, show_input_method_menu;
     int n_pages;
-    GdkEvent *event;
-    GdkSeat *seat;
-    GdkDevice *device;

     if (!gtk_widget_get_realized (GTK_WIDGET (screen)))
     {
@@ -1552,17 +1549,11 @@ popup_clipboard_targets_received_cb (GtkClipboard *clipboard,
     if (!gtk_menu_get_attach_widget (GTK_MENU (popup_menu)))
         gtk_menu_attach_to_widget (GTK_MENU (popup_menu),GTK_WIDGET (screen),NULL);

-    event = gtk_get_current_event ();
-
-    seat = gdk_display_get_default_seat (gdk_display_get_default());
-
-    device = gdk_seat_get_pointer (seat);
-
-    gdk_event_set_device (event, device);
-
-    gtk_menu_popup_at_pointer (GTK_MENU (popup_menu), (const GdkEvent*) event);
-
-    gdk_event_free (event);
+    gtk_menu_popup (GTK_MENU (popup_menu),
+                    NULL, NULL,
+                    NULL, NULL,
+                    info->button,
+                    info->timestamp);
 }

 static void
-- 
2.26.2
raveit65 commented 4 years ago

But maybe someone has a better solution than me, without adding a deprecation again :)

raveit65 commented 4 years ago

Any idea what we should do with the context menu?

raveit65 commented 4 years ago

@yetist Should i add my patch https://github.com/mate-desktop/mate-terminal/pull/342#issuecomment-637801534 to PR or do you have a better idea to fix broken context menu? Than we can merge the PR.

yetist commented 4 years ago

@yetist Should i add my patch #342 (comment) to PR or do you have a better idea to fix broken context menu? Than we can merge the PR.

I don't have a better idea, so I think you can apply your patch to solve the problem.

raveit65 commented 4 years ago

Done. Any one like to test/review it again, before merging?

raveit65 commented 4 years ago

@rbuj Your review is still open, any concerns left?

raveit65 commented 4 years ago

Ready to go?