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: Basic autocompletion on tab for console commands #12163

Merged
merged 1 commit into from Mar 13, 2024

Conversation

Fefer-Ivan
Copy link
Contributor

@Fefer-Ivan Fefer-Ivan commented Feb 22, 2024

Motivation / Problem

When typing console commands, it is very common to have some kind of autocompletion when pressing tab.
It is also a great way to discover console commands.

Description

When in console, the user pressed tab and have at least one character entered, simple autocompletion will happen:

  1. If the entered text is a prefix to no commands or aliases, nothing will happen.
] abacab
<Tab>
] abacab
  1. If the entered text is a prefix to a single command or alias, it will be replaced with this command/alias.
] he
<Tab>
] help
  1. If there are multiple options, the first one will be auto-competed.
] st
<Tab>
] start_ai
  1. If tab is pressed again, it will change to the next available option.
] start_ai  (from previous example)
<Tab>
] status
  1. If we run out of options, we will restore original text.
] stop_ai
<Tab>
] st

This is the same logic that is used in network_chat_gui. It is extracted into autocomplete.h/.cpp and is used in both places.

Limitations

This is very basic algorithm.
It can be improved by using more context-dependent hints.

For example, when trying to autocomplete start_ai A we can look at the list of installed AIs and autocomplete from it. For example start_ai AdmiralAI.

However, this will require more complex code modifications, so I want to test my waters with more simple PR, before submitting something bigger.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, game_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@michicc
Copy link
Member

michicc commented Feb 22, 2024

I like the idea.

Without having looked at the actual code or tried it, a few formal things:
We have a strict commit message check (see CODINGSTYLE.MD). We also usually like to squash/rebase down to a sane final state, so you can just squash the commits and give a new message.

Also, indentation is with tabs, not spaces.

@glx22
Copy link
Contributor

glx22 commented Feb 22, 2024

Handling of <TAB> needs to be very careful as it activates fast forward.

Also NetworkChatWindow has some kind of autocompletion, might be nice to have some common mechanism.

@Fefer-Ivan Fefer-Ivan changed the title Add: basic autocompletion on tab for console commands Add: Basic autocompletion on tab for console commands Feb 23, 2024
@Fefer-Ivan
Copy link
Contributor Author

Thanks for the feedback.
I have rebased the branch. I don't see any spaces.

Yes, when in game, I can even see Fast-Forward button flash pressed when I press tab.
Any advice how to handle that? Prohibit fast-forward when console is open?

I will take a look at network_gui auto-completion implementation and refactoring into a separate class for reusing.
Thanks.

@ldpl

This comment was marked as off-topic.

@2TallTyler
Copy link
Member

Most hotkeys are disabled when the cursor is captured in a text box, because they are being used to type in the box — try pressing X for transparency when editing the name of a station, for example. I see no reason why should be any different. But it is, both in the console and in text boxes. Changing this might be a separate PR.

@Fefer-Ivan
Copy link
Contributor Author

I have re-implemented the algorithm, used to auto-complete network chat in a generic way.
However, it now allocates more memory and saves all possible suggestions on the first tab press, when old code used to iterate over them with each press.

It leads to more memory allocations, but given that we auto-complete on a pretty small set of options (network client names and towns for network chat and commands and aliases for console), I assumed that code readability and ease of maintenance is more important in this case.

@LordAro
Copy link
Member

LordAro commented Feb 23, 2024

Yeah, we're not talking large amounts of memory here, I can't imagine that's an issue. Especially with the frequency that it'd be used

I will say that I'm not sure it requires 6 new files though - we don't have any sort of requirement for one class per file, so feel free to shove them wherever is most appropriate (which might be 1 new cpp/h files, might be none at all, idk)

@Fefer-Ivan Fefer-Ivan force-pushed the add_basic_autocomplete branch 3 times, most recently from 443a2a8 to d5c9e08 Compare February 24, 2024 13:56
src/autocompletion.h Outdated Show resolved Hide resolved
src/autocompletion.h Outdated Show resolved Hide resolved
@Fefer-Ivan
Copy link
Contributor Author

@glx22 Thanks for the review. I have moved the code as suggested.
I am aware of squash and commit naming requiremenets. I will do it before merging.

Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

Largely codestyle nitpicks

src/autocompletion.h Outdated Show resolved Hide resolved

class AutoCompletion {
protected:
Textbuf* textbuf;
Copy link
Member

Choose a reason for hiding this comment

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

we no longer generally bother with the alignment stuff, especially not multiple "sections"

Also, * goes on the variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

Not everywhere :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looked through the code one more time and fix the only place I noticed.

src/autocompletion.h Outdated Show resolved Hide resolved
Comment on lines 79 to 81
while (prefix.ends_with(" ")) {
prefix.remove_suffix(1);
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a "trim" string function already? Seems like it would be more efficient than the explicit loop like that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked at string_func.h and there are two trim functions:
One for std::string and one for a null-terminated utf-8 strings.
Both of them doesn't combine well with std::string_view (as they can be not null terminated).

I can add one more for std::string_view.
Something like std::string_view StrTrimView(std::string_view s).
It also should be possible to implement StrTrimInPlace as (s = StrTrimView(s)).

Copy link
Contributor Author

@Fefer-Ivan Fefer-Ivan Mar 4, 2024

Choose a reason for hiding this comment

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

Implemented my suggestion and added unit tests.
Please let me know if you want me to extract it into a separate Pr.

src/console_gui.cpp Outdated Show resolved Hide resolved
src/console_gui.cpp Outdated Show resolved Hide resolved
src/network/network_chat_gui.cpp Show resolved Hide resolved
src/network/network_chat_gui.cpp Outdated Show resolved Hide resolved
src/network/network_chat_gui.cpp Show resolved Hide resolved
src/console_gui.cpp Outdated Show resolved Hide resolved
IConsoleWindow::scroll = 0;
IConsoleResetHistoryPos();
this->SetDirty();
}
}

Textbuf *GetFocusedTextbuf() const override
const Textbuf *GetFocusedTextbuf() const override
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why on Earth did it compile without it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The return type of an overriding function shall be either identical to the return type of the overridden function or covariant with the classes of the functions. If a function D::f overrides a function B::f, the return type of the functions are covariant if the satisfy the following criteria:

both are pointers to classes or references to classes98)
the class in the return type of B::f is the same class as the class in the return type of D::f or, is an unambiguous direct or indirect base class of the class in the return type of D::f and is accessible in D
both pointers or references have the same cv-qualification and the class type in the return type of D::f has the same cv-qualification as or less cv-qualification than the class type in the return type of B::f.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL.

But it is confusing, so I would like to fix it.

Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

Code LGTM

@glx22 glx22 merged commit 23d733b into OpenTTD:master Mar 13, 2024
15 checks passed
TrueBrain pushed a commit to TrueBrain/OpenTTD that referenced this pull request Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview This PR is receiving preview builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants