search

pianobooster / PianoBooster

A MIDI file player/game that displays the musical notes and teaches you how to play the piano.
Other
397 stars 74 forks source link

Editable clefs #332

Open gjdv opened 1 year ago

gjdv commented 1 year ago

I implemented editable clefs by having two dropdown boxes where you can set the clefs used per hand (default: right-treble, left-bass). It dynamically updates using the default redrawing mechanism, redraws the clefs and moves the notes and all other symbol elements (accidentals etc). This solves issue #52

Martchus commented 1 year ago

This looks generally good. I'll give it a test next time I'll work on PianoBooster. (I'm not the maintainer and can't merge PRs. Maybe I'll pick it up on my fork which is currently 16 commits ahead of this repo as PRs are generally only merged slowly here.)

gjdv commented 1 year ago

Thank you @Martchus. I pulled in from your fork and it all merges well, and runs without problems

Martchus commented 1 year ago

This works nicely. I've picked it up on my branch. It would be great if the code would be indented consistently, though.

gjdv commented 1 year ago

Ah... I didn't realize that Eclipse by default uses tabs rather than 4 spaces. Sorry for that. I corrected it

Martchus commented 1 year ago

Good. For the sake of this PR it would likely better to avoid including my commits (so individual PRs are small and easy to review). Since there's no conflict there's also really no necessity to do so. Supposedly it makes also sense to avoid any merge commits on feature branches. The easiest way to clean things up is to invoke git rebase -i develop and in the editor prompt remove all commits besides your actual commits. You might also want to squash the last commit.

I don't think this following this is required here but maybe still worth a read: https://www.theserverside.com/video/Follow-these-git-commit-message-guidelines

gjdv commented 1 year ago

Sorry, I mixed up a commit from your fork with that (with the same message) from mine, causing an unintended merge between the forks. I think it should be corrected now.