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

Make CurrentPlaylistItem non-bindable #30592

Closed

Conversation

smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented Nov 12, 2024

Prereqs:

This is the first bindable in the chain that is really ugly and prone to breakage. It changes meaning depending on the context - while inside a RoomSubScreen its value comes from RoomSubScreen.SelectedItem, but outside it comes from Room.CurrentPlaylistItem (the API source). See OnlinePlayComposite for how this is done.

I'm trying something new here which seems to work pretty well - replacing with implementations of the form:

public required Bindable<PlaylistItem?> SelectedItem
{
    get => selectedItem;
    set => selectedItem.Current = value;
}

private readonly BindableWithCurrent<PlaylistItem?> selectedItem = new BindableWithCurrent<PlaylistItem?>();

The required attribute means implementations are explicit about needing this to be set. I'm not sure if I'll/probably won't do this for every other property in OnlinePlayComposite, but it especially makes sense here since this governs a core part of multiplayer.

Another notable change is DrawableRoom. When implemented as DrawableMatchRoom, it takes its item from the subscreen. When implemented as DrawableLoungeRoom, it takes its item from the Room. This was previously done in the base DrawableRoom implementation, but now the base class exposes a protected Bindable<PlaylistItem?>, which is implemented in each case depending on the relevant data source.
I'm not sure if this is the best implementation for this, because it could just as easily use a property or a method, or maybe another way. If there's any opinions, I'd love to hear them.

As for Room.PropertyChanged... The two methods I've added are what Rider suggests when implementing the INotifyPropertyChanged interface and seem good to me. I'm not sure if this will stay around or if it will keep its current form, but I'm just simplifying things first with an ultimate goal of removing CachedModelDependencyContainer and OnlinePlaySubscreen, and then will figure out where this sort of signalling should go - may be nice to have a global source like MultiplayerClient events instead of inside the model but not sure yet.

@smoogipoo smoogipoo mentioned this pull request Nov 13, 2024
1 task
@smoogipoo
Copy link
Contributor Author

smoogipoo commented Nov 14, 2024

If you're interested in seeing how the final product looks like, take a peek at https://github.com/smoogipoo/osu/tree/multiplayer-remove-cmc-and-composite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants