tilda.c: wayland dbus visibility toggler

[kkrolczyk]

in order to toggle visibility in Wayland, commands have to be sent via dbus.
This commit introduces command line switch & function to call dbus method.
It changes the order of tilda startup, as a window is not needed,
if we intend to exit right after sending cmd.

Lastly, there are a couple of formatting fixes, and readme entry.

In order to use it:

• run Tilda with --dbus
• set your preferred shortcut in OS with Wayland to run tilda -T 1

Signed-off-by: Krzysztof Królczyk Krzysztof.Krolczyk@o2.pl

[kkrolczyk]

This is related to #487

[deltreey]

tested on ubuntu 23.10. This doesn't work.

[kkrolczyk]

Hmm, that's from a year ago. I'll see if i have time next week to check it, but the project looks abandoned, so I'm not sure if it's worth to focus on that.

[lanoxx]

Hello, thanks for this contribution. I merged a few other open pull requests and I will try to find some time in the next weeks to review this as well and get it merged. If you could double check and confirm if it works on 23.10 that would be great. I am still on Ubuntu 22.04.

I will try and aim for a new release before February, so that we can get the next release into Ubuntu 24.04.

[lanoxx]

Thanks for your replies on my comments. Somehow I don't see the changes on your branch. Can you double check if you correctly pushed your additional commits after you made the update and make sure they show up here in this pull request?

[lanoxx]

@kkrolczyk I still don't see the updated code, any chance you could push it and post the link to the updated commits here? Then I will take another look and merge them.

[kkrolczyk]

Sorry for the delay @lanoxx - I've updated the branch & tested on 23.10 with Wayland

[lanoxx]

I have made the following additional changes to your commit:

1. Moving the two TILDA_DBUS_ACTIONS_* macros breaks encapsulation of the tilda-dbus-actions unit. Instead we should provide a new method for the toggle action there, which handles the D-Bus call. This way, we can keep these strings hidden in the implementation and the client just needs to call something like tilda_dbus_actions_toggle (gint instance_id), with the implementation of just_force_dbus_toggle_for_wayland so that this method is no longer needed inside tilda.c. I introduced this method and kept the macros in the C file.
2. Most users will only be running one tilda process and thus the value for the -T argument will be 0 most of the time. It will be convenient to have the argument optional (i.e., -T instead of -T 0). Unfortunately, GOption only supports optional values via a callback method, so I changed the implementation to use such a callback. If -T is given without a value, we default to 0. In addition this adds a bit of error handling and groups the options under a new Tilda D-Bus Options category (see tilda --help-all). I also moved the --dbus option to this option group:

D-Bus Options:
  --dbus                     Enable D-Bus interface for this instance
  -T, --toggle-window        Toggle N-th instance Window visibility and exit

Long story short, this allows calls like tilda -T to be setup from the shortcut menu.

See commit: 9fbc5dc1cddc0c5c535e2c2542066c2d67f2776d.

I will probably merge this tomorrow or Friday as I just finished it and want to read over it one more time to catch any typos.

[kkrolczyk]

Hey, thanks, changes seem reasonable; I've tested them, they still work.

1. Though I'm not sure, if for simple toggling would it make sense to load config first,
   If user adds a shortcut tilda -T, each time it would try to open config_N+1.
   Also, in order to have reasonably well user experience, i'm mapping the global shortcut to the same button
   which Tilda uses to pull window up, while this works, i wonder if this is not racy.
   If Tilda is visible and has focus, keygrabber calls pull_up, then also OS runs new Tilda, to do toggle.
   If Tilda is visible but doesn't have focus - to hide it, user has to press shortcut key twice (once to get focus, next to hide).
   Maybe this is the reason, a year ago it passed "force" through D-BUS

I've spotted a tiny typo

To enable this, users needs to start Tilda

should be

To enable this, users need to start Tilda

but i guess this can be fixed together with some code style/formatting commit, if needed.

2. I wanted to refresh translations too, and while doing this, i've stumbled upon a couple of strings in tilda-match-registry.c
   which appeared not to be marked for translation. I've pushed the branch "kk/fix_translations" to my fork -
   if you think it makes sense, I can make it a PR.

3. I've wondered, would it help you to create separate PR with Github actions, so the repo could benefit from doing future releases in cloud as well (kk/simple-gh-action).

4. Lastly, while

• sessions PR (feature: support sessions #496) was rather PoC and shouldn't be merged as is,
• search-in-browser (PR feature: search selected in search engine from ctx menu #497) seems small and harmless (though i'd replace fprintf with g_printerr, and create an input field in tilda.ui to allow user to configure search engine uri, instead of requiring to manually edit config file).

[lanoxx]

1. Though I'm not sure, if for simple toggling would it make sense to load config first,
   If user adds a shortcut `tilda -T`, each time it would try to open config_N+1.

You are right, this is unnecessary, I reordered the initialization a little to make the D-Bus call before the lock is acquired and the config is opened. What do you think?

   Also, in order to have reasonably well user experience, i'm mapping the global shortcut to the same button
   which Tilda uses to pull window up, while this works, i wonder if this is not racy.

Yes, it would be better to make the keybinding optional so that it can be cleared when we are on Wayland. I had started to work on this a while a go but then did not merge it since there were still a few missing pieces. Basically, we want be able to clear the pull-down hotkey in the preferences, but then the tilda-keybinding validation needs to accept "NULL" as a valid value. However, I think we want to do this only when D-Bus is enabled and since we are not storing the --dbus argument in the config, there was no simple way to determine if D-Bus is active while we are in the tilda-keybinding validation. Maybe we should store a flag in the tilda_window struct so that we know at runtime if the D-Bus interface is running, then we can query that in the wizard.

Anyway, I cherry-picked my original commit that I had worked on and pushed it to our new branch. It needs more work, so I will probably not merge it right away, but you can take a look it you want.

   If Tilda is visible and has focus, keygrabber calls pull_up, then also OS runs new Tilda, to do toggle.
   If Tilda is visible but doesn't have focus - to hide it, user has to press shortcut key twice (once to get focus, next to hide).
   Maybe this is the reason, a year ago it passed "force" through D-BUS

Well, in this case you could just set the "Non-Focus Pull Up Behaviour" option to "Hide Terminal" or am I missing something?

I've spotted a tiny typo

Fixed, thanks for spotting this.

If you think the reordering of the initialization steps that I mentioned above are fine, then I would merge that first, and we can follow-up with another change that makes the pull toggle optional.

[lanoxx]

I cleaned up that commit from yesterday and added a solution that I think will work reasonable well. I also added a new desktop file, so that tilda can be started with D-Bus support directly from the application menu.

[kkrolczyk]

Hey, I've just checked - seems desktop file breaks the build on 23.10:

mkdir build && cd build && ../autogen.sh --enable-maintainer-flags && make -j2

sed -e 's|\@BINDIR\@|/usr/local/bin|' \
	-e 's|\@PIXMAPSDIR\@|/usr/local/share/pixmaps|' ../tilda.desktop.in > tilda.desktop
  GEN      data/man/tilda.1
make[2]: *** No rule to make target 'tilda-dbus.desktop', needed by 'all-am'.  Stop.

i guess small change in Makefile.am fixes this

index 9d5c57b..b0b41eb 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -47,11 +47,11 @@ Pixmaps_DATA = tilda.png
 
 EXTRA_DIST = tilda.desktop.in tilda.png tilda.appdata.xml README.md
 
-tilda.desktop: tilda.desktop.in
+%.desktop: %.desktop.in
+
        sed -e 's|\@BINDIR\@|$(bindir)|' \
                -e 's|\@PIXMAPSDIR\@|$(Pixmapsdir)|' $< > $@
-               
-               
+
 ACLOCAL_AMFLAGS = -I m4 ${ACLOCAL_FLAGS}

I guess it might be also worth a while to add to a readme, that one has to change the key shortcut in Tilda first, before setting up the global handler, otherwise you won't be able to assign/change the key inside Tilda's config (well, other than manually editing config file, though).

You were right about "Non-Focus Pull Up Behaviour" option to "Hide Terminal" - I wonder if this could be somehow phrased better, is it not confusing? But then again, it might be just me, given that no one complained ;) Then, even if, i can't really think of any better phrasing, maybe except somewhat verbose "Action to take, on receiving pull up command when Tilda window has no focus". Maybe it's better to keep the old "well-known" one.

there was no simple way to determine if D-Bus is active while we are in the tilda-keybinding validation.

Right, to be honest passing keybinding validation to a functions seems somewhat unwieldy for me, but alternative is also poor - one could simply do a D-BUS query, to check if bus name exists. User might also be confused, "why is this not working?!" if they forget to start Tilda with --dbus and then try -T switch, which will simply do nothing. I don't know if this could be helped though (reduces to the same problem, to know if Tilda was running with dbus or not; again "solvable" by introducing another D-BUS query, but perhaps the cost is not worth it).

[lanoxx]

Many thanks for the merge request and all the feedback. I have just merged it. There are certainly more improvements that could be made but I think this should be good enough to merge now.

According to the Ubuntu Wiki the Debian import freeze for Ubuntu 24.04 starts on Feb. 29:
February 29, Debian Import Freeze

I will try to make a new upload to Debian before that so these changes and several other bug fixes can make it into the next Ubuntu release.

[kkrolczyk]

Happy to hear! I guess we can close this PR then