Holiday Hacking 1: Architecture: Flatten key press handling #83
Conversation
- Rely on default init values - Deduplicate range check
The hub itself shouldn't implement its own listener-interface, to avoid infititely-recursive triggers, so un-implement PianoListener, and rename the 'onKey*'-methods to 'doKey*' to signal their importance.
…ic helpers. The old `SOUNDSET_DIR_PREFIX` already indicated in its name it wanted to live in a SoundSet class. Additionally, it's not just for DIRs, since it's also (ab?)used to get at the translation keys. As such, move the prefix to SoundSet, and isntead use SoundSet helpers everywhere the prefix is currently used. Also, the SoundSet class, which is already handling the raw folder access, is a far better candidate for scanning that folder, than the current SettingsFragment.
An interface for something with only one sane implementation (SampledSoundSet) might seem like overkill, but this allows us to write tests without adding a dependency on a mocking framework, which would be worse overkill.
…istener. Move the last context-awareness out of piano, by converting `play_sound()`, including all its supporting infrastructure, to external PianoListener. Rather than one single, complex Melody-Preference-checking Listener, use a strategy pattern with two simple listeners: - one for 'normal' behaviour: StraightKeySoundMaker - one for melodies: MelodicKeySoundMaker Choose which one to listen with only once during piano-init, rather than checking melody-not-null on every key-press.
It was getting cluttered in the main package.
juleskers
changed the title
|
Awesome. I've been putting this off as I worked on separate projects over the past several weeks, but over the next few days I'll review these changes. I'm looking forward to this one, because I am keen to have the preferences screen "Theme chooser" actually render a small piano for each theme. However when I looked at the rendering code, I also noticed the Piano / PianoCanvas / AppConfigTrigger issue which makes it hard to extract the rendering code. I then noticed the same thing when trying to resolve the "Touching the black key anywhere triggers settings" which you've eluded to elsewhere. Hopefully this tidies it all up a little. Might take a few nights to review though. Happy new year! |
|
Happy new year! Take all the time you need. It took me many evenings to put together, so it should be normal that it takes multiple to review too. 💙 |
|
On device test works well. Merging. Thanks! |
Hi @nicolasbrailo , hi @pserwylo ,
I hope your lead-up to the holiday season is going well!
I've scraped some free time together to play more with PianOli in my own holiday prep 👼
This is a big architecture decoupling: disentangling the key-press handling into some semblance of "architecture", via a new
PianoListenerinterface.The new interface is implemented by everything that responds to key-presses, which includes the 'old'
PianoCanvasandAppConfigTrigger, as well as newly-extracted stuff (see below)This turns the up-and-down calling from
PianoCanvas->Piano->PianoCanvas->AppConfigTriggerinto one 'flat' listener-loop inPiano, with some new tests that guarantee thatPianocalls its listeners.Given that new
Listener-infrastructure, I could finally realize my long-held wish of riddingPianoof all Android-Context.It's now the pure "geometry-only" thing I envisioned.
I've achieved this by disentangling the note-playing from the melody-handling (see the
KeySoundMakerandSoundSetclasses), which I personally think makes it easier to grok.It definitely makes it easier to test!
I think this is best reviewed commit-by-commit, since that tells the "story" of the refactoring better.
That story was written over several evenings, so sometimes I needed multiple steps to reach a sane end-state.