lanoxx / tilda

A Gtk based drop down terminal for Linux and Unix
GNU General Public License v2.0
1.27k stars 162 forks source link

Add a D-Bus implementation for Tilda (draft) #396

Closed ghost closed 4 years ago

ghost commented 5 years ago

This is not a pull request to be merged, just a quick hack I developed for myself, others might find it useful.

The issue

On Fedora 30, tilda won't open any more, if the focus is on a native gnome app or no app is running (focus is on empty desktop). For instance when the focus is on a nautilus window, the F1 (or whatever the shortcut is) won't pull down tilda. But when the focus is on Firefox, F1 works and pulls down tilda.

How it works

In tilda.c, a callback is registered for a method invocation Hello on path io.koosha.Toggle and pulls down the tilda. There is an app called opener.c in tilda_toggler, when opener is called, it invokes the Hello method, which in turn pulls down tilda

How it should be

In a proper namespace unique to tilda, instead of method invocation, tilda must listen for incoming signals: open signal, close signal, toggle, ....

Anticipated issues

I'm counting on tilda to grab the shortcut key before it reaches gnome custom shortcut handler. If it doesn't the key will bounce back and open tilda again.

How to use

Then:

lanoxx commented 4 years ago

Thanks for this pull request. Based on this I have created branch wip-dbus and started to experiment with this. Its not ready to merge, but it works and it supports multiple tilda processes each with their own dbus name. This makes it possible to control multiple instances of tilda.

taoqf commented 4 years ago

Debian buster gnome when file manager or system monitor is open, or even no window is opened, host key f1 won't call tilda.

lanoxx commented 4 years ago

@taoqf I assume you are on a wayland session. In this case, you can try to build wip-dbus and toggle the terminal with a dbus command.

taoqf commented 4 years ago

Thanks, I will say I would wait for wip-dbus be published

tuxxi commented 4 years ago

Hello,

Thank you for making this PR! I am having some issues with the wip-dbus branch. I built and ran it correctly, but I don't see the dbus service being registered on my system.

$ qdbus com.github.lannox.tilda.Actions
Service 'com.github.lannox.tilda.Actions' does not exist.

I also tried:

$ dbus-send --session --dest=org.freedesktop.DBus --type=method_call --print-reply /org/freedesktop/DBus org.freedesktop.DBus.ListNames | grep tilda

but to no avail.

Additionally, even if I could see the service, I'm not entirely sure what message to send. Something like this?

$ dbus-send --dest=com.github.lannox.tilda.Actions \
--type=method_call \
com.github.lannox.tilda.Actions.Toggle

Thank you for your help!

lanoxx commented 4 years ago

The dbus support is opt-in, so you need to pass --dbus to tilda at start. Then it should expose an interface unter com.github.lannox.tilda.Actions + <instanceNumber> so for example com.github.lannox.tilda.Actions1.

Try:

dbus-send --session --print-reply --dest=com.github.lanoxx.tilda.Actions1 /com/github/lanoxx/tilda/Actions1 com.github.lanoxx.tilda.Actions1.Toggle

Look into the terminal, tilda will print the DBus interface name, otherwise use d-feet and search for tilda then you should be able to determine that DBus interface.

lanoxx commented 4 years ago

I am currently considering whether to merge this for tilda 1.5 which I plan to release in the next few weeks. So some feedback on this would be useful.

tuxxi commented 4 years ago

Thanks!

I'm new to dbus stuff, so I am confused as to why the command doesn't appear to work if --print-reply is omitted. I was able to get a global keyboard shortcut working to send the dbus command. However, if the shortcut is the same as the native x11 shortcut key, tilda will crash with a SIGTRAP and the following message:

(tilda:25409): tilda-ERROR **: 14:57:25.923: X Error: BadMatch (invalid parameter attributes)

A workaround is to just set the native show/hide shortcut to something else.

amo13 commented 4 years ago

Hey @lanoxx , I came here a bit late for the 1.5 release but I would actually be very happy about the inclusion of the experimental d-bus implementation into the main tilda release! I tried to build the wip-dbus branch but the compilation fails because at some point tilda-dbus.c it is not finding config.h when trying to include it.

lanoxx commented 4 years ago

@amo13 It looks like I had left over config.h in my build folder (tilda/build/src/config.h), which was not supposed to be there.

You can try to run touch build/src/config.h or src/config.h depending on how you setup your build.

I usually run VPATH builds. Something like this:

cd tilda
mkdir build
cd build
../autogen.sh --enable-maintainer-flags  # you can omit the option but its useful for debugging.
make

Hope that helps.

amo13 commented 4 years ago

awesome, thank you! I was able to compile it with this simple trick :)

unfortunately, now the dbus interface complains about an unknown service:

Error org.freedesktop.DBus.Error.ServiceUnknown: The name com.github.lanoxx.tilda.Actions1 was not provided by any .service files

Edit: This is the response to the dbus command you indicated above:

dbus-send --session --print-reply --dest=com.github.lanoxx.tilda.Actions1 /com/github/lanoxx/tilda/Actions1 com.github.lanoxx.tilda.Actions1.Toggle

But I did start tilda with the --dbus flag

amo13 commented 4 years ago

Oh no, I am sorry... I need to open my sleepy eyes! Tilda clearly says:

(tilda:168390): VTE-CRITICAL **: 17:19:43.096: void vte_terminal_match_set_cursor_type(VteTerminal*, int, GdkCursorType): assertion 'tag >= 0' failed
Activating D-Bus interface on bus name: com.github.lanoxx.tilda.Actions0
Tilda has started. Press <Shift><Alt>t to pull down the window.

Using the correct instance number works

lanoxx commented 4 years ago

Glad that it works.

Here are some thoughts from my side on this. I am not really happy with the implementation at the moment and so I have not merged it into the 1.5 release.

The reason is mostly that the tilda-dbus.{c,h} files have been generated from an XML file via gdbus-codegen. But I had to modify it to support that each Tilda instance can run its own DBus server. This is clearly not ideal. If we want to add additional DBus interfaces we will need to regenerate that file and then the modifications would be lost and need to be merged back manually.

The question is if we really want to have each tilda process run its own DBus Server. Tilda currently supports running multiple processes, so each tilda process you start runs in its own process with exactly one window and own configuration file. I like this design and I do not really want to make tilda a single process app. But DBus is not designed that way. It assumes there is one app that runs as a singleton.

A possible design could be to have only the first tilda process support DBus, so that you can use the --dbus option only once. Additional tilda processes could still be started but would not have DBus enabled.

An alternative would be to modify the gdbus-codegen tool to support dynamic DBus interface names. But I am not so sure if that would be accepted.

amo13 commented 4 years ago

Accepted by whom?

I see the design problem and your point on this. From my point of view, I think that having only the first instance of tilda use the dbus interface is already a win big enough to include it into the release. Maybe with an toggle in the settings window to choose to opt in, so that wayland users can easily use tilda - even if not (yet) with its full powers. The user would still need to be instructed to bind the dbus-send call to tilda with a key combination in their desktop environment. For me it's GNOME and it is was pretty easy to accomplish.

lanoxx commented 4 years ago

Added an issue for GLib to find out what Glib developers think about modifying gdbus-codegen: https://gitlab.gnome.org/GNOME/glib/-/issues/2078

lanoxx commented 4 years ago

Regarding @tuxxi's comment:

I'm new to dbus stuff, so I am confused as to why the command doesn't appear to work if --print-reply is omitted.

This is explained by the dbus-send(1) man page about --print-reply:

Block for a reply to the message sent, and print any reply received in a
human-readable form. It also means the message type (--type=) is method_call.

So --print-reply implies --type=method_call. If you just add --type=method_call instead of --print-reply, then it also works.

@amo13 Turns out my problems with gdbus-codegen were based on some misconception about DBus from my side and this can actually be made to work without modifying gdbus-codegen.

amo13 commented 4 years ago

Cool, getting misconceptions out of the way is always good :) What does this mean in consequence?

lanoxx commented 4 years ago

@amo13 I have updated the wip-dbus branch. The D-Bus skeleton is now correctly build by gdbus-codegen and we support adding multiple Tilda processes each with a D-Bus interface. The tilda-config.h has been renamed to config.h so the build issue is gone. The feature toggle is still in place so you still need to add --dbus.

amo13 commented 4 years ago

Wow, that is awesome news! Thanks for looking into dbus. I think that makes tilda the first really working drop-down terminal on wayland! Any chance you can include the feature with the feature toggle into the main releases? This would even guarantee an easy update flow

amo13 commented 4 years ago

Something seems not yet right with the renamed config. Compilation now complains about a missing tilda-config.h

make[2]: Entering directory '/home/amo/tilda-dbus/src/tilda'
sed -e 's|\@BINDIR\@|/usr/bin|' \
    -e 's|\@PIXMAPSDIR\@|/usr/share/pixmaps|' tilda.desktop.in > tilda.desktop
  CC       src/tilda-glade-resources.o
  CC       src/tilda-tilda-enum-types.o
  CC       src/tilda-configsys.o
  CC       src/tilda-tilda-dbus.o
  CC       src/tilda-eggaccelerators.o
src/configsys.c:20:10: fatal error: tilda-config.h: No such file or directory
   20 | #include <tilda-config.h>
      |          ^~~~~~~~~~~~~~~~
compilation terminated.

Renaming config.h back to tilda-config.h of course works, but it would probably be nicer to have it compile without the need for user intervention.

amo13 commented 4 years ago

It looks like this branch has stopped working with the dbus command you mentioned above. Upon startup it states that it is

Activating D-Bus interface on bus name: com.github.lanoxx.tilda.Actions0

...but when I issue the command you gave above, I get the following error:

dbus-send --session --print-reply --dest=com.github.lanoxx.tilda.Actions0 /com/github/lanoxx/tilda/Actions0 com.github.lanoxx.tilda.Actions0.Toggle

Error org.freedesktop.DBus.Error.UnknownMethod: No such interface “com.github.lanoxx.tilda.Actions0” on object at path /com/github/lanoxx/tilda/Actions0
lanoxx commented 4 years ago

Two things:

I probably forgot to rename the include from tilda-config.h to config.h some of the .c files.

The DBus interface name now does not need to instance id anymore. So its just ...Actions (without 0). You can start d-feed and search for com.github.lanoxx. That should show how its registered in DBus.

amo13 commented 4 years ago

Awesome! You mean d-feet. So for whomever might be looking for this, the command I bind in GNOME keyboard bindings to F1 is dbus-send --session --print-reply --dest=com.github.lanoxx.tilda.Actions0 /com/github/lanoxx/tilda/Actions0 com.github.lanoxx.tilda.Actions.Toggle

lanoxx commented 4 years ago

Yes I meant d-feet :-)

I have update the branch again. Please let me know if you still have problems building it.

Btw. the exact interface details are also mentioned in the commit message of commit e48ddfe5cad66fc927cc4410230f6279c6c25c59.

I am now closing this merge request. For further discussion, please use: https://github.com/lanoxx/tilda/pull/407

lanoxx commented 4 years ago

Oh, and may thanks to @hkoosha for the initial work. I have mentioned you in the commit message (see https://github.com/lanoxx/tilda/commit/e48ddfe5cad66fc927cc4410230f6279c6c25c59).

shashwatsridhar commented 4 years ago

Hey! Sorry for commenting on a closed thread. I am not familiar with dbus nor do I have the necessary background to understand the details of this commit. I am using Wayland on Ubuntu 20.04 and found tilda to work on the Gnome session, but not on the Wayland session. Should that have been fixed with this commit? I am using version 1.5.2. Being a frequent user of tilda who made a recent shift to Wayland, I wanted to understand the status of the problem. Thanks!

amo13 commented 4 years ago

@shashwatsridhar you have to bind a d-bus terminal command to a key with gnome key bindings (keyboard settings I think). Read my last message here.

amo13 commented 4 years ago

@shashwatsridhar oh sorry and you may have to compile tilda with d-bus implementation. It is the branch of this pull request. It does not look like this has been merged into the main/release branch (yet?)...