-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 #175886: allow fretboards to be added to chord symbols #3199
fix #175886: allow fretboards to be added to chord symbols #3199
Conversation
Could you add a test or two ? |
Absolutely. I still need to check out how this code works on 2.x anyhow, and might end up modifying the code as a result. ideally the same code would work for both, but I won't know that for a little bit yet. |
Hmm, now that I look at adding a test, I'm not really sure how. The change is mostly about the "drop" code, and I'm not clear on the best way to hook into that. |
I've added tests to the 2.2 version of the branch #3205. Need to re-incorporate that code into this branch (too much different between 2.2 and master to share PR). There are also a couple of bugs on master involving chord symbols that probably need to be looked at before this current code will actually work correctly. |
467f9c2
to
cc55a23
Compare
In theory this is good to go, and we could cherry pick the two mtest commits from #3205 into this branch. In reality, I'm not sure they'll pass as I see other bugs I need to investigate. I'm not clear on how to do a clean cherry pick on my own system, but I figured I'd push this as is now in case my attempts mess up my local branch. |
At first, I thought you were resurecting the approach of making a single entity of "Chord Symbol + Fretboard" which was a cool idea because it would mean we can add these into the palette. But now I understand you want to be able to add a fretboard when a chordname is selected, and vice versa, but they remain attached to a segment. OK, why not. There is one thing though that doesn't work, dropping a chordname on a fretboard with Ctrl+Shift+Click... but it's also broken when dropping on rest or chord. |
I'll merge this. Cloning chord symbols is another issue. |
Yes, it was a cool idea, but was never fully implemented as far as I know, and I have no idea how much effort would be involved to get it to work, and I suspect it would not be completely compatible although I guess that depends on whether this affected how the tags get written to the files or if it's just a matter on internals. Still worth revisiting for 3.0 I guess, but the current approach is safer for 2.2, hence the corresponding PR #3205 |
Right now we can easily attach chord symbols to any beat whether there is a note there or not (space bar while entering chord symbols to advanced beat by beat). But there is no equivalent simple way to add fretboard diagrams. Eventually we could consider some sort of fretboard entry mdoe with similar shortcuts to advance by beat, but that's a lot of work. For now, an extremely simple solution seems to be to let the user drop fretboard diagrams on chord symbols.
We should also allow adding chord symbols to fretboard diagrams (select a diagram, Ctrl+K). This works on master but not in 2.1. Should be simple to add this as well, but I'll need to make a 2.x-specific PR for that I guess. Note the current PR is tested on master and works as far as chord symbols work on master at all, which is pretty sketchy. So I should probably do a separate 2.x version of this PR anyhow.