Skip to content

ListView collection navigator not overridable #4027

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

Closed
danielmarbach opened this issue Apr 11, 2025 · 8 comments · Fixed by #4029
Closed

ListView collection navigator not overridable #4027

danielmarbach opened this issue Apr 11, 2025 · 8 comments · Fixed by #4029
Milestone

Comments

@danielmarbach
Copy link

It took as a while to understand that key strokes on a list view are always intercepted and forwarded to the collection navigator. Unfortunately this means when I want to intercept key strokes on a list view in a different way I have no possibility to do so but have to fall back to the application level key down events.

The collection navigator also looses the actual Key information while doing so which means on the search string I can't different a 'b' from a 'B' as an example.

If the collection navigator would be overridable I could at least return one that never gets a matching item by always returning -1 which then would return false on the OnKeyDown and therefore raising the KeyDown event if I understand correctly.

@tznind
Copy link
Collaborator

tznind commented Apr 11, 2025

@YourRobotOverlord
Copy link

I would +1 this as a feature request, for TreeView as well.
I ran into this whilst looking for a way to add icons to ListView items and realized that prepending them to a string broke the built-in list navigation, which works wonderfully.
I also ran into this while trying to implement some custom navigators such as "j4" = down 4 lines

🤔 Implementing this in TG seemed simple at a glance:

CollectionNavigator.cs

- public class CollectionNavigator : CollectionNavigatorBase
+ public class CollectionNavigator : CollectionNavigatorBase, ICollectionNavigator

ICollectionNavigator.cs

+ using System.Collections;
+ 
+ namespace Terminal.Gui;
+ public interface ICollectionNavigator
+ {
+ IList Collection { get; set; }
+ public int GetNextMatchingItem (int currentIndex, char keyStruck);
+ public event EventHandler<KeystrokeNavigatorEventArgs> SearchStringChanged;
+ }

ListView.cs

- public CollectionNavigator KeystrokeNavigator { get; } = new ();
+ public ICollectionNavigator KeystrokeNavigator { get; init; } = new CollectionNavigator();

TreeView.cs

- public CollectionNavigator KeystrokeNavigator { get; } = new ();
+ public ICollectionNavigator KeystrokeNavigator { get; init; } = new CollectionNavigator();

@tznind
Copy link
Collaborator

tznind commented Apr 13, 2025

There is a lot of complexity in collection navigator and different subclasses for each view type that uses the abstract base.

The most common things to want to change are probably these two methods:

public static bool IsCompatibleKey (Key a)
private bool IsMatch (string search, object value)

Easiest way to give user control of those could be to make a sub property e.g. ISearchStrategy (Or similar) that has only these two methods.

Then code becomes

myView.CollectionNavigator.SearchStrategy = new MyStrategy()

@tznind
Copy link
Collaborator

tznind commented Apr 15, 2025

PR is ready for review, guys please feel free to check it out and let me know if it does not go far enough for your needs.

If the collection navigator would be overridable I could at least return one that never gets a matching item by always returning -1 which then would return false on the OnKeyDown and therefore raising the KeyDown event if I understand correctly.

You can now do this with:

class NeverMatcher : ICollectionNavigatorMatcher
{
    /// <inheritdoc />
    public bool IsCompatibleKey (Key a) { return false; }

    /// <inheritdoc />
    public bool IsMatch (string search, object value) { throw new NotSupportedException (); }
}

[...]

ListView lv = new ListView { Source = new ListWrapper<string> (source) };
lv.KeystrokeNavigator.Matcher = new NeverMatcher ();

Unfortunately this means when I want to intercept key strokes on a list view in a different way I have no possibility to do so

There is a seperate issue which is that we want to prioritise user key down event handlers over keystroke navigation, but that is best handled seperately and on a 'whole repo' scope (see #3950)

@tig
Copy link
Collaborator

tig commented Apr 16, 2025

Here's an updated test that ensures the entire keydown flow is processed correctly:

    [Fact]
    public void CollectionNavigatorMatcher_KeybindingsOverrideNavigator ()
    {
        Application.Top = new ();

        ObservableCollection<string> source = new () { "apricot", "arm", "bat", "batman", "bates hotel", "candle" };
        ListView lv = new ListView { Source = new ListWrapper<string> (source) };

        Application.Top.Add (lv);
        lv.SetFocus ();

        lv.KeyBindings.Add (Key.B, Command.Down);

        Assert.Equal (-1, lv.SelectedItem);

        // Keys should be consumed to move down the navigation i.e. to apricot
        Assert.True (Application.RaiseKeyDownEvent (Key.B));
        Assert.Equal (0, lv.SelectedItem);

        Assert.True (Application.RaiseKeyDownEvent (Key.B));
        Assert.Equal (1, lv.SelectedItem);

        // There is no keybinding for Key.C so it hits collection navigator i.e. we jump to candle
        Assert.True (Application.RaiseKeyDownEvent (Key.C));
        Assert.Equal (5, lv.SelectedItem);

        Application.Top.Dispose ();
        Application.ResetState ();
    }

This test fails currently.

Here's the fix to ListView:

    /// <inheritdoc/>
    protected override bool OnKeyDown (Key key)
    {
        // If marking is enabled and the user presses the space key don't let CollectionNavigator
        // at it
        if (AllowsMarking)
        {
            IEnumerable<Key> keys = KeyBindings.GetAllFromCommands (Command.Select);

            if (keys.Contains (key))
            {
                return false;
            }

            keys = KeyBindings.GetAllFromCommands ([Command.Select, Command.Down]);

            if (keys.Contains (key))
            {
                return false;
            }

        }

+        // If the key was bound to a command, let normal KeyDown processing happen. This enables overriding the default handling.
+       // See: https://github.com/gui-cs/Terminal.Gui/issues/3950#issuecomment-2807350939
+       if (KeyBindings.TryGet (key, out _))
+       {
+            return false;
+       }
+
        // Enable user to find & select an item by typing text
        if (CollectionNavigatorBase.IsCompatibleKey (key))
        {
            int? newItem = KeystrokeNavigator?.GetNextMatchingItem (SelectedItem, (char)key);

            if (newItem is { } && newItem != -1)
            {
                SelectedItem = (int)newItem;
                EnsureSelectedItemVisible ();
                SetNeedsDraw ();

                return true;
            }
        }

        return false;
    }

@tznind
Copy link
Collaborator

tznind commented Apr 17, 2025

Here's the fix to ListView

Thanks, I have applied

@tznind
Copy link
Collaborator

tznind commented Apr 17, 2025

@tig thanks for this fix, I have also applied it to TableView and TreeView along with relevant tests - in #4029

@danielmarbach
Copy link
Author

@tznind wow that was quick 😍 Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

4 participants