-
Notifications
You must be signed in to change notification settings - Fork 79
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
Editable clefs #332
base: develop
Are you sure you want to change the base?
Editable clefs #332
Conversation
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.) |
Thank you @Martchus. I pulled in from your fork and it all merges well, and runs without problems |
This works nicely. I've picked it up on my branch. It would be great if the code would be indented consistently, though. |
Ah... I didn't realize that Eclipse by default uses tabs rather than 4 spaces. Sorry for that. I corrected it |
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 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 |
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. |
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