New tie-fret-tab-show-not-show thing #20744
Conversation
| </property> | ||
| <property name="currentIndex"> | ||
| <number>0</number> | ||
| <number>2</number> |
There was a problem hiding this comment.
For the future, I think it's generally preferable to not commit a change like this; but for now it's not worth the effort of a force-push.
| } | ||
|
|
||
| ShowTiedFret showTiedFret = style().value(Sid::tabShowTiedFret).value<ShowTiedFret>(); | ||
| if (showTiedFret == ShowTiedFret::TIE_AND_FRET) { |
There was a problem hiding this comment.
Actually to be very honest, I'm not sure if these methods are as clear as they could be. A few things that may be the cause of this are:
- the fact that
Sid::tabShowTiedFretis queried in a lot of places (both in forceShowFret itself, and by the callers of forceShowFret) makes the logic a bit less easy to grasp. - Patterns like the following seem to be not my favourite:
A simple switch might be slightly nicer:
if (it is A) return … if (it is C) return … // then it must be B return …case A: return … case B: return … case C: return … - It's not immediately clear which parameter takes precedence over the others, so to speak. This might become clearer by using switches. Maybe there is simply some interaction between the parameters instead of a strict precedence hierarchy, which is fine; but I think it would be possible to write it down slightly clearer.
Anyway, not sure if this is worth spending time on right now, but in the refinement stage you could keep this in mind.
There was a problem hiding this comment.
These methods look cumbersome because they implement cumbersome logic (which I deeply hate, but I also can't do much about). I honestly couldn't find a clearer way to do this. The logic isn't simple enough that a switch statement would make things better imo, and in these cases with intricate relations I do prefer the "early return" pattern (the alternative is a nesting chain of hell).
There was a problem hiding this comment.
It's very NICE!
Only little thing is that the two vibrato elements, as well as the tremolo bar element, should also trigger parenthesised fret marks when tied (I.e they ought to behave like articulation markings). Apologies if this wasn't apparent from the design!
mike-spa
force-pushed
the
newTabTieSettings
branch
from
acd2730 to
aae6f2f
Compare
There was a problem hiding this comment.
Guitar-specific articulations are now showing in parentheses ✅
Something that still needs doing: when the "show...when articulation markings are present" is checked, parentheses should appear even when the **Show ties only** option is selected.
This will also mean (for @cbjeukendrup or @Eism) that the "Parentheses indicating ties" section should become enabled when Show ties only is selected.
src/engraving/dom/note.cpp
Outdated
|
|
||
| auto hasTremoloBar = [&] () { | ||
| for (EngravingItem* item : ch->segment()->annotations()) { | ||
| if (item && item->isTremoloBar()) { |
There was a problem hiding this comment.
Is it intentional that you're not checking the track or staff index here?
There was a problem hiding this comment.
Good point, thanks 👍
src/engraving/dom/note.cpp
Outdated
| setFret(prevNote->fret()); | ||
| } | ||
|
|
||
| bool Note::hideFret() const |
There was a problem hiding this comment.
For me based on the name of the method, it looks like this method hides fret and does not return an indication that fret needs to be hidden
The same for forceShowFret
There was a problem hiding this comment.
Also good point, thanks 👍
mike-spa
force-pushed
the
newTabTieSettings
branch
from
8c30f2b to
0808b82
Compare
|
This is ready for merge, from my side! |
|
Just noticed a couple of things:
|
mike-spa
force-pushed
the
newTabTieSettings
branch
from
0808b82 to
478c17e
Compare
mike-spa
force-pushed
the
newTabTieSettings
branch
from
478c17e to
296faf4
Compare
6 participants
Resolves: #20536
New options for showing/hiding ties and fret marks in TAB staves