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

Better support for notifying cascading calculated properties #857

Open
333fred opened this issue Mar 31, 2024 · 1 comment
Open

Better support for notifying cascading calculated properties #857

333fred opened this issue Mar 31, 2024 · 1 comment
Labels
feature request 📬 A request for new changes to improve functionality mvvm-toolkit 🧰 Issues/PRs for the MVVM Toolkit

Comments

@333fred
Copy link
Contributor

333fred commented Mar 31, 2024

Overview

I have a particularly complex scoring UI that I'm trying to model which has a number of inputs (in the 15-20 input range per side, and there are 2 sides in each match). These inputs then cascade into a dozen outputs from the match, all of which I would like to display in the UI. Further, these outputs often influence each other. As a concrete example:

  • This UI reflects game pieces scored in one of 12 positions.
  • Scoring in one of these affects the total number of points scored by that side, which affects who won the match.
  • Scoring in one can also affect secondary outputs; during qualification rounds, teams can get extra points for doing what I'll call "playing the game extremely well". The number of game pieces scored affects this value.

These calculations are easier to display to the user broken up, rather than just "here's the final value". For example, we display both the number of total game pieces scored across all components, as well as the final score. However, because each of these inputs can have multiple cascading impacts (scoring in a single location impacts total number of items scored, which impacts total number of points), I have to do a lot of NotifyPropertyChangedFor duplication across all of these inputs. I would prefer to annotate the calculated properties with what they themselves will cause to change; then all I have to do is annotate the inputs with the outputs they directly impact, and let secondary impacts be sorted out through a graph walk by the source generator.

API breakdown

No new APIs are necessary. Just add AttributeTargets.Property to NotifyPropertyChangedFor, and update the source generator to understand that it should explore all NotifyPropertyChangedFor properties for cascaded properties.

Usage example

A very simplified example. Note that this sample is missing several inputs to the calculated properties I'm showing.

public class AllianceScore : ObservableObject
{
    [ObservableProperty, NotifyPropertyChangedFor(nameof(TotalSpeakerNotes), nameof(TotalSpeakerPoints))]
    private int speakerNotesAuto;
    [ObservableProperty, NotifyPropertyChangedFor(nameof(TotalSpeakerNotes), nameof(TotalSpeakerPoints))]
    private int speakerNotesTeleop;
    [ObservableProperty, NotifyPropertyChangedFor(nameof(TotalSpeakerNotes), nameof(TotalSpeakerPoints))]
    private int speakerNotesAmplifiedTeleop;

    // This is the new bit. It would cause the generated properties to also notify for MelodyBonusAchieved, so that piece of UI is updated as well as this one.
    [NotifyPropertyChangedFor(nameof(MelodyBonusAchieved))]
    public int TotalSpeakerNotes => SpeakerNotesAuto + SpeakerNotesTeleop + SpeakerNotesAmplifiedTeleop;
    public bool MelodyBonusAchieved => TotalSpeakerNotes >= MELODY_THRESHOLD;

    // Another example of a new bit.
    [NotifyPropertyChangedFor(nameof(TotalPoints))]
    public int TotalSpeakerPoints => SpeakerNotesAuto * SPEAKER_AUTO_POINTS + SpeakerNotesTeleop * SPEAKER_TELEOP_POINTS + SpeakerNotesAmplifiedTeleop * SPEAKER_TELEOP_AMPLIFIED_POINTS;

    public int TotalPoints => TotalSpeakerPoints + /* other point inputs here */
}

Breaking change?

No

Alternatives

Currently, I just have to duplicate these NotifyPropertyChangedFor across all the inputs. It's a lot of duplication, and is very fragile. I don't want to maintain it 🙂.

Additional context

No response

Help us help you

No, just wanted to propose this

@333fred 333fred added the feature request 📬 A request for new changes to improve functionality label Mar 31, 2024
@Sergio0694 Sergio0694 added the mvvm-toolkit 🧰 Issues/PRs for the MVVM Toolkit label Mar 31, 2024
@OJacot-Descombes
Copy link

I like your proposition and would like to suggest an alternative way of annotation that would of course also cascade automatically.

It would probably be easier to maintain the annotations if an attribute could be set on the dependent property because the dependency is directly visible. Also, when changing the expression, you would not have to make changes elsewhere. Something like:

[NotificationDependsOn(nameof(SpeakerNotesAuto), nameof(SpeakerNotesTeleop), nameof(SpeakerNotesAmplifiedTeleop))]
public int TotalSpeakerNotes => SpeakerNotesAuto + SpeakerNotesTeleop + SpeakerNotesAmplifiedTeleop;

In simple cases (when all the properties are directly visible in the expression), the dependencies could also be determined automatically by analyzing the syntax tree:

[NotificationDependsOnAuto]
public int TotalSpeakerNotes => SpeakerNotesAuto + SpeakerNotesTeleop + SpeakerNotesAmplifiedTeleop;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request 📬 A request for new changes to improve functionality mvvm-toolkit 🧰 Issues/PRs for the MVVM Toolkit
Projects
None yet
Development

No branches or pull requests

3 participants