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

fix #276840: Soundfont selector: button to move soundfont to top #3983

Closed
wants to merge 28 commits into from
Closed

fix #276840: Soundfont selector: button to move soundfont to top #3983

wants to merge 28 commits into from

Conversation

barafael
Copy link
Contributor

This branch adds a button to the soundfont selector (View -> Synthesizer -> Fluid tab). The button moves the currently selected item to the top, changing the playback soundfont.

Previously, one had to repeatedly click the "Move soundfont up" button.

Note: This is affected by issue #276456, which I created. I verified that the issue persists without my changes.

@anatoly-os
Copy link
Contributor

Thank you for your contribution. We thought about "Apply" button which should apply all changes made after using arrows to change the place of the soundfonts. Your solution is good as well.
Could you please sign cla so we could merge your changes to the product? (https://musescore.org/en/user/1743378/cla)

@barafael
Copy link
Contributor Author

barafael commented Sep 22, 2018

I have signed the CLA.

https://musescore.org/en/user/31589/cla

@anatoly-os
Copy link
Contributor

@barafael could you please create the issue in the tracker which describes the main reason of these changes. It is useful to track related changes in the future.
Could you also please squash all commits at one and add "fix #XXXXXX" at the beginning of the title and the commit message (XXXXXX is a number of the created issue mentioned in the first paragraph), so the issue could be automatically closed?

@barafael barafael changed the title Soundfont selector: button to move soundfont to top fix #276840: Soundfont selector: button to move soundfont to top Oct 4, 2018
@barafael
Copy link
Contributor Author

barafael commented Oct 4, 2018

Travis fails, it looks like due a package management issue. I squashed my commits but did not change the code. If there is something I need to do (like cherry pick from master) let me know.
Issue: https://musescore.org/en/node/276840

@anatoly-os
Copy link
Contributor

Some internal error, I've restarted the build.

@barafael
Copy link
Contributor Author

IMHO one possible implementation of this feature would be to have a drop-down list of soundfonts. I am willing to try to implement this, are you open for this change?

@anatoly-os
Copy link
Contributor

@barafael what are advantages of the proposed approach? I think having "move to top" already does its job. Btw, you also need to add this widget and related logic to Zerberus synth.

@barafael
Copy link
Contributor Author

@anatoly-os right now the purpose of the up/down buttons is unclear, that is why I thought to experiment a bit.
A dropdown requires 2 clicks, just like selecting a list entry and moving it to top. Actually, dropdown is not better here.
I applied the changes to zerberus gui as well.
I still need to change the move-to-top button to gray out when list is empty (like the up/down buttons).

@barafael
Copy link
Contributor Author

The buttons in fluid and zerberus are now grayed out when the list is empty or the first/last element is selected. I changed the code in the zerberus updateButtons function as it didn't correctly en/disable the buttons correctly. I hope it has no other side effects.

The weird bug where the soundfont list is growing with every reload does NOT affect the zerberus list, by the way.

@anatoly-os
Copy link
Contributor

anatoly-os commented Oct 25, 2018

@barafael please add "fix #276840:" at the beginning of the commit message so the related issue could be closed automatically.

UPD: which username did you use in CLA?

@barafael
Copy link
Contributor Author

I used username "rafaelement": https://musescore.org/en/user/31589 which is my old forum name.

@barafael
Copy link
Contributor Author

barafael commented Nov 6, 2018

Please let me know if there is anything else I need to do or if there is something wrong with the CLA.

@barafael
Copy link
Contributor Author

barafael commented Aug 16, 2019

If there is nothing more to do here, I will delete the branch and close the PR.
Else, I will resolve the merge.
Please let me know :)

@anatoly-os
Copy link
Contributor

Let me look at this PR again tomorrow. I'll leave the comment once I refresh my memory.

@anatoly-os
Copy link
Contributor

@barafael could you please rebase the changes on top of the master branch? The changes look good and should have been merged earlier...

@barafael
Copy link
Contributor Author

I think this is beyond my merge-fu. I'd rather open a new pull request with the same changes.

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Oct 21, 2020

@barafael are you still planning to open a new pull request on this?
One to move a soundfont to the bottom?

@barafael
Copy link
Contributor Author

No, I don't think I will.

@Jojo-Schmitz
Copy link
Contributor

OK, I'm on it 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.

8 participants