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

SelectionPrompt does not search on results of UseConverter #1626

Closed
snus-kin opened this issue Sep 2, 2024 · 7 comments · Fixed by #1627
Closed

SelectionPrompt does not search on results of UseConverter #1626

snus-kin opened this issue Sep 2, 2024 · 7 comments · Fixed by #1627
Labels
bug Something isn't working needs triage
Milestone

Comments

@snus-kin
Copy link

snus-kin commented Sep 2, 2024

Information

  • OS: MacOS (also tested on Windows)
  • Version: 0.49.1
  • Terminal: IntelliJ Rider Built-in Terminal (using Bash)

Describe the bug
Selection prompts which use UseConverter do not jump to the search (they do not use the mapped text as the search index).

To Reproduce

  • Create a selection prompt
  • Use .UseConverter()
  • Search
  • Observe that the result you have searched for is not jumped to

You can also search for the underlying object and observe that it is jumped to.

Example unit test that currently fails (where MyClass is just a class with a name parameter):

    [Fact]
    public void Should_Search_In_Remapped_Result()
    {

        // Given
        var console = new TestConsole();
        console.Profile.Capabilities.Interactive = true;
        console.EmitAnsiSequences();
        console.Input.PushText("2");
        console.Input.PushKey(ConsoleKey.Enter);
        var myObject = new List<MyClass>{ new() { name = "Item 1" }, new() {name = "Item 2"} };

        // When
        var prompt = new SelectionPrompt<MyClass>()
            .Title("Select one")
            .EnableSearch()
            .UseConverter(o => o.name)
            .AddChoices(myObject);
        var selection = prompt.Show(console);

        // Then
        selection.name.ShouldBe("Item 2");
    }

Expected behavior
SelectionPrompt should search based on the results of UseConverter and not on the underlying object remapped by it. (see test)

Screenshots
If applicable, add screenshots to help explain your problem.

Additional context
Add any other context about the problem here.


Please upvote 👍 this issue if you are interested in it.

@snus-kin snus-kin added bug Something isn't working needs triage labels Sep 2, 2024
@github-project-automation github-project-automation bot moved this to Todo 🕑 in Spectre Console Sep 2, 2024
@snus-kin
Copy link
Author

snus-kin commented Sep 2, 2024

Some testing, this test works:

    [Fact]
    public void Should_Search_In_Remapped_Result()
    {

        // Given
        var console = new TestConsole();
        console.Profile.Capabilities.Interactive = true;
        console.EmitAnsiSequences();
        console.Input.PushText("2");
        console.Input.PushKey(ConsoleKey.Enter);

        var myObject = new List<Tuple<string, string>>
        {
            new ( "Value", "Item 1" ),
            new ( "Value", "Item 2" )
        };

        // When
        var prompt = new SelectionPrompt<Tuple<string, string>>()
            .Title("Select one")
            .EnableSearch()
            .UseConverter(o => o.Item2)
            .AddChoices(myObject);
        var selection = prompt.Show(console);

        // Then
        selection.Item2.ShouldBe("Item 2");
    }

@snus-kin
Copy link
Author

snus-kin commented Sep 2, 2024

Potential fix here: 93e3689

Not sure if there's a better way of doing this, tried to refactor so the function is only called once per item. Will wait for a reply on this issue before creating a PR or anything.

@patriksvensson
Copy link
Contributor

Thanks for attempting to fix this, but we don't want to change the API. The UseConverter is used to get a representation, so we do not want to add an explicit representation when adding the item (optional or not) since this is what the converter is for. I will look into what is required to fix the search.

patriksvensson added a commit to patriksvensson/spectre.console that referenced this issue Sep 2, 2024
patriksvensson added a commit to patriksvensson/spectre.console that referenced this issue Sep 2, 2024
@patriksvensson
Copy link
Contributor

@snus-kin I've submitted a PR that should fix this. Will merge shortly

@github-project-automation github-project-automation bot moved this from Todo 🕑 to Done 🚀 in Spectre Console Sep 2, 2024
@patriksvensson patriksvensson added this to the 0.50 milestone Sep 2, 2024
@patriksvensson
Copy link
Contributor

@snus-kin This is fixed and is now available in 0.49.2-preview.0.26

@snus-kin
Copy link
Author

snus-kin commented Sep 3, 2024

Thanks pal, appreciate it

@snus-kin
Copy link
Author

snus-kin commented Sep 3, 2024

Tested this on my use-case today and it works perfectly, thanks for the prompt fix 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs triage
Projects
Status: Done 🚀
Development

Successfully merging a pull request may close this issue.

2 participants