Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support ya sub subcommand for the Ya CLI #1004

Merged
merged 9 commits into from
Jun 16, 2024

Conversation

mikavilpas
Copy link
Contributor

The motivations for this change are:

  • easy debugging of DDS messages for yazi and plugin developers
  • allow external applications to monitor yazi events.
ya-sub-static-demo.mov

In neovim specifically, there is a limitation that the stdout and stderr streams cannot be monitored when displaying an embedded terminal application. A second yazi instance could theoretically be started, but the ui does currently not work when there is no screen to draw on.

@sxyazi
Copy link
Owner

sxyazi commented May 4, 2024

Ideally, I don't want to add new ya sub / ya sub-static because it overlaps with the functionality of yazi --remote-events, which means maintaining an additional set of code with identical functionality, something I'd prefer to avoid if possible.

Can you describe the issues you encountered when reading the output of yazi --remote-events in Neovim, ex what was the code like? I'll try it later to see if I can make it work.

@mikavilpas
Copy link
Contributor Author

The error I get is the following:

   Info  18:42:06 notify.info stderr: "\27[?1004l\27[?2004l\27[?1049l\27[0 q\27[?25hError: Device not configured (os error 6)\n"
   Info  18:42:06 notify.info stdout: nil

I realize it's not much to work with. Here's a code snippet that causes that error for me:

    local events_job = vim.system({ 'yazi', "--remote-events='hover'" }, {
      text = true,
      stderr = function(_, data)
        vim.notify('stderr: ' .. vim.inspect(data))
      end,
      stdout = function(_, data)
        vim.notify('stdout: ' .. vim.inspect(data))
      end,
    })

The error does not occur if I start lazygit instead (just as a test).

yazi-boot/src/boot.rs Outdated Show resolved Hide resolved
@mikavilpas
Copy link
Contributor Author

Just rebased to main and resolved all conflicts. No change in functionality was made.

yazi-cli/src/args.rs Outdated Show resolved Hide resolved
yazi-dds/src/client.rs Outdated Show resolved Hide resolved
@mikavilpas
Copy link
Contributor Author

The suggestions have now all been addressed. However, some odd issues now remain that I would like to see fixed:

  • if no server is running, the command fails (I actually don't mind this myself)

  • when starting, some old events are reported every time on startup:

    hover,0,200,{"tab":0,"url":"/Users/mikavilpas/git/yazi/yazi-dds/src/pump.rs"}
    hover,0,200,{"tab":0,"url":"/Users/mikavilpas/git/yazi/yazi-dds/src/pubsub.rs"}
    hover,0,200,{"tab":0,"url":"/var/folders/23/y7gf7hz129l56_83w_g49jl80000gn/T"}
    hover,0,200,{"tab":0,"url":"/var/folders/23/y7gf7hz129l56_83w_g49jl80000gn/T/yazi"}
    cd,0,100,{"tab":0,"url":"/Users/mikavilpas/git/yazi/yazi-cli/src"}
    hover,0,200,{"tab":0,"url":"/var/folders/23/y7gf7hz129l56_83w_g49jl80000gn/T/.ts-nodecDOP5F"}
    hover,0,200,{"tab":0,"url":"/Users/mikavilpas/git/yazi.nvim/.nvmrc"}
    hover,0,200,{"tab":0,"url":"/Users/mikavilpas/git/yazi/yazi-dds/src/payload.rs"}
    hover,0,200,{"tab":2,"url":"/var/folders/23/y7gf7hz129l56_83w_g49jl80000gn/T/651E3D9C-9D65-4176-AC6E-BEF001DC35BF"}
    hover,0,200,{"tab":0,"url":"/Users/mikavilpas/git/yazi/yazi-cli/src/main.rs"}
    hover,0,200,{"tab":0,"url":"/Users/mikavilpas/git/yazi/yazi-dds/src/payload.rs"}
    hover,0,200,{"tab":0,"url":"/Users/mikavilpas/git/yazi/yazi-dds/src/pump.rs"}
    hey,0,1715359754665986,{"peers":{"1715359756825029":{"abilities":["hover","cd","hey","hi"]},"1715359754665986":{"abilities":["dds-cd"]}}}

    These seem to be from very old sessions and I'm not sure why they are still being reported.

  • when yazi closes and reopens, no events are ever reported again. I think this makes sense as for the new yazi instance, the handshake has not been done. However, I would like to be able to reestablish the connection. Do you have advice how this could be done?

yazi-cli/Cargo.toml Outdated Show resolved Hide resolved
@sxyazi
Copy link
Owner

sxyazi commented May 10, 2024

if no server is running, the command fails (I actually don't mind this myself)

Yeah this is the expected behavior.

when starting, some old events are reported every time on startup:

Try rm ~/.local/state/yazi/.dds and see if it still. This may be related to the dirty data tested before.

when yazi closes and reopens, no events are ever reported again. I think this makes sense as for the new yazi instance, the handshake has not been done. However, I would like to be able to reestablish the connection. Do you have advice how this could be done?

When next_line() returns None, it means that the server has stopped. At this time, you can call Stream::connect() to try to re-establish the connection (if other servers are started), but you need to add time::sleep() for each attempt to avoid CPU consumption caused by fast failure

@mikavilpas
Copy link
Contributor Author

When next_line() returns None, it means that the server has stopped. At this time, you can call Stream::connect() to try to re-establish the connection (if other servers are started), but you need to add time::sleep() for each attempt to avoid CPU consumption caused by fast failure

Thanks for the instructions! I was able to implement reconnecting, and it's much better now.

However, there is some odd behaviour when starting up: about 2000 empty messages are received.

As a proof of concept, I added a hacky counter to display the number of the current message. Here's a very short demo:

yazi-messages.mov

I'm not sure what this might mean. Could you try this out and see if you get the same behaviour?

@sxyazi
Copy link
Owner

sxyazi commented May 20, 2024

Why is this PR closed?

@mikavilpas
Copy link
Contributor Author

There are two reasons:

  1. it's been open for a long time and I'm starting to forget little details and
    ideas
  2. I'm no longer convinced it will be a good idea. In my use case with neovim,
    this would enable real time event reading, and thus writing a few minor
    features. But what I have in mind for the neovim integration (which I wrote
    about in Increase possibilities for yazi<->embedding editor integration #989) would require a deeper
    level of integration, such as RPC.

I could maybe see such an integration layer being implemented in lua, perhaps
also as a yazi plugin, but right now I consider this "lua route" blocked by
(although all of these are totally solvable issues!)

  • lack of typing for yazi apis (difficult to write lua bindings without them)
  • lack of testing facilities in yazi plugins (hard to write features and
    maintain them)
  • lack of versioning for yazi apis (upcoming breaking changes in yazi could
    break plugins without a clear way to detect this)
  • uncertainty about what your vision for yazi is regarding these things (hard to
    justify the effort)

As you explained in
#1057 (comment), I think in
general yazi is still taking shape. It's probably best to wait and simply
reimplement it later, or do something completely new.

@mikavilpas mikavilpas reopened this May 30, 2024
yazi-dds/src/client.rs Outdated Show resolved Hide resolved
The motivations for this change are:
- easy debugging of DDS messages for yazi and plugin developers
- allow external applications to monitor yazi events.

In neovim specifically, there is a limitation that the stdout and stderr
streams cannot be monitored when displaying an embedded terminal
application. A second yazi instance could theoretically be started, but
the ui does currently not work when there is no screen to draw on.
- don't start a new server. Instead, crash if the server is not running.
- remove support for listening for a specific peer. The protocol doesn't
  support it and it cannot be made to work.
- if the existing yazi closes, this cannot be detected, and the
  connection is never reestablished. Maybe this can be made to work in a
  future commit.
If the connection cannot be made when starting, the client will fail and
not reconnect. However, if the connection is lost after the client has
started, it will forever attempt to reconnect with a 1s delay.

Also clean up the implementation by removing previous mistakes
yazi-cli/src/main.rs Outdated Show resolved Hide resolved
yazi-dds/src/client.rs Outdated Show resolved Hide resolved
yazi-dds/src/client.rs Outdated Show resolved Hide resolved
Copy link
Owner

@sxyazi sxyazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now, thanks for contributing this feature!

@sxyazi sxyazi changed the title feat(cli): allow printing selected DDS messages to the console feat: support ya sub subcommand for the Ya CLI Jun 16, 2024
@sxyazi sxyazi merged commit 2a35d30 into sxyazi:main Jun 16, 2024
6 checks passed
@mikavilpas mikavilpas deleted the add-ya-sub-command branch June 16, 2024 07:53
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants