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

Editable clefs #332

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Editable clefs #332

wants to merge 2 commits into from

Conversation

gjdv
Copy link
Contributor

@gjdv gjdv commented Feb 6, 2023

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
Copy link
Contributor

Martchus commented Feb 6, 2023

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
Copy link
Contributor Author

gjdv commented Feb 6, 2023

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

@Martchus
Copy link
Contributor

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

@gjdv
Copy link
Contributor Author

gjdv commented Feb 13, 2023

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

@Martchus
Copy link
Contributor

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
Copy link
Contributor Author

gjdv commented Feb 14, 2023

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.

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

Successfully merging this pull request may close these issues.

2 participants