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

Expose classes and utility is DryWetMidi.Multimedia #301

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

noonesimg
Copy link

Changed visibility of several properties, classes and methods to public
To allow for easier custom playback implementations

@melanchall
Copy link
Owner

Hi!

Thanks for the PR. Unfortunately I can't accept it right now because I don't see reasons to open all those classes and members.

A couple of examples:

public TimedEvent TimedNoteOnEvent
public TimedEvent TimedNoteOffEvent

Why? There are methods GetTimedNoteOnEvent and GetTimedNoteOffEvent. This is a protection from changing a note in an unexpected way.

public static IEnumerable<TimedObjectAt<ITimedObject>> GetNotesAndTimedEventsLazy

Please read Getting objects article to learn how to get objects of different types.

About Playback: without examples of usage I can't understand why you need to make those classes and members public.

I as the developer of the library must always think about common cases and API clarity. Making classes public without obvious profit just adds noise to API, set of different classes become bigger and user can be confused.

@noonesimg
Copy link
Author

noonesimg commented Jul 23, 2024

Okay, these two are definitely a mistake

public TimedEvent TimedNoteOnEvent
public TimedEvent TimedNoteOffEvent

I didn't realize those get methods exist.

Speaking of GetNotesAndTimedEventsLazy
As far as I understand, there's now other easy way to get a nice collection of PlaybackEvents without rewriting a lot of code, that's already there :)

this is from Playback.cs

var noteDetectionSettings = playbackSettings.NoteDetectionSettings ?? new NoteDetectionSettings();
_playbackEvents = GetPlaybackEvents(timedObjects.GetNotesAndTimedEventsLazy(noteDetectionSettings, true), tempoMap);
MoveToNextPlaybackEvent();

Speaking of PlaybackEvents, those have a really useful PlaybackEventsComparer which is also internal and sealed.
I was just trying to be as close to the source Playback class as possible.

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.

2 participants