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

Add limited cursor support (left / right) #45

Merged
merged 12 commits into from
Jan 16, 2024

Conversation

icamaster
Copy link
Contributor

This adds limited cursor support for cursor left / right. Let me know what you think.

I started working on this a few days ago, but just noticed that you have put this repo in maintenance mode. Does this mean no PRs for features will be accepted? Thanks!

@funbiscuit
Copy link
Owner

Thanks for your contribution! PRs are always welcome, I'll look into this one when I have time.
But I see that CI doesn't pass, please take a look at errors.

@icamaster
Copy link
Contributor Author

Thanks, I'll have a look at the failures later this week. Feel free to ignore it until I add the fixes.

@icamaster
Copy link
Contributor Author

I've made a small change that fixes some of the errors seen, but that introduced another bug, unfortunately. I'll let you know when it's ready.

Copy link

codecov bot commented Jan 9, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (a1d3935) 81.68% compared to head (097cca5) 82.67%.
Report is 1 commits behind head on master.

Files Patch % Lines
lib/src/embedded_cli.c 94.44% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
+ Coverage   81.68%   82.67%   +0.99%     
==========================================
  Files           2        2              
  Lines         535      560      +25     
  Branches      130      135       +5     
==========================================
+ Hits          437      463      +26     
+ Misses         64       63       -1     
  Partials       34       34              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@icamaster
Copy link
Contributor Author

icamaster commented Jan 9, 2024

Ok, I believe I have added all the necessary fixes now.

Copy link
Owner

@funbiscuit funbiscuit left a comment

Choose a reason for hiding this comment

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

I finally found some time to look at code. I've added some questions and there is also a bug:

  • Enter 'abcd'
  • Press Left two times
  • Press backspace five times
    You'll see that command prompt > was cleared and output is corrupted.

There are some other bugs but it looks like they will be fixed if this one is fixed (they all appear when left and backspace are used). Please take a look and fix it. Ideally, add a test case first, that reproduces the bug.

Also this change breaks arduino example, now it requires two more bytes to work. Please edit lines 23 and 24 to use 166 bytes instead of 164.

README.md Outdated Show resolved Hide resolved
lib/src/embedded_cli.c Outdated Show resolved Hide resolved
lib/src/embedded_cli.c Outdated Show resolved Hide resolved
lib/src/embedded_cli.c Outdated Show resolved Hide resolved
@icamaster
Copy link
Contributor Author

Thank you for looking at this PR.

I finally found some time to look at code. I've added some questions and there is also a bug:

  • Enter 'abcd'
  • Press Left two times
  • Press backspace five times
    You'll see that command prompt > was cleared and output is corrupted.

There are some other bugs but it looks like they will be fixed if this one is fixed (they all appear when left and backspace are used). Please take a look and fix it. Ideally, add a test case first, that reproduces the bug.

I think I have fixed the bug you mentioned and added a few tests to check for it.

Two other 'quirks' which I am not sure if they should be fixed or not are:

  • Doing an autocomplete adds a whitespace. If you don't delete this whitespace, then further autocompletes won't work if you forget to delete it. The fix would be to trim the current input before autocompletion, but not sure if it needs to be done. For example, using the 'Windows' example which has a binding for 'hello' :

    • Type 'hell'
    • Press tab to autocomplete
    • Press left key to move cursor once then backspace to delete 'o'
    • Press tab to autocomplete again - this will fail because of the whitespace at the end
  • Requesting an autocomplete in the middle of a command by pressing 'tab' doesn't bring the cursor to the end if the command is already fully typed. This is mainly because 'getAutocompletedCommand' returns 'candidateCount = 0' for a fully typed command. Should this be the expected behavior? For example, using the 'Windows' example which has a binding for 'hello' :

    • Type 'hello'
    • Press the left key two times to move the cursor to the left two spaces
    • Press tab to autocomplete - nothing happens

Also this change breaks arduino example, now it requires two more bytes to work. Please edit lines 23 and 24 to use 166 bytes instead of 164.

Good spot, fixed this. I believe this could be improved in the checks by adding some 'static_assert's, but might require some rewrite and is out of the scope of this PR.

@funbiscuit funbiscuit self-requested a review January 15, 2024 08:18
@funbiscuit
Copy link
Owner

Doing an autocomplete adds a whitespace.

Ideally, this should be fixed. If you have time, you can fix it in this PR. But it may be not as straight-forward as it might seem. For example input hello (there are 4 spaces) after two left keypresses and tab should still have all extra spaces to the right of the cursor. So maybe it's a good idea to do this in a separate PR (or never bother with it at all). I'm okay with merging this PR in current state - everything looks great.

Requesting an autocomplete in the middle of a command by pressing 'tab' doesn't bring the cursor to the end if the command is already fully typed

I wasn't able to reproduce it. Can you check if following test will fail?

  • Input hello
  • Press left two times
  • Press tab
  • expect input to be > hello and cursor position to be 8

If I do this in linux example, everything works as expected.

@icamaster
Copy link
Contributor Author

Thank you for looking at this so soon.

Doing an autocomplete adds a whitespace.

Ideally, this should be fixed. If you have time, you can fix it in this PR. But it may be not as straight-forward as it might seem. For example input hello (there are 4 spaces) after two left keypresses and tab should still have all extra spaces to the right of the cursor. So maybe it's a good idea to do this in a separate PR (or never bother with it at all). I'm okay with merging this PR in current state - everything looks great.

Agree that it might be a bit complex. I think I will look at this in the future, but will keep it out of this PR for now. Happy to raise an issue against this if you end up merging this PR.

Requesting an autocomplete in the middle of a command by pressing 'tab' doesn't bring the cursor to the end if the command is already fully typed

I wasn't able to reproduce it. Can you check if following test will fail?

  • Input hello
  • Press left two times
  • Press tab
  • expect input to be > hello and cursor position to be 8

If I do this in linux example, everything works as expected.

You are correct, it is working as expected.

I looked again over my initial notes and realized that it was the same problem as described above, I just forgot to mention that I used autocomplete (tab) before moving the cursor left:

  • Input hello
  • Press left two times
  • Press tab
  • Expect input to be > hello and cursor position to be 8
  • Press left two times again
  • Press tab again
  • Expect nothing to happen because of the whitespace added by the first autocomplete

@funbiscuit funbiscuit merged commit ffa8014 into funbiscuit:master Jan 16, 2024
9 checks passed
@funbiscuit
Copy link
Owner

Merged into master, thanks again for great contribution!

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.

2 participants