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

Further improvements to the Score Wizard #1375

Merged
merged 9 commits into from
Mar 26, 2023
Merged

Further improvements to the Score Wizard #1375

merged 9 commits into from
Mar 26, 2023

Conversation

bmjcode
Copy link
Contributor

@bmjcode bmjcode commented Jun 6, 2021

Hi there! I've made a few more improvements to the Score Wizard:

  • Made the MIDI instrument user-selectable for several keyboard and guitar part types
  • Added new part types for steelpan (steel drums) and dulcimer
  • Added a special "Structure" type for laying out breaks, bar lines, etc. separately from musical content
  • Made various small fixes and corrections

@fedelibre
Copy link
Member

@bmjcode Could you please remove the fix typo commit from this PR? You may do a new PR containing only the modifications to .py and .dic file. Do not touch the .pot and .po files, as these are normally handled separately when we are closer to a release.

Then I may test your branch and comment on the features you added. I can't review the code, but maybe others will as soon as you remove that commit and rebase on current master.

Sorry for the long delay, but as you can see from the history Frescobaldi development is slowly coming back. We'd like to release 3.3 soon, after fixing the last critical issue.

@fedelibre fedelibre mentioned this pull request Jan 26, 2023
@fedelibre
Copy link
Member

@bmjcode I did the cleanup for you and force-pushed to your branch.
I'll test it as soon as I have some time.

@fedelibre fedelibre added this to the 3.3 release milestone Jan 26, 2023
@fedelibre
Copy link
Member

I've now tested it and it looks good. Waiting for others to give feedback on code.

Copy link
Collaborator

@dliessi dliessi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me and seem to work as expected.

There is a merge commit in the history, which is undesirable. @bmjcode Can you please rebase your branch on origin/master and push again? (Next time you may use a new branch for submitting changes, instead of master.) Thanks.

Marking my review as "Request changes" for the rebasing, otherwise I approve it.

@bmjcode
Copy link
Contributor Author

bmjcode commented Mar 26, 2023

@dliessi I'll be happy to if you can tell me how to do it. My understanding of Git is frankly pretty limited.

@fedelibre
Copy link
Member

It's more difficult now that you've rebased on master and your commits are way down the list. I would have squashed them first and then rebased on master.
I would wait for suggestions from Davide and Jean, who are more competent than me.

My suggestion is making a backup branch first:

git checkout -b 1375-backup

Then change back to the branch of this PR and try fixing the commits:

git checkout master
git rebase -i 36d7a38b4db25c608958da08f289268dd8dd1ae1

where that committish is the first commit in this PR.

If you're not confident on rebasing you may leave this task to Jean.

@jeanas jeanas merged commit 989674d into frescobaldi:master Mar 26, 2023
@jeanas
Copy link
Member

jeanas commented Mar 26, 2023

Now rebased and merged (based on @dliessi's review; I didn't re-review thoroughly myself). Thank you!

fedelibre added a commit to fedelibre/frescobaldi that referenced this pull request Mar 26, 2023
jeanas pushed a commit that referenced this pull request Mar 26, 2023
@bmjcode
Copy link
Contributor Author

bmjcode commented Mar 26, 2023

Cool. Thanks all for your help and patience with this. I have a couple other things I'd like to work on, but they're a bit more complicated. I'll open issues for those so we can talk through the details first.

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.

None yet

4 participants