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.

Sign up for GitHub

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

Jump to bottom

Updating ListView.ListViewItem.ListViewSubItem Text or Name values updates them all, why? is this normal? #10963

Closed
creizlein opened this issue Feb 28, 2024 · 11 comments · Fixed by #11401
Closed

Updating ListView.ListViewItem.ListViewSubItem Text or Name values updates them all, why? is this normal? #10963

creizlein opened this issue Feb 28, 2024 · 11 comments · Fixed by #11401
Labels
area-Controls-ListView tenet-performance Improve performance, flag performance regressions across core releases
Milestone
Help wanted

Comments

@creizlein
Copy link

.NET version

Tested and Checked on .NET Runtime 7.0 and 8.0

Did it work in .NET Framework?

Not tested/verified

Did it work in any of the earlier releases of .NET Core or .NET 5+?

n/a.

Issue description

Talking about WinForms, ListView Control, ListViewItem and ListViewSubItems

when you have a ListViewItem , lets say, with 10 ListViewSubItems, on a Details view, and you change the text, it triggers a update text on ALL of the subitems.

If you check on the Text Setter, it reads as follows:

ListViewSubItem class

        public string Text {
            get => text ?? string.Empty;
            set {
                text = value;
                _owner?.UpdateSubItems(-1);
            }
        }

_owner being the ListViewItem, so if we go there, we find this:

internal void UpdateSubItems(int index) => UpdateSubItems(index, SubItemCount);
internal void UpdateSubItems(int index, int oldCount) {
    // trim down, not modified, just formated.
    if (_listView is not null && _listView.IsHandleCreated) {
        int subItemCount = SubItemCount;
        int itemIndex = Index;

        if (index != -1) {
            _listView.SetItemText(itemIndex, index, _subItems[index].Text);
        } else {
            for (int i = 0; i < subItemCount; i++) {
                _listView.SetItemText(itemIndex, i, _subItems[i].Text);
            }
        }

        for (int i = subItemCount; i < oldCount; i++) {
            _listView.SetItemText(itemIndex, i, string.Empty);
        }
    }
}

the ListView itself is then the one in charge of triggering the PInvoke.SendMessage with each of the subitems actual text.

So my question is, why does this happen? and even worst, why does changing the NAME of the item also triggers an update on the text?

Seems a little bit weird and overwhelming, but then again I am just checking and making sure this is not a problem..

I have found some laggy and performance issues and found myself that the ListView was killing the UI Thread with message loops because I am updating a text on a subitem on column 20 , of course, it then updates all 30 of them which are all the columns I have.

Steps to reproduce

Create a WinForms app with a ListView on it, add a couple of ListViewItems with several subitems, and try to change the text of Nth subitem only and check how many of them get triggered to update.
@creizlein creizlein added the untriaged The team needs to look at this issue in the next triage label Feb 28, 2024
@creizlein
Copy link
Author

I believe the problem comes from the fact that the ListViewSubItem calls UpdateSubItems(-1) , with -1 of course, but then the ListViewItem itself does not correct that and calls the ListView to only update that specific item with the index of itself.

@elachlan elachlan added area-Controls-ListView tenet-performance Improve performance, flag performance regressions across core releases labels Feb 28, 2024
@elachlan
Copy link
Contributor

@creizlein I'd suggest you look into the code history and see. Generally small mistakes like this happen in a large code base. I imagine the team would be happy to review a PR if you submitted one.

@elachlan
Copy link
Contributor

Maybe something like:

[Localizable(true)]
[AllowNull]
public string Text
{
    get => text ?? string.Empty;
    set
    {
        text = value;
        _owner?.UpdateSubItems(_owner.SubItems.IndexOf(this));
    }
}

[Localizable(true)]
[AllowNull]
public string Name
{
    get => name ?? string.Empty;
    set
    {
        name = value;
        _owner?.UpdateSubItems(_owner.SubItems.IndexOf(this));
    }
}

@creizlein
Copy link
Author

Idk I am up to the task, but I will try to find all the possibilities and submit something.
I still don't understand why it updates the TEXT when you change the NAME, I believe it has something to do for when there is no text at all, it takes the name and put it as text, but if that's the case then it should IF it at least.

Also I an wondering why there is no check on if the name/text is changed first before sending the messages, like if (name!=value) just return. I will put together a PR for it.

Was first wondering if it was on purpose at all, maybe there is a solid reason behind updating them all.

@elachlan
Copy link
Contributor

I'd just try it and see. Same with the name update triggering the change. I figure its probably got something to do with the designer. There aren't much references to the Name though.

@paul1956
Copy link
Contributor

paul1956 commented Feb 28, 2024 via email

@merriemcgaw
Copy link
Member

@paul1956 's theory makes a lot of sense. As @elachlan said, we would be happy to review a PR should you submit one. Given that this would be an actual behavior change, we would need to put it behind a quirk so that it doesn't impact existing applications.

@merriemcgaw merriemcgaw removed the untriaged The team needs to look at this issue in the next triage label Feb 28, 2024
@merriemcgaw merriemcgaw added this to the Help wanted milestone Feb 28, 2024
@elachlan
Copy link
Contributor

@merriemcgaw, would you accept a PR without the quirk if the behavior is only changed at Text and not Name?

@merriemcgaw
Copy link
Member

@elachlan I think we would actually, thank you!

elachlan added a commit to elachlan/winforms that referenced this issue May 19, 2024
@elachlan
Fixes dotnet#10963 Change ListViewSubItem.Text to pass index to ListV…
cb65f53 
…iewItem.UpdateSubItems to avoid loop
@elachlan elachlan mentioned this issue May 19, 2024
Merged
@elachlan
Copy link
Contributor

@creizlein I have a pr up if you would like to review it. We can't seem to reproduce your performance issue, but it should remove one of the loops.

LeafShi1 pushed a commit that referenced this issue May 22, 2024
@elachlan
Improve ListViewSubItem.Text performance (#11401)
af38079 
* Fixes #10963 Change ListViewSubItem.Text to pass index to ListViewItem.UpdateSubItems to avoid loop

* Reduce Nesting and add comments to make clear what is happening in ListViewItem.UpdateSubItems
@creizlein
Copy link
Author

Sorry I am late to the party, i was some days out of town and was also watching @merriemcgaw presentation today, I would love to give this a try, let me setup my dev platform so i can run this last code and provide some feedback hopefully this week.

Btw, great presentation @merriemcgaw , kudos !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-Controls-ListView tenet-performance Improve performance, flag performance regressions across core releases
Projects
None yet
Milestone
Help wanted
Development

Successfully merging a pull request may close this issue.

Improve ListViewSubItem.Text performance elachlan/winforms
4 participants
@elachlan @paul1956 @merriemcgaw @creizlein