linutil
linutil copied to clipboard
ChrisTitusTech
Close linutil on Control-C, don't reset cursor position on exit
Pull Request
Title
Added the ability to exit the application with Control-C to match the expected behavior of some users Added a screen blank on exit to prevent text present on screen from persisting from before starting the application.
Type of Change
- [x] New feature
- [x] Bug fix
- [ ] Documentation Update
- [x] Refactoring
- [ ] Hotfix
- [ ] Security patch
- [ ] UI/UX improvement
Description
Control-C is the universal "I want to get out of here" chord. Having the binding only exist on the 'q' key makes it unintuitive to exit the app while running. It was only a few lines that needed to be added to accomplish this.
Also, I noticed that stale text would be persisted from before starting the application, so I removed the cursor position reset and it seems to be fixed.
Testing
I verified that both 'q' and Control-c behave as expected, and I ran a test neovim install to verify that package management functionality wasn't broken.
Impact
I believe that this makes the application behave as a naive user would expect.
Issue related to PR
- Resolves #236
Checklist
- [x] My code adheres to the coding and style guidelines of the project.
- [x] I have performed a self-review of my own code.
- [x] I have commented my code, particularly in hard-to-understand areas.
- [x] I have made corresponding changes to the documentation.
- [x] My changes generate no errors/warnings/merge conflicts.
it looks like #239 also adds a tips box, if this gets merged, we might want to add a note for "Control-C" as well @prschorn
@cartercanedy should we add the control-c note already on #239, or wait for this to merge and then create a second PR adding that?
@cartercanedy should we add the control-c note already on #239, or wait for this to merge and then create a second PR adding that?
I'd just wait until this gets merged and create a new pr
This should not be merged, it brakes pressing CTRL-c when a command is running to stop it. Instead the whole app will just die. The approach to cleaning the screen is also wrong. There is no need to clear the screen, just no need to restore the position either.
I'll rework the logic to scope the control-c to cancel installations if it's focused.
@JustLinuxUser Just fixed the edge case that you brought up, should be ready to review again
I think you didn't git push something
might need a git pull on your end?
Hey, I know github supports shared (co-authored) pull requests, what to share the PR with me? I can add your CTRL-c handling logic to my PR, and we can both have credit, plus you get a GH achievement if you don't have it already @cartercanedy. And there is no chance of having a PR conflicting because Titus chose to apply 1 before / after the other, so everybody wins
Hey, I know github supports shared (co-authored) pull requests, what to share the PR with me? I can add your CTRL-c handling logic to my PR, and we can both have credit, plus you get a GH achievement if you don't have it already @cartercanedy. And there is no chance of having a PR conflicting because Titus chose to apply 1 before / after the other, so everybody wins
it's pretty simple to just rebase on top of main if this gets merged
Hey, I know github supports shared (co-authored) pull requests, what to share the PR with me? I can add your CTRL-c handling logic to my PR, and we can both have credit, plus you get a GH achievement if you don't have it already @cartercanedy. And there is no chance of having a PR conflicting because Titus chose to apply 1 before / after the other, so everybody wins
it's pretty simple to just rebase on top of main if this gets merged
Yes, but rn the PRs will conflict with each other, if we make it 1 pr, it won't conflict with each other. But If I just delete the portion where I removed the restore position thingie from my PR the conflict gets resolved, so I will do that
1 last thing, mb don't add esc as another way to exit, esc is already used to exit from a terminated command / preview, and pressing it 1 more time is just too easy
1 last thing, mb don't add
escas another way to exit,escis already used to exit from a terminated command / preview, and pressing it 1 more time is just too easy
I could go both ways. Esc would be the third way to exit, so kinda redundant. On the other hand, we use 'q' to exit previews, so the same argument you made could be applied there as well
1 last thing, mb don't add
escas another way to exit,escis already used to exit from a terminated command / preview, and pressing it 1 more time is just too easyI could go both ways. Esc would be the third way to exit, so kinda redundant. On the other hand, we use 'q' to exit previews, so the same argument you made could be applied there as well
I didn't know that, I think ading q to exit previews is a bad idea too, you already had Enter and esq
1 last thing, mb don't add
escas another way to exit,escis already used to exit from a terminated command / preview, and pressing it 1 more time is just too easyI could go both ways. Esc would be the third way to exit, so kinda redundant. On the other hand, we use 'q' to exit previews, so the same argument you made could be applied there as well
I didn't know that, I think ading
qto exit previews is a bad idea too, you already hadEnterandesq
removed
#239 should be deferred for a while, so it'll need to be rebased anyway. I think this is good to merge, just need approval
sorry for all of the force-pushes, I was battling the buildbot in my own fork without realizing it
Sorry for the inconvenience. We had a massive restructure of the codebase to improve future development. Because of this can you update your PR to the new structure. Thank you for your assistance and contribution.
