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

duplicated code in learn.py #29

Open
IARI opened this issue Jul 21, 2015 · 6 comments
Open

duplicated code in learn.py #29

IARI opened this issue Jul 21, 2015 · 6 comments

Comments

@IARI
Copy link
Contributor

IARI commented Jul 21, 2015

look at the processNote method - why is it that way?

@IARI IARI changed the title lots fo duplicated code in learn.py duplicated code in learn.py Jul 21, 2015
@Vampouille
Copy link
Owner

I think it's more readable this way. But I agree with you, this is not good... There is also a bug with knowCtrl and knownBtn list : When you overwrite some mapping, old values are not removed from lists of used btn an ctrl. So you cannot switch two controls (for example play and pause).
Those lines :

self.knownCtrl.add(ctrl_key)
self.knownBtn.add(btn_key)

are not present in every if part. Maybe we can replace string in self.send_midi_to by a callable function. This way, we can create separate little functions that will handle midi event.

@IARI
Copy link
Contributor Author

IARI commented Jul 25, 2015

That's a good idea - i'll put it in our fork :)

@IARI
Copy link
Contributor Author

IARI commented Jul 25, 2015

Actually I in gui.py there is a similar situation in processNote:
There is a big if construct to decode what function to use for what message.
i would consistently replace it with a mapping from midi-messages (or your abstract representations of them) directly to functions. Would you agree?

@Vampouille
Copy link
Owner

yes, but can you make this modification in a new branch created from last tag 1.2.0 ?

@IARI
Copy link
Contributor Author

IARI commented Jul 27, 2015

Damn i have already started - To be honest i have rewritten the whole device and most of learn.py.
in the main branch of our fork..but I will try to put it in a new branch!

@IARI
Copy link
Contributor Author

IARI commented Aug 3, 2015

Ok, brace yourself @Vampouille, here it is: 660dd95

I'll clean it up a bit more, then we will do some more extensive testing - and then I'll issue a pull request.
Its much but I hope you like it ;)
I had to add another dependency: the wrapt package. Tried to get around without it, but vanilla python decorators aren't that nice with objects. I hope that's not a huge issue?!

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

No branches or pull requests

2 participants