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

stop using tmpdir() due to runtime inconsistencies #21

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fidgetingbits
Copy link

This is a fix for talonhub/community#966, which makes it possible to use nix environments in vscode and still have command client / server work.

I haven't actually tested this on windows, as I don't have it, so I'm not sure if the path I used is sufficient. This will have a corresponding pull request for community, and unfortunately this means that things will break for people that don't update both at the same time.

Its also worth noting that in the issue 966 above, there was a suggestion to use XDG_RUNTIME_DIR on Linux, which I had implemented and tested originally. However, when looking into snaps I ran across https://forum.snapcraft.io/t/rethinking-how-we-handle-xdg-runtime-dir/22223 which suggests that they may want to change snaps to no longer have access to the same XDG_RUNTIME_DIR folder. If they actually ever implemented that then this would break, so in the end I opted not to use it.

There's also per user temp directories on mac, which you can query using getconf DARWIN_USER_TEMP_DIR, but I decided not to keep that code in the end since it keeps the mac/linux case simpler both using /tmp, especially in light of not using XDG_RUNTIME_DIR in the end.

Copy link
Owner

@pokey pokey left a comment

Choose a reason for hiding this comment

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

Looks fine with some comments.

Also, we need to figure out backwards compatibility here. The command-server VSCode extension will be updated automatically by VSCode, whereas the Talon side requires a manual git pull.

The VSCode extension creates the communication directory on init. I think the easiest / most robust thing to do is to

  • create both old and new directories on init
  • every time the client hits the keystroke that triggers a read, look in both directories for request.json and use the newer one (or the only one that exists if only one exists)

There are plenty of ways to optimise that approach, but I think that's a robust way to start. cc/ @lunixbochs

src/paths.ts Outdated Show resolved Hide resolved
src/paths.ts Outdated Show resolved Hide resolved
src/paths.ts Outdated Show resolved Hide resolved
@fidgetingbits
Copy link
Author

Looks fine with some comments.

Also, we need to figure out backwards compatibility here. The command-server VSCode extension will be updated automatically by VSCode, whereas the Talon side requires a manual git pull.

The VSCode extension creates the communication directory on init. I think the easiest / most robust thing to do is to

* create both old and new directories on init

* every time the client hits the keystroke that triggers a read, look in both directories for `request.json` and use the newer one (or the only one that exists if only one exists)

There are plenty of ways to optimise that approach, but I think that's a robust way to start. cc/ @lunixbochs

I updated the windows folder here to use .comms to match the other side.

I also added code for handling backwards compatibility as suggested above, and have tested it on both the old and new folders (on Linux only so far) and confirmed it works.

I did briefly run into an issue when testing back and forth over and and over again that sometimes cursorless thinks it can't find hat markers and that lasts for some amount of time (maybe the minute timeout?), which I seems to be related to the prePhrase signaling, though I'm not very familiar with how that code works yet. The hat errors would eventually go away and then it would work normally.

I think that might have been due to some stale data being present in the signaling folder though, as it's not happening for me anymore. But I thought I'd mention it in case it helps ring any alarm bells while you're reviewing the changes.

@pokey
Copy link
Owner

pokey commented Jan 22, 2024

I made a bunch of changes code because the order in commandRunner is really important, and it needs to be as clear as possible what's happening there. This code is really subtle, so I wanted NativeIo and commandRunner to be as simple as possible. @lunixbochs and I spent a bunch of time working out the different possible ways things could fail so I want to preserve that order

Lmk if my changes don't make sense

I tested commands and prePhrase signal on both community main and talonhub/community#966

@pokey
Copy link
Owner

pokey commented Jan 22, 2024

btw @bjaspan your abstraction is already paying off 🙌

@fidgetingbits
Copy link
Author

Tested it and it works well on my end too, and without the hat warnings I had run into. Also looks good to me.

@fidgetingbits
Copy link
Author

Looks like you dropped the .comms folder here but not on the talon side? I'm on the mobile app so can't add a comment directly on the change it seems.

@pokey
Copy link
Owner

pokey commented Jan 23, 2024

Looks like you dropped the .comms folder here but not on the talon side? I'm on the mobile app so can't add a comment directly on the change it seems.

I don't think so. Maybe have another look on desktop?

@fidgetingbits
Copy link
Author

Ah ya I see now, disregard.

@saidelike
Copy link
Contributor

Just to say that I've tested this PR with neovim on Windows and it works fine:

2024-06-10 16:58:39 INF loadExtension(command-server): start
2024-06-10 16:58:39 INF Creating communication dir C:\Users\User\AppData\Roaming\talon\.comms\neovim-command-server
2024-06-10 16:58:39 INF Creating communication dir C:\Users\User\AppData\Local\Temp\neovim-command-server
2024-06-10 16:58:39 INF loadExtension(command-server): done

@fidgetingbits
Copy link
Author

Rebased this finally because vscode likes to occasionally ignore my don't auto-update extension settings sometimes and clobber the custom 0.9 extension I use with this fix. Still will need to be tweaked depending on how #27 develops.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants