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

Prototype: Eliminate TreeNodeVM #1055

Merged
merged 4 commits into from
Mar 21, 2024
Merged

Prototype: Eliminate TreeNodeVM #1055

merged 4 commits into from
Mar 21, 2024

Conversation

Sewer56
Copy link
Member

@Sewer56 Sewer56 commented Mar 13, 2024


Checks off Consider unifying TreeNodeVM with the underlying flat item view model. in
#1048

Successor to:

Need to merge previous PR if you want a slightly cleaner diff against main, as this PR builds upon it.
(Or just look at the new commits since that PR in isolation)


This is a prototype which shows an implementation of inlining what was formerly TreeNodeVM directly into the ViewModels, for cleanliness. This PR adds the new system for doing so via IDynamicDataTreeItem<TItem, TKey>, and implements it for the View Mod Files view.

This PR is purely for feedback/iteration on the idea we had. i.e. a working prototype.
For @Al12rs and anyone else interested.

Notable Differences:

DynamicData tree creation changes from:

cache.Connect()
    .TransformToTree(model => model.ParentKey)
    .Transform(node => new ModFileNode(node))
    .Bind(out result)
    .Subscribe(); // force evaluation

To

cache.Connect()
    .TransformToTree(model => model.ParentKey)
    .Transform(node => node.Item.Initialize(node))
    .Bind(out result)
    .Subscribe(); // force evaluation

ViewModels that inherit from IDynamicDataTreeItem must expose 2 more properties.

// Example: IFileTreeNodeViewModel
public ReadOnlyObservableCollection<IFileTreeNodeViewModel>? Children { get; set; }
public IFileTreeNodeViewModel? Parent { get; set; }

Aside from that, no difference in usability from consumer end. The reactiveness works just a-ok, we can access children, access parents. etc.

The TreeNodeVM wrapper is no longer needed, the hack of re-broadcasting PropertyChanged from inner Item is no longer needed, and code that interacts with the ViewModel no longer needs to have .Item all over the place. This saves on the amount of code we have to write.

Logically speaking, there's no difference in how things operate behind the scenes. The difference is, Node<TObject,TKey> now holds a transitive reference to TItem instead of TreeNodeVM<TItem, TKey> via the .Children.Connect() subscription.

@Sewer56 Sewer56 requested a review from Al12rs March 13, 2024 08:18
@Sewer56 Sewer56 self-assigned this Mar 13, 2024
@Sewer56 Sewer56 marked this pull request as ready for review March 20, 2024 17:06
@Al12rs Al12rs merged commit debaf50 into main Mar 21, 2024
7 of 8 checks passed
@Al12rs Al12rs deleted the eliminate-treenodevm branch March 21, 2024 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants