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

Fix TreeDataGridTextCell changing TextCell.Value #304

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

erri120
Copy link

@erri120 erri120 commented Sep 4, 2024

TreeDataGridTextCell listens to property changes of ITextCell.Value to update its own Value property. The setter of TreeDataGridTextCell.Value updates ITextCell.Text to the string representation of the new value.

This is done because the cell can be edited, and
TreeDataGridTextCell.Value is bound to TextBox.Text two-way when the user wants to edit the cell.

However, TextCell<T>.Text which the setter of TreeDataGridTextCell.Value will always set, doesn't respect the read-only status or the editing status of the cell:

  1. TextCell<T>.Value updates.
  2. TreeDataGridTextCell reacts and sets TreeDataGridTextCell.Value
    to the string representation of the new value.
  3. The setter of TreeDataGridTextCell.Value will set TextCell<T>.Text
    to the string representation.
  4. The setter of TextCell<T>.Text will try to convert the text
    representation of the new value as the new value...

If it weren't for the fact that RaiseAndSetIfChanged does what it's supposed to do, which is only raise if changed, this would be a loop that goes on forever.

Users will get an exception if T of TextCell<T> fails to convert: Convert.ChangeType(value, typeof(T)). This obviously isn't an issue when T is string but it does have issues for base types like numbers (localization issues), and custom types will almost always result in an exception.

@erri120
Copy link
Author

erri120 commented Sep 4, 2024

The Modified_Text_Is_Written_To_Binding test is failing because it does

target.Text = "new";

without using BeginEdit. I'm fine with removing the !_isEditing check from the TextCell<T>.Text setter to prevent API breakage.

erri120 added a commit to erri120/NexusMods.App that referenced this pull request Sep 4, 2024
`TreeDataGridTextCell` listens to property changes of `ITextCell.Value`
to update its own `Value` property. The setter of `TreeDataGridTextCell.Value`
updates `ITextCell.Text` to the string representation of the new value.

This is done because the cell can be edited, and
`TreeDataGridTextCell.Value` is bound to `TextBox.Text` two-way when the
user wants to edit the cell.

However, `TextCell<T>.Text` which the setter of `TreeDataGridTextCell.Value`
will always set, doesn't respect the read-only status or the editing
status of the cell:

1) `TextCell<T>.Value` updates.
2) `TreeDataGridTextCell` reacts and sets `TreeDataGridTextCell.Value`
    to the string representation of the new value.
3) The setter of `TreeDataGridTextCell.Value` will set `TextCell<T>.Text`
    to the string representation.
4) The setter of `TextCell<T>.Text` will try to convert the text
   representation of the new value as the new value...

If it weren't for the fact that `RaiseAndSetIfChanged` does what it's
supposed to do, which is only raise if changed, this would be a loop
that goes on forever.

Users will get an exception if `T` of `TextCell<T>` fails to convert:
`Convert.ChangeType(value, typeof(T))`. This obviously isn't an issue
when `T` is `string` but it does have issues for base types like numbers
(localization issues), and custom types will almost always result in an
exception.
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.

1 participant