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

v2 Fixes #2897. ListView ListWrapper - marking does work if you give an IList which in which the count changes #3510

Merged
merged 15 commits into from
Jun 8, 2024

Conversation

BDisp
Copy link
Collaborator

@BDisp BDisp commented May 28, 2024

Fixes

Proposed Changes/Todos

  • Added CollectionChanged event to the IListDataSource interface
  • Change ListWrapper to use ObservableCollection instead of the IList
  • Change the scenarios to use the changed implementation
  • Change the unit tests to use the changed implementation

Pull Request checklist:

  • I've named my PR in the form of "Fixes #issue. Terse description."
  • My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

@BDisp BDisp requested a review from tig as a code owner May 28, 2024 22:32
@BDisp
Copy link
Collaborator Author

BDisp commented May 28, 2024

To test, you can copy and paste this code into a scenario.

    public override void Main ()
    {
        // Init
        Application.Init ();

        // Setup - Create a top-level application window and configure it.
        Window appWindow = new ()
        {
            Title = $"{Application.QuitKey} to Quit - Scenario: {GetName ()}"
        };

        var count = 0;
        var source = new ObservableCollection<string> ();

        var listView = new ListView
        {
            Width = 20,
            Height = Dim.Fill (),
            AllowsMarking = true,
            AllowsMultipleSelection = true,
            Source = new ListWrapper<string> (source)
        };
        appWindow.Add (listView);

        var btnAdd = new Button
        {
            X = Pos.Center (),
            Y = Pos.Center (),
            Text = "_Add Item"
        };

        btnAdd.Accept += (s, e) =>
                         {
                             source.Add ($"Item{count}");
                             count++;
                             listView.MoveEnd ();
                         };
        appWindow.Add (btnAdd);

        var btnRemove = new Button
        {
            X = Pos.Center (),
            Y = Pos.Center () + 1,
            Text = "_Remove Item"
        };

        btnRemove.Accept += (s, e) =>
                            {
                                if (source.Count > 0 && listView.SelectedItem > -1)
                                {
                                    source.Remove (source [listView.SelectedItem]);
                                }
                            };
        appWindow.Add (btnRemove);

        // Run - Start the application.
        Application.Run (appWindow);
        appWindow.Dispose ();

        // Shutdown - Calling Application.Shutdown is required.
        Application.Shutdown ();
    }

@tig the scrolling in the ListView is broken. The MoveEnd method is now using the code Viewport = Viewport with { Y = _selected }; but the Y does not change.

Copy link
Collaborator

@dodexahedron dodexahedron left a comment

Choose a reason for hiding this comment

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

I like the thinking in this, but...

While ObservableCollection does make things a lot easier for us, it is now restricting users to use that type, specifically, which is non-ideal.

It's a nice thought to try to handle things automatically for the user, here, but they may not even need that behavior.

WPF doesn't force the use of ObservableCollection, but you still can use it, as the consumer, if you want to. But what if it's a huge collection or they just want to hook it up to their ORM of choice like EFCore? Now they have to copy to a new collection before feeding it to us.

For a Terminal.Gui case:
SnapsInAZfs uses views with various different collections, depending on what is appropriate at the time. At least one is an ObservableCollection, but others aren't, or there's even at least one that implements INotifyCollectionChanged itself, using a dictionary underneath, and providing the values to Terminal.Gui for rendering.

Basic point is we shouldn't be forcing users to use things like that which are fundamentally different types, while also not restricting them from doing so, if they want/need to.
Thus, use of IEnumerable<T> or similar is likely to be the best for us to do.

Terminal.Gui/Views/ComboBox.cs Show resolved Hide resolved
Terminal.Gui/Views/ListView.cs Outdated Show resolved Hide resolved
@BDisp
Copy link
Collaborator Author

BDisp commented May 29, 2024

This is the current behavior.

WindowsTerminal_YvE1tHMrhR

Copy link
Collaborator

@dodexahedron dodexahedron left a comment

Choose a reason for hiding this comment

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

Noticed a multiple-subscription issue.

Terminal.Gui/Views/ListView.cs Show resolved Hide resolved
Terminal.Gui/Views/ListView.cs Show resolved Hide resolved
Copy link
Collaborator

@dodexahedron dodexahedron left a comment

Choose a reason for hiding this comment

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

Big performance hit here

Terminal.Gui/Views/ListView.cs Show resolved Hide resolved
@tig tig merged commit da688a3 into gui-cs:v2_develop Jun 8, 2024
1 check passed
@BDisp BDisp deleted the v2_2897-listwrapper-source-fix branch June 8, 2024 21:43
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.

3 participants