Wrong behaviour of DataGridCellInfo Equality operator.

[NotThatBen]

• .NET Core Version: (e.g. 3.0 Preview1, or daily build number, use dotnet --info) - Any
• Windows version: (winver) - Win7
• Does the bug reproduce also in WPF for .NET Framework 4.8?: Yes/No - Yes
• Is this bug related specifically to tooling in Visual Studio (e.g. XAML Designer, Code editing, etc...)? If yes, please file the issue via the instructions here. - No

• Security issues and bugs should be reported privately, learn more via our responsible disclosure guidelines. - Not Related

  Problem description: DataGridCellInfo equality operator returns wrong result when virtualization enabled.

  Actual behavior: Equality operator returns false instead true on the same cell. Disabling RowVirtualization eliminate this issue.

  Expected behavior: Obvious...

  Minimal repro: DataGrid with RowVirtualization enabled and enough items in source collection to run in virtualization. Create DataGridCellInfo object for any cell, keep it, then scroll to hide that cell (virtualize). Then create another CellInfo for any visible cell. Then scroll back to first cell, create CellInfo again and make "=="-ing.

[miloush]

You should provide a simple repro project since there are ambiguities in the steps (such as is the data value or reference types, how do you create cell info - the constructor has two overrides).

That said, the operator returns the correct result. If virtualization is enabled, the item ends in two different instances of DataGridRow, i.e. different cells, hence different cell info.

As you observed, if you want the item to stay in the same cell, disable virtualization.

It sounds to me like DataGridCellInfo is not the correct solution for your purpose. Why do you need it to be equal in this scenario?

[NotThatBen]

Hi!

If virtualization is enabled, the item ends in two different instances of DataGridRow, i.e. different cells, hence different cell info.

It is right for internals of DataGrid control, but if "...is used instead of a reference to the actual DataGridCell container when the DataGrid gets a cell" in outer world, it would be better to be independent from internal mecanisms. Container is a container (hence, subject of virtualization) for internal usage, cell - is a cell, for public.

Also, Equality op Remarks has incorrect description of realisation. Actually, it compares _itemInfo field instead of Item property. IMO, here is the reason.

...the constructor has two overrides

Both have same behaviour.

...disable virtualization.

Too expensive for my purposes.

I was looked for the way to check (i.e. in PreviewMouseLeftButtonDownEvent handler) whether sender cell is the cell i stored. By description, Equality op is exactly what i need, by actual behaviour - not. IMHO, it need to be corrected. Right now, i'm not use DataGridCellInfo in my solution, found another way, but would be nice to have such simple operator to check cells equality.

I still need provide repro?

[miloush]

Well as you just cited, the struct is designed to be a reference to a container, which is a cell. The behaviour seems to be by documented design.

If cell reference was unique you would be able to keep reference to all the cells which is effectively against the virtualization.

You didn't say why you need to check whether sender cell is the cell you stored, but either you want to know whether it's the same item, then check for the item - if it is equal to another item then use a wrapper; or you want to know the actual cell and then it is in fact different cell each time when virtualized, so you get expected results.

I still need provide repro?

Not for me. Repro or code snippet makes it easier for people to address your issue (which is what you want). I had to create a new project and come up with an app that reproduces the issue to verify we are talking about the same thing.

[NotThatBen]

You didn't say why you need to check whether sender cell is the cell you stored,

For my app DataGrid required Drag/Drop rows reordering functional, but i want to keep selection and editing posibilities of DataGrid(DG). i'm not found way to block selection changing on mouseDown in extended mode. Idea is defer all actions DG(actualliy - DataGridCell) performed on MouseLeftButtonDownEvent to perfom them on MouseLeftButtonUpEvent handling or cancel if it needed.

Comparison of cells was part of one of possible scenario of mouseUp handling.

..keep reference to all the cells which is effectively against the virtualization.

I tought purpose of CellInfo not to be reference holder to cell as container, but as way to compare cells as regular table cells, miss/hit. If it'not right i will close this issue.

[miloush]

You can prevent the selection changing for example by handling the PreviewMouseLeftButtonDown event on DataGridCell (using cell style) and if the sender cell is selected (or other logic), mark the event as handled - then it won't get to the selection changing code.

I thought purpose of CellInfo not to be reference holder to cell as container

The documentation you quoted literally says it is used instead of a reference to a container. I do agree however, that the Data​Grid​Cell​Info.​Equals remark is misleading (though technically true). It clearly requires container match (or a special case) to return true as well. I would support a change in the documentation to clarify the virtualizing behavior.

[NotThatBen]

change in the documentation

Think, it will best solution. Should i close this issue?

prevent the selection changing..

Here is my current solution, Code.zip

[miloush]

If you are concerned about reflection and don't need to alter the existing mouse down handler, you can call it yourself by raising the event in the PreviewMouseLeftButtonUp handler:

private void OnDataGridCellPreviewMouseLeftButtonUp(object sender, MouseButtonEventArgs e)
{
    if (sender is DataGridCell cell && cell.IsSelected)
    {
        e = new MouseButtonEventArgs(e.MouseDevice, e.Timestamp, e.ChangedButton, e.StylusDevice);
        e.RoutedEvent = MouseLeftButtonDownEvent;
        cell.RaiseEvent(e);
    }
}

(There is always an option of deriving your class from DataGridCell too.)

Note however, that by moving the selection change code to the mouse up event, you loose the ability to select range of cells by clicking a cell and "dragging" it.

You can leave this issue open and the team will decide whether the documentation should be updated. At this point I don't think the API needs updating for your scenario as there exists a supported mechanism to prevent the selection to change and deferring it to different input events breaks the current interaction paradigm, but I am open to other views.

[NotThatBen]

leave this issue open and the team will decide whether the documentation should be updated.

I agree.

loose the ability to select range of cells

Loosing "dragging" selection started on already selected cell is not a problem. On unselected cells selection starts fine.

raising the event in the PreviewMouseLeftButtonUp handler...

Thank you, it's works. But i prefer to keep control of run into cell editing and focus management, because of specific of my app. i concerned a bit about Reflection mainly due lack of expiriense using it (side effects or something else). Performance impact is not critical here and i can made improvements of currrent code (e.g. move GetType and GetMethod out of handlers).

deriving your class from DataGridCell

I'm trying to avoid this approach. Running into depth of Control's development is not in my current goals.

[miloush]

@NotThatBen I am personally quite liberal regarding reflection as long as you understand that you have no right to complain when the framework decides to change or remove the method without notice. In your case, you shouldn't crash when method will be null, e.g. use method?.Invoke(...) . For performance improvement, you would generally cache the MethodInfo instance in a static member, but you are not in a hot path here.

@ryalanms For the needs-more-info label, please hint what kind of info is missing. It is my understanding that in summary, the proposal is to fix the documentation for DataGridCellInfo.Equals in the sense that the Remarks section includes a note explaining that DataGridCellInfo might not be equal for the same cell when virtualized.

[NotThatBen]

@ryalanms Right, as @miloush said:

the proposal is to fix the documentation for DataGridCellInfo.Equals in the sense that the Remarks section includes a note explaining that DataGridCellInfo might not be equal for the same cell when virtualized.

To clarify a bit - cell here is not "cell as container", but as table cell from human point of view. UPD. The same for DataGridCellInfo.Inequality.

@miloush

you have no right to complain when the framework decides to change or remove method

Hope, with those changes there will be more convenient way for me. IMO, DataGrid need some improvements in their mouse handling. But it's off-topic here. Anyway, thank you!

[SamBent]

@miloush needs-more-info was merely our way of marking the issue as "we've seen it, but we don't have time in this meeting to read the whole thread, understand it, and decide what to do about it". In this case, the more-info needed is the informed opinion of a team member (me). We need a better GitHub label for this (we're actually working on this), so as not to confuse people.

I've changed it to "doc issue" - I agree that the doc needs clarification. Please open an issue at https://github.com/dotnet/dotnet-api-docs, and refer back to this issue. The doc team will need my help to get the right wording, which I'll need to think about.

@NotThatBen et al. DataGridCellInfo, along with many other types, moved from direct item references to ItemInfo about 10 years ago (.NET 4.5) in part to solve problems arising when collections have duplicate items. The documentation didn't keep up - apologies for missing that. The equality test returns false when the items are the same but the containers are different; this is what you want from a human point of view when the same item appears in two different rows. But it means that it will also return false in your virtualizing scenario, as the containers are different.

There's no practical way to return true in your case, because you don't know if the item is the "same" one you saved or merely another copy of that item from a different place in the collection. Figuring that out would defeat virtualization. It's easy if you know the collection has no duplicates - just the actual items for equality. WPF can't do that (we don't know if there are duplicates), but you can.

BTW, DataGridCellInfo is really meant to be a short-lived object, used for tasks that don't scroll the DataGrid. Keeping them around can defeat virtualization, as they hold references to the containers.

[NotThatBen]

@SamBent Thank you for clarification.