Add a `focus` and a `tag` command for remote interaction through the FIFO #35

lenormf · 2016-10-02T04:27:37Z

Hi,

I've added support for dvtm to my editor, and I needed two more commands to match my workflow with tmux before I switched over to dvtm. Those two commands are focus and tag, both implemented duplicating/tweaking already existing code.

There's an additional commit that improves the readability of a function by using an if statement instead of a switch, but it's just cosmetics so feel free to drop it if you don't like it.

HTH.

This commit adds a `focus` command to the list of supported commands available through the external FIFO, which switches focus to the window whose id is equal to the one passed as parameter.

Introduce a `tag` command that allows assigning a tag to a visible window through the commands FIFO. This commit also fixes the `bitoftag` function which compared pointers instead of the actual characters of the strings containing the tags.

martanne · 2016-10-02T11:15:23Z

Thanks! Improving the scripting ability of dvtm is something I would like to do. Eventually my vis editor should also use a client/server design similar in spirit to Kakoune. Are your integration scripts online somewhere?

Just a few remarks from a quick glance at the diff:

your changes to bitoftag are probably not entirely correct. The reason it did a pointer comparison before was because the function was only ever called with arguments from the tags[] array. This is no longer true with your external command. But now you are only comparing the first character of the tag. You probably want to use strcmp there to handle tags longer than 1 character.
why do you restrict your commands to visible clients? For example, should it not also be possible to focus a currently not visible client (and automatically apply the current tag set to it)?
I'm not sure whether we should introduce a dedicated tool dvtm-cmd or something which abstracts the communication so that we could still change it later on. In any case we should document the available commands. (Yes the create command lacks documentation too. That is mostly because the scripting interface was so far considered "experimental").

lenormf · 2016-10-02T11:49:10Z

You can find my WIP dvtm compatibility script here. It follows the same features provided by the official tmux script (plus tagging), and allows to interact with dvtm from within kakoune (e.g. to open new buffers in a new window automatically).

But now you are only comparing the first character of the tag.

Since right now there are only one byte long tags (as far as I know?), I went for the least expensive check and decided not to use strcmp.

why do you restrict your commands to visible clients?

No reason, I just did a lazy copy/paste of the loop in the focusn function :) I'll correct that.

I'm not sure whether we should introduce a dedicated tool dvtm-cmd or something which abstracts the communication so that we could still change it later on. In any case we should document the available commands

A dedicated tool to interact with a session would be nice indeed (dvtm-cmd -c $FIFO cmd [ args…] maybe ?). How should the function be documented, a comment in the config.def.h file, or a section in a dedicated file, or both ?

martanne · 2016-10-02T12:42:27Z

Since right now there are only one byte long tags (as far as I know?)

In the default configuration yes. However I know that some people use longer tag names. We should keep that working.

How should the function be documented?

Maybe a short single line comment in config.def.h and proper user facing documentation in new manual page dvtm-cmd.1 would be my preferred way.

Since tags can be set to strings longer than the default one byte long digits, we need to compare the whole strings and not only the first character. Assigning a tag should not depend on the visibility of a window, so we loop over all the clients (as opposed to only the visible ones).

lenormf · 2016-10-02T14:11:24Z

Commits added that take into account the remarks above.

I have also made a PoC version of a dvtm-cmd utility, is that what you had in mind ? I can throw this script in the PR and create a man page for it if that would help.

martanne · 2016-10-02T19:11:35Z

Your changes and the dvtm-cmd script look fine, thanks! A few more comments:

Tags can be arbitrary strings, we should not restrict them to numeric values by calling atoi in tagid.
There probably should be a way to assign multiple tags to a window (e.g. tag <win-id> <tags...> i.e. tag 1 foo bar). It might also be useful to be able to add/remove certain tags tag 1 +foo -bar or similar.
More generally it might make sense to use a Unix domain socket instead of a named pipe for communication purposes. This would allow bidirectional communication i.e. query commands. The dvtm-cmd script would then use socat(1).
With my previous comment about focusing non visible windows I meant that focusid should probably contain something along the lines of:

if (!isvisible(c))
    c->tags |= tagset[seltags];

Also a man page for dvtm-cmd would be fantastic!

This commit updates the documentation of the `create`, `focus` and `tag` commands in the default configuration file; the `tagid` function no longer expects tags to be integers and only appends tags instead of replacing them; the `focusid` function allows focusing non-visible windows.

THe `untagid` function the implements the `untag` command removes the given tags from the ones assigned to a window.

Introduce the `dvtm-cmd` wrapper that allows interacting with `dvtm` through its command FIFO easily. A man page that documents its features was also added, and they are both installed by default.

lenormf · 2016-10-05T07:46:19Z

The remarks above have been taken into account.

I haven't implemented the tag 1 +foo -bar case because I decided to stick to primitives on dvtm's side for the time being, and eventually implement that in dvtm-cmd. The fact that commands are limtied to MAX_ARGS - 1 arguments is a bit annoying at times, maybe this number should be bumped up?

Implementing bi-directional communications would be a major overhaul (we would have to get the return messages back from the command functions to the select call), also I don't like the idea of forcing a dependency (socat) on users who want to use the commands FIFO. Maybe this could be done in a later time, when there's really a need for this feature?

dvtm-cmd and its man page added as well.

martanne · 2016-10-09T20:48:26Z

Hi, I committed the first part of your changes. Thanks!

With your original code it was possible to "lose" windows by removing all tags. I think this is rather confusing, we now check that each window is assigned to at least one tag. Furthermore unknown tags are now properly ignored. I also removed the untag command, instead tags can be added by prefixing them with +, similarly - removes the following tag. Specifiying neither plus or minus will replace the current tag set. This approach also has the advantage that it is atomic i.e. no spurious redraws are performed. You will have to adjust the dvtm-cmd script.

MAX_ARGS has been increased to 8.

I agree supporting bidirectional communication would require a major overhaul, but it might be useful to support "query commands". At the moment there exists no feedback whatsoever whether the command succeeded etc. Also depending on socat (or a similar tool allowing to pipe data into a unix domain socket) doesn't seem that bad to me.

As for the manual page, it seems like you used Docbook as source format? I would prefer a handwritten man page in mdoc format. I can take care of the conversion, if needed.

Thanks again!

lenormf · 2016-10-10T07:55:00Z

Thanks for integrating the changes, they will be helpful.

I use a2x from asciidoc to generate the manpage, why do you want the pages to be specifically in the mdoc format? Compatibility?

I'll go over the commits made and rebase this branch on master, to get rid of the commits that were merged in and those that were discarded.

lenormf added 3 commits September 30, 2016 19:45

Make the handle_cmdfifo function more readable

9c3192a

Allow focusing windows through the command FIFO

5870e0b

This commit adds a `focus` command to the list of supported commands available through the external FIFO, which switches focus to the window whose id is equal to the one passed as parameter.

lenormf added 2 commits October 2, 2016 16:17

Add a short documentation of the FIFO commands

785017b

ghost mentioned this pull request Oct 2, 2016

Configuration file #34

Open

lenormf added 3 commits October 5, 2016 10:34

Add an untag command available through the command FIFO

6a748d8

THe `untagid` function the implements the `untag` command removes the given tags from the ones assigned to a window.

Add a remote interaction utility script and its man page

131529f

Introduce the `dvtm-cmd` wrapper that allows interacting with `dvtm` through its command FIFO easily. A man page that documents its features was also added, and they are both installed by default.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a `focus` and a `tag` command for remote interaction through the FIFO #35

Add a `focus` and a `tag` command for remote interaction through the FIFO #35

lenormf commented Oct 2, 2016

martanne commented Oct 2, 2016

lenormf commented Oct 2, 2016 •

edited

Loading

martanne commented Oct 2, 2016

lenormf commented Oct 2, 2016

martanne commented Oct 2, 2016 •

edited

Loading

lenormf commented Oct 5, 2016

martanne commented Oct 9, 2016

lenormf commented Oct 10, 2016

Add a focus and a tag command for remote interaction through the FIFO #35

Are you sure you want to change the base?

Add a focus and a tag command for remote interaction through the FIFO #35

Conversation

lenormf commented Oct 2, 2016

martanne commented Oct 2, 2016

lenormf commented Oct 2, 2016 • edited Loading

martanne commented Oct 2, 2016

lenormf commented Oct 2, 2016

martanne commented Oct 2, 2016 • edited Loading

lenormf commented Oct 5, 2016

martanne commented Oct 9, 2016

lenormf commented Oct 10, 2016

Add a `focus` and a `tag` command for remote interaction through the FIFO #35

Add a `focus` and a `tag` command for remote interaction through the FIFO #35

lenormf commented Oct 2, 2016 •

edited

Loading

martanne commented Oct 2, 2016 •

edited

Loading