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: add an inline view mode #648

Merged
merged 2 commits into from Mar 25, 2023
Merged

feat: add an inline view mode #648

merged 2 commits into from Mar 25, 2023

Conversation

pdecat
Copy link
Contributor

@pdecat pdecat commented Dec 19, 2022

  • Use crossterm and inline viewport
  • Reposition cursor on exit and fix The cursor position could not be read within a normal duration errors due to blocking reads

@pdecat pdecat force-pushed the crossterm_inline branch 3 times, most recently from a904b54 to ed2b950 Compare December 19, 2022 13:14
@pdecat
Copy link
Contributor Author

pdecat commented Dec 19, 2022

Partly duplicate of #331 for the crossterm changes.

@ellie
Copy link
Member

ellie commented Feb 6, 2023

Hey @pdecat! Thanks for the work here - once #331 is merged, would we be able to keep the inline view from here?

I think I'd like to make it the default for "up" arrow bindings

@pdecat
Copy link
Contributor Author

pdecat commented Feb 6, 2023

That's the plan 🙂

@pdecat pdecat mentioned this pull request Feb 8, 2023
2 tasks
@pdecat
Copy link
Contributor Author

pdecat commented Feb 8, 2023

Left a comment on https://github.com/ellie/atuin/pull/331/files#r1100358866 to determine the best way to backport fdehau/tui-rs#552.

@conradludgate conradludgate changed the title Use crossterm and add an inline view mode add an inline view mode Mar 23, 2023
@vercel
Copy link

vercel bot commented Mar 23, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
atuin ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 25, 2023 at 4:24PM (UTC)

@conradludgate
Copy link
Collaborator

The diff is now so smol 😄

@pdecat
Copy link
Contributor Author

pdecat commented Mar 23, 2023

Awesome, thanks @conradludgate!

I'll try and take another look at finishing this PR ASAP. Feel free to to hack on it if you like :)

@pdecat
Copy link
Contributor Author

pdecat commented Mar 25, 2023

Looks like this is actually working great in its current state.

I guess what's left to do is to make this new behaviour optional, isn't it?

Also, being able to configure the number of lines would be great too.

@ellie
Copy link
Member

ellie commented Mar 25, 2023

Looks like this is actually working great in its current state.

I guess what's left to do is to make this new behaviour optional, isn't it?

Also, being able to configure the number of lines would be great too.

It does! I've been using it since last night. Started to make it configurable but my parents are visiting this weekend so haven't finished 🤣

  1. Make it optional
  2. Configure the number of lines
  3. Release! 🚀

I think it would also feel a bit better if the search box was at the top for inline mode, but that can wait - I'd like to make the whole UI more configurable

@pdecat
Copy link
Contributor Author

pdecat commented Mar 25, 2023

Added an inline_height setting which can be used to define the number of lines to use in inline mode.
When setting it to zero (the default), inline mode is disabled.

Maybe the naming could be improved.

@conradludgate @ellie WDYT?

@pdecat
Copy link
Contributor Author

pdecat commented Mar 25, 2023

Started to make it configurable but my parents are visiting this weekend so haven't finished

@ellie oops, read that part of your reply too fast, I didn't realize you actually started implementing the setting, sorry about that.

Feel free to scratch my version if you prefer yours.

@pdecat pdecat changed the title add an inline view mode feat: add an inline view mode Mar 25, 2023
@pdecat
Copy link
Contributor Author

pdecat commented Mar 25, 2023

Working on putting the input chunk at the top when inline mode is active...

Edit: I believe it also means reversing the history list direction, which means more work needed. Will open another PR.

@conradludgate
Copy link
Collaborator

We can merge as is, I like it. We can fix the UI later. I've been daily driving for a few days and it still feels pretty nice

@pdecat
Copy link
Contributor Author

pdecat commented Mar 25, 2023

Good for me! :)

Submitted #808 as a draft, will have to come back to it at a later time.

@conradludgate
Copy link
Collaborator

No worries! Thank you for your efforts. They are really appreciated

Copy link
Member

@ellie ellie left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you so much for this, it's a huge improvement <3

@ellie ellie merged commit 13ce5f7 into atuinsh:main Mar 25, 2023
6 checks passed
@pdecat pdecat deleted the crossterm_inline branch March 25, 2023 17:49
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.

None yet

3 participants