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 exact match search in debug spawns and fix duplicated debug key #1936

Merged
merged 5 commits into from
Oct 10, 2022

Conversation

leoCottret
Copy link
Collaborator

@leoCottret leoCottret commented Oct 2, 2022

Summary

SUMMARY: Content "Add exact match search in debug spawns and fix duplicated debug key"

Purpose of change

fix duplicated debug key

The fix is quite simple, you couldn't use the "S" key to change your stamina through the debug menu because the "Learn all Spells" functionality used the same key

add exact match search in debug spawns

The debug search menu use a "search substring in string" system, which can be annoying when you type the full name of an item and still get the wrong item as first choice.

Describe the solution

fix duplicated debug key

I rebinded some keys so all debug functionalities can be accessed through shortcuts.
image

add exact match search in debug spawns

Most of the time this system is fine. So I added an optionnal way to search for exact match when using double quotes like this -> "example" (very popular system, exist in most web browsers)
Cf example below.
image
By default, searching for 'sledge hammer' would give you 'heavy sledge hammer' by default, or one of the three if you had selected one of the results before.
Well now you can search for "sledge hammer" and be 100% sure to get what you asked.

Testing

  • I tested the new debug keys and the exact match search system with a lot of items

src/ui.cpp Outdated
@@ -222,9 +222,19 @@ void uilist::filterlist()

int f = 0;
int num_entries = entries.size();
// check if string begin by " and finish by ". If that's the case, we only return a result if it matches it exactly
bool exactMatchOnly = filter.front() == '\"' && filter.back() == '\"';
Copy link
Contributor

Choose a reason for hiding this comment

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

What if filter is empty?

There's a check on line 216 that implies the filter string may be empty, so calling filter.front() or filter.back() would result in undefined behavior (i.e. a crash on some systems).

Copy link
Collaborator Author

@leoCottret leoCottret Oct 3, 2022

Choose a reason for hiding this comment

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

If you go see the code of .front() and .back(), they have an internal check on empty strings (which is why I never crashed even when testing those).
Still I added an extra check in case this line is modified later.

Copy link
Contributor

@olanti-p olanti-p Oct 3, 2022

Choose a reason for hiding this comment

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

they have an internal check on empty strings

I'm looking into MSVC's implementation of .front(), and there are checks, except they're skipped in Release build, so essentially you're comparing " with some random byte in the memory, and get a false result 255 times out of 256.

On Debug build, the checks are present and working
image

I ought to omit my own statements regarding preferring Release builds where possible...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah! Then good catch indeed

@RelMayers
Copy link
Contributor

Why those both in single PR?

@scarf005
Copy link
Member

scarf005 commented Oct 3, 2022

I think that's okay, since those two are related (debug) and being only a 16 line edit

@olanti-p olanti-p merged commit ccb219d into cataclysmbnteam:upload Oct 10, 2022
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.

5 participants