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

Interactive Mode #17

Open
khamer opened this issue Dec 3, 2016 · 8 comments
Open

Interactive Mode #17

khamer opened this issue Dec 3, 2016 · 8 comments

Comments

@khamer
Copy link
Contributor

khamer commented Dec 3, 2016

I'm willing to put together a pull request for interactive mode, but I'd most likely want to depend on junegunn/fzf for the interactive functionality. @pimterry - does that jive with what you were looking to do?

@pimterry
Copy link
Owner

pimterry commented Dec 5, 2016

I don't know how distribution is going to work easily if we start bundling in extra packages - I have a very strong preference for being able to pull down and get started in a single command on any system with a standard shell available, and I worry that this would start to break that.

I'm also very cautious about letting the core of notes itself get too large, rather than being a set of small dumb commands. I want a toolkit of components, not a big blob that does everything.

Is there a practical way this could be layer on top of the command line notes interface? What commands would notes have to provide to make it possible to build a UI separate that called into it to provide this interface? We already just build an autocomplete-style dropdown by just calling notes find with the provided input, I think. Does that sound like a potentially practical direction for what you're imagining?

@khamer
Copy link
Contributor Author

khamer commented Dec 6, 2016

Something like

$ editor $NOTES_DIRECTORY/$(notes find | fzf)

Does what I'm suggesting. I can dig a little bit to see what it'd take to simplify this. Would you rather me investigate dmenu than fzf? That seems like another viable option to provide interactive mode, and I've done a fair amount of scripting with both.

@primis
Copy link
Collaborator

primis commented Dec 7, 2016

We could create a new command, one called "notes" and the other called "vnotes" or something, and vnotes would be visual mode. This would solve the "blob" problem quite well.

@pimterry
Copy link
Owner

pimterry commented Dec 7, 2016

@primis Yeah, a separate command that wraps notes feels like a good place to start.

@khamer:

I have no preference - build it with whatever you think works best!

Just to focus on your example for a second: in general, we should be avoiding this tool understanding notes internals too much, and it should instead be making calls to the built-in commands. As a case in point, that example using $NOTES_DIRECTORY is going to break if we allow setting the notes directory in config (#21), and that example also doesn't know the default notes directory to use if the env var isn't set. I know it's only an illustrative example, but just something to watch out for.

That doesn't necessarily mean you can't start with an implementation that knows about those internal details, but it'd be good to keep a note of what information it has to duplicate, so we can eventually expand the built-in notes commands so that that's no longer necessary (in the above example: by completing #23, so you can get the full note paths from notes directly)

@nosarthur
Copy link

I have notes 1.0.0 and vim 8.2.3025

This command works where n is an alias to notes

n o `n ls |fzf`

But this one doesn't work completely

n ls | fzf | n o

There is a warning

Vim: Warning: Input is not from a terminal

The correct file is still opened. But after exiting vim, my bash is messed up: terminal input doesn't show up. Why the 2nd form doesn't work?

@pimterry
Copy link
Owner

pimterry commented Sep 7, 2021

Thanks for the info @nosarthur. I've just done some testing, and it seems this was a bug that was introduced in 1.0.0 by this change: 15f870e.

That was added to fix GitHub Actions, where /dev/tty is not available. It's actually buggy though, it's using the wrong test, so it effectively disables the </dev/tty part for all cases, which means in some piped cases, as you're seeing, you hit issues.

I think I've fixed this in 6315323. Can you test that out and let me know?

@nosarthur
Copy link

Thanks @pimterry The new commit 6315323 works

@pimterry
Copy link
Owner

pimterry commented Sep 7, 2021

Great! Thanks @nosarthur, that fix is now released as v1.0.1.

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

No branches or pull requests

4 participants