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

[WIP] Add ListViewGroup items to ListView when adding group #4000

Closed
wants to merge 1 commit into from

Conversation

hughbe
Copy link
Contributor

@hughbe hughbe commented Sep 24, 2020

Fixes #3987

WIP as I need to do more testing and check some things

Proposed changes

  • Add ListViewGroup items to ListView when adding group

Customer Impact

  • Items that were in the ListViewGroup but not added to the ListView were not added, but now they are

Regression?

  • No

Risk

  • Performance? People relying on the incorrect behaviour
Microsoft Reviewers: Open in CodeFlow

@hughbe hughbe requested a review from a team as a code owner September 24, 2020 16:08
@ghost ghost assigned hughbe Sep 24, 2020
@RussKie RussKie marked this pull request as draft September 25, 2020 02:28
@RussKie RussKie added the test-enhancement Improvements of test source code label Sep 25, 2020
@RussKie
Copy link
Member

RussKie commented Sep 25, 2020

Will this fix #3439?

@hughbe hughbe force-pushed the ListViewGroup-fixes branch 2 times, most recently from b7f0f22 to 3b07c6b Compare September 25, 2020 10:19
@@ -129,8 +129,6 @@ public void ListViewAccessibleObject_ListWithTwoGroups_FragmentNavigateWorkCorre
item2.Group = group;
item.Group = group;
listView.Groups.Add(group);
listView.Items.Add(item);
listView.Items.Add(item2);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

only problem with the fix is that this now throws ArgumentException as listView.Items already contains item and item2 as they were added in the call to listView.Groups.Add(group)

I'm not sure of the best solution as this may be a significant breaking change?

Copy link
Member

Choose a reason for hiding this comment

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

Thinking out loud here.

From a user perspective the following use would likely look logical and expected:

  • If an item added to a group, and then the group is added to the listview - all items should have a link to the listview.
  • If the group is removed from the listview - all items in the group should no longer have links to the listview.
  • If the group (with items) is added to the listview, and then the same item is added to the listview - then the operation is no-op.
  • If an item is added to the listview, and the added to the group - no-op, the item is already linked to the listview.
  • If an item is added to the group, which is bound to the listivew - the item linked to the listview.
  • If an item is removed from the group, which is bound to the listview (i.e. item.Group = null), it remains in the listview.
  • If an item is removed from the group, which is not bound to the listview (i.e. item.Group = null), the item must not have a link to the listview. (We should never be in this situation).

I don't have a formed opinion about the following use case though:

  • If an item that has a link to listivew1 is then added to listview2 - then it is
    • an IOE? or
    • silent re-assign the link to listview2?

@merriemcgaw @Tanya-Solyanik @JeremyKuhne thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

We might want to consider overloading the API to allow people to opt into what we think is the "better" behavior. So keep the existing behavior without the option...

Copy link
Member

Choose a reason for hiding this comment

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

I think @JeremyKuhne has a great point. We've had the 'bad' behavior long enough that I think people have worked around it and we don't want to break them by default.

@hughbe hughbe force-pushed the ListViewGroup-fixes branch from 3b07c6b to 9865e5e Compare September 25, 2020 12:30
Base automatically changed from master to main February 1, 2021 04:50
@dreddy-work
Copy link
Member

@hughbe, checking back to see if you get some time to take this to finish line.

@dreddy-work
Copy link
Member

@hughbe, checking back to see if you get some time to take this to finish line.

@hughbe welcome back to winforms repo. like to get this to your attention.

@ghost ghost added the draft draft PR label Sep 11, 2021
@ghost ghost closed this Sep 30, 2023
@ghost
Copy link

ghost commented Sep 30, 2023

The current status of this "draft" PR has persisted for over 180 days, making it highly probable that it is no longer aligned with the latest codebase. Our repository is set up to automatically close draft PRs that have become outdated, and it requests the author to revisit and reopen them if they deem it necessary, thereby bringing them to the team's attention.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 30, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
draft draft PR test-enhancement Improvements of test source code
Projects
None yet
5 participants