Enum-ify Theme.
#89
Enum-ify Theme.
#89
Conversation
|
Looks good. First time reviewing on my mobile, a little squishy, but I got there. Once rebased, I'll give it another quick check, plus a run and a test some time soon. Then I reckon we cut a release to get all your good stuff out. The sooner we get the tech changes out, the easier to diagnose any accidental bugs, and we can also get the user facing feature of the new melody out too. |
Since this shifts the color-evaluation to static-initializer time, we additionally need to mock Android.graphics.Color, so that our Theme-using tests continue to work.
Move the Context-dependence 'up' to PianoCanvas for now.
This will prevent app-crashes if we ever seriously mishandle preferences in a future release.
Bug introduced in 2018 with commit cf8bd94. That converted a null-return to KeyArea(0,0,0,0), but forgot to update the matching check in the rendering logic.
This: - makes small keys theme-able in the future, and - brings the drawing implementation for both key-types closer together.
Logic outside of Piano shouldn't have to care about the relationship between key-index odd/even and big-vs-small-ness of the keys. We're not completely there yet, due to the z-index draw ordering, but it's definitely an improvement. - Style: convert snake_case names to proper camelCase, where touched.
juleskers
force-pushed
the
theme-cleanup
branch
from
b0fd1fd to
252fac1
Compare
|
Thanks for the review, Peter! (And the thumbs-up, Nico) I've rebased, and added a small round of additional javadoc to the themes themselves. I couldn't find any issues in 5 minutes of on-device testing on my phone, and theme switching still worked. I did run into #75 again; I'm suspecting an interaction between rotation and the lock-screen that warrants some testing. As for mobile reviewing: I find that landscape-rotation is a must for code-view, and even then it usually wraps lines. |
|
LGTM. Thanks for rebasing. I'll merge them prep a new release. |
Per discussion in #88 , I think I can make at least the
Themereflection there cleaner by making it an Enum instead of a class with static self-constants.That can be done as a separate change, which is this PR. If this is acceptable, I'll rebase #88 onto this after it's merged.
While Enum-ifying Theme, I noticed that it's a bit inconsistent that only big keys are theme-able.
Pulling the color-decision for the small/flat keys into Theme as well allows some unification of drawing logic in
PianoCanvas.Right now, flat keys are still hardcoded black, but this improvement means that if we ever want to make colorful flat keys, we need to adapt only
Theme, which satisfies the Single-Responsibility-Principle.The drawing-simplifications are further aided by making more of the "big vs small key area" logic internally handled in
Piano.Ideally the property that "odd keyIdx == small key" should be a completely invisible implementation detail of
Piano.I'm close to that goal, but not all the way there.
One thing I don't like about this MR is that we now need a mock for
android.graphics.Color, since the enum-constants forThemeare now evaluated at static-initialiser time. Previously, since we didn't actually use the colors, we didn't run into this, as the classloader doesn´t load static fields until somebody accesses the first one (and then it loads all static fields in that class).For the moment, I've gone the simple-but-boilerplate-y route that I previously took for
Log: provide a stub test-double that shadows the android-class, only during tests.I guess this is an example of why one should normally have wrappers and adapter-classes to isolate ones business logic from such "external" library stuff.
Please let me know how much this bugs you; the alternative would be code
Themeagainst acom.nicobrailo.pianoli.Color, and only convert toandroid.graphics.Colors once we get to drawing inPianoCanvas.