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

[WIP] Advanced tab-completion for the console #1840

Merged
merged 12 commits into from
Apr 7, 2022

Conversation

blinry
Copy link
Contributor

@blinry blinry commented Feb 13, 2022

I'd appreciate code review/feedback/help on this! I'm building better tab-completion, which is aware of the different commands. It knows how to complete files, directories, help topics, commands, and the import/export types:

vid22

Implementation notes:

  • Console commands get two callbacks for autocompleting their first/second option. Both can be null when they don't take parameters.
  • An AutocompleteData struct keeps track of the word the user is trying to tab-complete, the options that have been gathered so far, as a string, and the longest common prefix of all options, in order to add it to the typed command later.
  • An addAutocompleteOption adds a possible completion option to the struct.
  • In the end finishAutocomplete is called to actually perform the tab-completion, its shows all options and expands the typed command.

Questions/things to do:

  • The code keeps track of the available options in a char* of size CONSOLE_BUFFER_SIZE. That seems excessive, but I'm not sure which other maximum size to pick?
  • Would it be good to sort the available options? Is there already some code sorting string arrays in the codebase?
  • Why is other code dealing with tic_fs_enum using MOVE to pass the data object? I should probably do that, as well? But I don't understand why. -> it's an async function
  • When using cd tic80.com and load w<tab>, TIC-80 crashes in addAutocompleteOption. I guess this could be related. -> It was!
  • There are probably edge cases in finding the longest common prefix when completing from an empty string. Right now, line 1162 can't differentiate between not having seen any option, or the situation where options had nothing in common already. Would need an additional flag or something? -> Checking the list of options so far works!
  • Looks like the Windows builds are failing right now.
    D:\a\TIC-80\TIC-80\src\studio\screens\console.c(3188): error C2065: 'incompleteWord': undeclared identifier [D:\a\TIC-80\TIC-80\build\tic80studio.vcxproj]
    D:\a\TIC-80\TIC-80\src\studio\screens\console.c(3188): error C2059: syntax error: ':' [D:\a\TIC-80\TIC-80\build\tic80studio.vcxproj]
    

@nesbox
Copy link
Owner

nesbox commented Feb 14, 2022

The code keeps track of the available options in a char* of size CONSOLE_BUFFER_SIZE. That seems excessive, but I'm not sure which other maximum size to pick?

Yes, CONSOLE_BUFFER_SIZE looks excessive, I think CONSOLE_BUFFER_SCREEN should be enough here.

Would it be good to sort the available options? Is there already some code sorting string arrays in the codebase?

Would be good, pls look at https://github.com/nesbox/TIC-80/blob/main/src/studio/screens/console.c#L3913 where we sort commands for example.

Why is other code dealing with tic_fs_enum using MOVE to pass the data object? I should probably do that, as well? But I don't understand why.

tic_fs_enum is an asynchronous function and you should store data objects on the heap, not on the stack. I prefer to initialize the data object on the stack and then move a copy of it onto the heap with the MOVE macro. Of course, you do not need to do this, it's enough to allocate memory and initialize it in any way convenient for you.

When using cd tic80.com and load w, TIC-80 crashes in addAutocompleteOption. I guess this could be related.

It needs an investigation.

There are probably edge cases in finding the longest common prefix when completing from an empty string. Right now, line 1162 can't differentiate between not having seen any option, or the situation where options had nothing in common already. Would need an additional flag or something?

Don't know what to say, make your own decision :)

Looks like the Windows builds are failing right now.

Seems you should use AutocompleteData data = { console, .incompleteWord = secondParam }; here

Thanks for the amazing work.

@nesbox
Copy link
Owner

nesbox commented Apr 7, 2022

Can we merge it before the release?

@nesbox nesbox marked this pull request as ready for review April 7, 2022 11:26
@nesbox nesbox merged commit 831e04a into nesbox:main Apr 7, 2022
nesbox added a commit that referenced this pull request Apr 7, 2022
@blinry
Copy link
Contributor Author

blinry commented Apr 7, 2022

Implementing sorted completions would likely need a few more hours of work – but I'm really happy with this PR as-is! We can always add sorting later! :)

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