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

Implements MIKMIDIConnectionManager (Issue #106) #115

Merged
merged 27 commits into from
Nov 14, 2015
Merged

Implements MIKMIDIConnectionManager (Issue #106) #115

merged 27 commits into from
Nov 14, 2015

Conversation

armadsen
Copy link
Member

@cflesner and @johndpope I'd very much appreciate your feedback on this before I merge it. The major relevant API is in https://github.com/mixedinkey-opensource/MIKMIDI/blob/Issue106/Source/MIKMIDIConnectionManager.h

Implements Issue #106.

Andrew Madsen added 19 commits November 5, 2015 10:13
…ered the saved configuration during loading.
…n disconnection to MIKMIDIConnectionManager.
Andrew Madsen added 2 commits November 11, 2015 14:34
… disconnected upon calling -[MIKMIDIConfigurationManager loadConfiguration].
@johndpope
Copy link

I love your work. Will test this out over weekend. Thanks

@kris2point0
Copy link
Contributor

It looks great to me. The only thing I don't necessarily like is that the saved configurations rely on the class name not ever changing. I think simply exposing -userDefaultsConfigurationKey for subclasses to be able to override would be sufficient for resolving that though.

@armadsen
Copy link
Member Author

Nice catch, Chris. Perhaps, I'll use [MIKMIDIConnectionManager class]. I don't want subclassing to break obvious things, but then again, I'm not sure I really designed MIKMIDIConnectionManager with the explicit intention of encouraging subclasses, and name already lets you customize userDefaultsConfigurationKey.

Actually, I should have read my own code ;). -userDefaultsConfigurationKey only depends on [self class] if name is not set (or empty), which is considered invalid, really. name is a nonnull argument to the designated initializer. So I think I'm just going to leave it as-is for now. I did add a warning that name can't be nil/empty to the documentation.

…ager initWithName:delegate:eventHandler:].
@kris2point0
Copy link
Contributor

Okay, I missed that too :)

That was my only concern, and since that was a non-issue, I'd say I give the pull request a 👍🏻

armadsen added a commit that referenced this pull request Nov 14, 2015
Implements MIKMIDIConnectionManager (Issue #106)
@armadsen armadsen merged commit 424fa42 into 1.1 Nov 14, 2015
@armadsen armadsen deleted the Issue106 branch November 14, 2015 18:28
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.

3 participants