kdheepak / taskwarrior-tui

`taskwarrior-tui`: A terminal user interface for taskwarrior
https://kdheepak.com/taskwarrior-tui
MIT License
1.5k stars 68 forks source link

Refactor for maintainability and features #349

Open bartenbach opened 2 years ago

bartenbach commented 2 years ago

So I have been using this a ton lately because it's fantastic. Using it so frequently means I have had a lot of time to come up with little ideas. (Maybe some are already possible and I just don't know it).

  1. Reverse video for selected item. I see bold, italic, dim, blink, but no reverse video. I'd just like the selected item to be reverse video, and have no selection character. I guess the color of the reverse video would change (and maybe that is why it is not implemented). Perhaps a static reverse video highlighted item color would be better, but I think just toggling reverse video would be easier.

Use case: granted, this is due to my own configuration, but you can see how when you are already using reverse video, the reverse video item actually intuitively seems like it would be the highlighted item, but it's not. image

  1. vi-mode for rustyline The bindings are mostly vi anyway, but not the editor line. It looks like rustyline supports this, but I don't know if this is trivial or not. I find myself wanting to use things like '0' or '$' or 'dw' in the edit line, but it raises some questions like, how do you then submit your changes? Enter in insert mode would simply create a new line. That would actually be pretty cool for annotating tasks with a lot of information, but you'd need an intuitive way to write out the changes. The line editor window would probably need to grow as well to view the multiple lines you were editing.

  2. Escape annotation text Sometimes when annotating a task, I'll type a word like "don't" with an apostrophe absent-mindedly. This causes an error, and I usually have to either quote the entire annotation or change it to "do not." It would be nice if this was handled automatically somehow, but again I have no clue what the level of effort for that would be. image image

Also, I realize that grouping several feature requests in one is a horrible way for tracking them, so just let me know what you'd like me to do with them and I can do whatever. I just wanted to throw them out there first and get a feel for what your thoughts were.

kdheepak commented 2 years ago

Thanks for opening the issue! I'm glad taskwarrior-tui is being useful for you, and I'm always happy to hear feature requests!

1) Reverse video for selected item.

I could have sworn I had implemented this already. I searched through the git history as well, and nope, turns out I hadn't. I've added it in a PR, and it'll be part of the next release.

2) vi-mode for rustyline

I've occasionally found myself wishing for the same too, but I'm not sure what the best way to go about this would be. As you mentioned, growing the line editor + keeping track of position of cursor + wrapping lines is a challenge. I remember experimenting with multiple lines and deciding to punt on that for the time being.

At the very least, I won't be able to do with until I refactor the code base. Currently, there's some technical debt from when I first wrote this tool and didn't really know what I was doing :)

However, on the topic at hand, one simple idea that might be useful is to allow the user to open the contents of "command pane" (what you called the line editor) in the $EDITOR and use that content for submission. Currently hitting e on the task opens the editor for that task using taskwarrior's task edit feature, which is what I use when I expect to be editing a longer annotation, but it would be nice to have something similar for the command pane. It would be a lot more efficient to delegate the responsibility of editing a line of text to a proper editor, at the cost of a slightly degraded user experience. Thoughts?

3) Escape annotation text

This is also really annoying issue, and I'm tried multiple workarounds for this all leading to various bugs and issues. I finally decided to leave it the way it was (relevant discussions: https://github.com/kdheepak/taskwarrior-tui/issues/208, https://github.com/kdheepak/taskwarrior-tui/issues/231).

There's two workarounds to this right now:

1) you can surround any string with double quotes, i.e. "whoops I can't do this" or whoops I "can't" do this will work, or 2) you can escape the ' using a \, i.e. whoops I can\'t do this.

Basically, whatever you type in the command window is sent to the shell command of taskwarrior, i.e. if you were trying to add a task and you typed whoops I can't do this, taskwarrior-tui tries to run task add whoops I can't do this. The best idea I have here is to prefill the command pane with double quotes. When the user hits a to add a task, it'll show this prompt.

Screen Shot 2021-12-31 at 2 56 22 PM

This is the same suggestion I made here (https://github.com/kdheepak/taskwarrior-tui/issues/231#issuecomment-852160950), but never really followed up on it. And I think I'm leaning towards it. The only issue is that typing and submitting "whoops I can't do this +tag1 +tag2" will add a task with that exact string as the task itself and will not add any tags. Users can do "whoops I can't do this" +tag1 +tag2 and that'll work as expected. Thoughts?

bartenbach commented 2 years ago

Thanks for opening the issue! I'm glad taskwarrior-tui is being useful for you, and I'm always happy to hear feature requests!

  1. Reverse video for selected item.

I could have sworn I had implemented this already. I searched through the git history as well, and nope, turns out I hadn't. I've added it in a PR, and it'll be part of the next release.

Nice, thanks!

  1. vi-mode for rustyline

I've occasionally found myself wishing for the same too, but I'm not sure what the best way to go about this would be. As you mentioned, growing the line editor + keeping track of position of cursor + wrapping lines is a challenge. I remember experimenting with multiple lines and deciding to punt on that for the time being.

At the very least, I won't be able to do with until I refactor the code base. Currently, there's some technical debt from when I first wrote this tool and didn't really know what I was doing :)

However, on the topic at hand, one simple idea that might be useful is to allow the user to open the contents of "command pane" (what you called the line editor) in the $EDITOR and use that content for submission. Currently hitting e on the task opens the editor for that task using taskwarrior's task edit feature, which is what I use when I expect to be editing a longer annotation, but it would be nice to have something similar for the command pane. It would be a lot more efficient to delegate the responsibility of editing a line of text to a proper editor, at the cost of a slightly degraded user experience. Thoughts?

So now that I think about this more, multiple lines is really a separate issue. If the command pane's vi mode is just like the normal shell vi mode, pressing Enter in insert mode would "submit" it just like it does now. The only difference is that pressing Esc puts you into normal mode (just like set -o vi with bash or bindkey -v with zsh).

I am probably oversimplifying this though, because I looked at rustyline which does appear support vi mode, but I could not figure out how to tie that into the current code base. That may be the tech debt you are referring to. It is one of those things that looks like it should be easy in theory, but I couldn't figure out how to do it.

Opening up an actual editor seems like a cool idea. If you annotate paragraphs of text I'm not sure how it will display in the UI though. Perhaps there could be an indicator of some sort that there is more text than can be displayed, and the user can have a key binding open the annotations with $PAGER which is usually something like less? I'm not sure how it all works behind the scenes, but giving people the green light to write giant chunks of text may also exacerbate the shell splitting issue. If you return from $EDITOR but the text was not able to be parsed, I'm not sure what would happen.

  1. Escape annotation text

This is also really annoying issue, and I'm tried multiple workarounds for this all leading to various bugs and issues. I finally decided to leave it the way it was (relevant discussions: #208, #231).

There's two workarounds to this right now:

  1. you can surround any string with double quotes, i.e. "whoops I can't do this" or whoops I "can't" do this will work, or
  2. you can escape the ' using a \, i.e. whoops I can\'t do this.

Basically, whatever you type in the command window is sent to the shell command of taskwarrior, i.e. if you were trying to add a task and you typed whoops I can't do this, taskwarrior-tui tries to run task add whoops I can't do this. The best idea I have here is to prefill the command pane with double quotes. When the user hits a to add a task, it'll show this prompt.

Screen Shot 2021-12-31 at 2 56 22 PM

This is the same suggestion I made here (#231 (comment)), but never really followed up on it. And I think I'm leaning towards it. The only issue is that typing and submitting "whoops I can't do this +tag1 +tag2" will add a task with that exact string as the task itself and will not add any tags. Users can do "whoops I can't do this" +tag1 +tag2 and that'll work as expected. Thoughts?

Yeah, this is much trickier than I realized. I think it stems from the fact that the tui doesn't feel like a shell prompt, so it's easy to forget that it's all interpreted by the shell.

Annotate mode feels like it should be free form text input, but that's not the case. When using task annotate you can still append modifiers like +tag that are interpreted correctly (IE task annotate i am an annotation +tag). If it wasn't for that, this problem would be relatively straightforward.

I suppose you could logically separate free form inputs (description and annotations) from task modifiers like due:, pri:, +tag, etc. Unfortunately that makes everything much more inconvenient as you cannot do it all on one line, which I really don't like.

You could write an incredibly complicated string parser that would probably break and become a maintenance nightmare, detecting words with colons and splitting them, detecting words that start with +. What could possibly go wrong?

What about some kind of signal? This would differ from the shell's behavior, which is unfortunate, but perhaps some special sequence could indicate that everything that follows is a modifier. Something like: image

Interestingly enough, if you annotate the sequence -- it actually gets removed from the string. The problem with this is that the behavior differs from the underlying program which is never a good thing, and becomes some kind of "special knowledge" you have to have to use the program.

I don't know, I'm out of ideas. Every solution I can come up with sounds worse than the original problem.

kdheepak commented 2 years ago

Thanks for the detailed comment, that's good brainstorming.

Every solution I can come up with sounds worse than the original problem.

I think I went through a very similar thought process and came to the similar conclusion. There's a bunch of issues related to escaping new lines, escaping backslashes etc that I've unfortunately just had to dust under the rug.

But because taskwarrior-tui simply relies on how subprocess works + taskwarrior's semantics, I'm operating under the assumption that surfacing error messages appropriately is sufficient. I believe most users that are familiar with the command line / how subprocesses work / how taskwarrior handles input will find this the acceptable. I do think for other users that want a more "intuitive" interface and nicer abstractions, there's a lot more that can be done, but it'll be a lot of work and the maintenance cost for me will increase significantly. This definitely won't happen until I refactor parts of the code base.

I am probably oversimplifying this though, because I looked at rustyline which does appear support vi mode, but I could not figure out how to tie that into the current code base.

I think it wouldn't be too hard to support vi mode for movement / navigation right now. The harder part is supporting things like dw for delete word. The current mechanism for input handling does not deal with multiple inputs as a sequence to perform an action. Every time you press a key, a event is generated and currently that event is consumed by the "App" to handle it. I'd have to refactor it such that events are buffered in some manner, and a sequence of events in some timeout period maps to an action, which is then consumed by the App. This would also allow multiple key sequence configuration too, which currently is not supported. This is partly what I meant when I said technical debt in the earlier comment.

The other part of the technical debt (not relevant but I'm mentioning it here for completeness sake) is that rendering is coupled to layout and logic in a some unnecessary ways. When I do eventually attempt this refactor, then vi mode will definitely be make it in as a feature.

Btw I did just implement the auto double quote insert feature, and from my limited use after implementing it I think it works well. I think that's the best we can do with the current implement to deal with the unescaped single quote ' issue.

bartenbach commented 2 years ago

Thanks for the detailed comment, that's good brainstorming.

Happy to help :)

I think I went through a very similar thought process and came to the similar conclusion. There's a bunch of issues related to escaping new lines, escaping backslashes etc that I've unfortunately just had to dust under the rug.

Yeah, it's definitely a can of worms.

But because taskwarrior-tui simply relies on how subprocess works + taskwarrior's semantics, I'm operating under the assumption that surfacing error messages appropriately is sufficient. I believe most users that are familiar with the command line / how subprocesses work / how taskwarrior handles input will find this the acceptable. I do think for other users that want a more intuitive interface and nicer abstractions, there's a lot more that can be done, but it'll be a lot of work and the maintenance cost for me will increase significantly. This definitely won't happen until I refactor parts of the code base.

I think you're right. The target users are going to understand what's happening, and as you said the maintenance cost of starting to add layers that modify the interaction between using task at the command line and using it from the TUI are going to add complexity and overhead.

I think it wouldn't be too hard to support vi mode for movement / navigation right now. The harder part is supporting things like dw for delete word. The current mechanism for input handling does not deal with multiple inputs as a sequence to perform an action. Every time you press a key, a event is generated and currently that event is consumed by the "App" to handle it. I'd have to refactor it such that events are buffered in some manner, and a sequence of events in some timeout span map to actions, which are consumed by the App. This would also allow multiple key configuration too, which currently is not supported. This is partly what I meant when I said technical debt in the earlier comment.

The part that confused me was that the API appears to support vi mode, but what does that mean? I was hoping it handled some of that automatically, but maybe it doesn't. I'm not sure what happens when you change modes. Maybe nothing? I saw there was a configuration builder where you could set quite a few different options: https://docs.rs/rustyline/latest/rustyline/config/struct.Builder.html#method.edit_mode

Btw I did just implement the auto double quote insert feature, and from my limited use after implementing it I think it works well. I think that's the best we can do with the current implement to deal with the unescaped single quote ' issue.

I just tested it out, and I think for annotating it's absolutely perfect. For adding tasks though, it actually adds work for me, so I'm not sure I'm sold on that aspect. When I add tasks I always add some UDAs right after the description. Is it possible to only do it for annotating? Or does that make the behavior confusing? Maybe it could be configurable? Maybe one binding opens annotate and another opens quoted annotate? That would probably be the best of both worlds, as you could do whatever you wanted. If multiple inputs as a sequence worked you could do something like A for annotate and qA for quoted annotate. a for add task and qa for quoted add task. I'm just dreaming up ideas without looking at the code base though, so it's hard to say.

kdheepak commented 2 years ago

Thanks for the feedback. I was planning to use it for a little bit before making a release too.

Maybe it could be configurable?

I think I'd like the auto insert double quotes to always be on by default, since even though the subprocess behaviour makes sense when users think about it, wrapping it in quotes gives less surprising behavior. There have been a number of issues reported by users related to this, and having double quotes inserted by default would have informed them more clearly as to what was going on.

I have added an option to turn off auto insert quote though, and it is configurable per type of action (only for task add, task annotate or task log). For the remaining actions (filter, subprocess, etc) it'll never auto insert double quotes.

Maybe one binding opens annotate and another opens quoted annotate? That would probably be the best of both worlds, as you could do whatever you wanted.

I thought about this too, and it is just annoying to implement. Maybe in the future :)

The part that confused me was that the API appears to support vi mode, but what does that mean?

Regarding rustyline, I'm using a very small subset of their feature set. Specifically, I'm using the LineBuffer struct from that crate. This just lets me call a function to move the cursor to different locations and/or perform mutations on the string, i.e. move before word, delete next word etc. Which function is called is entirely handled in taskwarrior-tui. See handle_movement:

https://github.com/kdheepak/taskwarrior-tui/blob/2275f75373bf02690f9c2d48087afdb8a031a765/src/app.rs#L3461-L3506

So supporting vi mode for single key stroke movement would be straightforward, we'd just have to add more matches here along with a check for a mode. If we wanted to do multiple keystroke actions though, I'd have to refactor this function (and a few other functions) to not just take an Key event, but take an TaskwarriorTUICommandEvent. Currently, the Key event defined here:

https://github.com/kdheepak/taskwarrior-tui/blob/2275f75373bf02690f9c2d48087afdb8a031a765/src/event.rs#L20-L45

is a one to one mapping of the crossterm Key event, i.e. whatever the key the user presses:

https://github.com/kdheepak/taskwarrior-tui/blob/2275f75373bf02690f9c2d48087afdb8a031a765/src/event.rs#L85-L126

I'd have to modify make it such that this Key enum wasn't the key a user pressed, but the action the user wanted to take. So that'd require looking up the key the user pressed in the key configuration and generating an appropriate event from whatever new Enum I create to handle this. I think this would be a breaking change in how the key configuration is handled, but I'm fine with doing that. I just need to find the time to get around to doing it :)

kdheepak commented 2 years ago

Mind if I co-opt this issue to track progress on the refactoring work? I've described what I want to do more clearly here than anywhere else, and I think I've covered as much as I can currently do about your feature requests.

bartenbach commented 2 years ago

Mind if I co-opt this issue to track progress on the refactoring work? I've described what I want to do more clearly here than anywhere else, and I think I've covered as much as I can currently do about your feature requests.

Not at all. I agree and thanks for being so responsive! I'm super happy with the changes 😸

Ticket summary:

kdheepak commented 2 years ago

Related issues:

kdheepak commented 2 years ago

I made a new release that fixes some of the issues in the top post: https://github.com/kdheepak/taskwarrior-tui/releases/tag/v0.17.0.

bartenbach commented 2 years ago

I made a new release that fixes some of the issues in the top post: https://github.com/kdheepak/taskwarrior-tui/releases/tag/v0.17.0.

Nice! Looking forward to test driving it 😺

As I am sure you are aware of, the version on crates.io is a bit outdated. I'm not sure how the releases there work, I only ask because I usually use it for package management. The developer of Alacritty was able to release a version that was newer but not default version as well, although from the command line I don't see how to query the available versions of a package and it wasn't super intuitive IMO.

Edit: no pressure to update it though, I have it installed locally just fine.

bartenbach commented 2 years ago

@kdheepak I really like both the reverse video and the quotes. I didn't think i would like the quotes at first, but I find in practice that I'm no longer afraid to type good task titles and use proper punctuation. Moving outside of the quotes to add modifiers like due date is really a non issue and only takes a second. Great work.

kdheepak commented 2 years ago

Great! Thanks for the feedback, I also feel similarly. It’s a minor inconvenience moving out of quotes but overall it’s a quality of life improvement.