Fix #21572: Changing color of beamed chords should only change the elements within selection

[Nekotizimo]

Resolves: #21572

Changing the color of a range selection now only affects the highlighted elements (i.e. those in selection()), instead of every note in the beamed chords (i.e. m_elementList, which the leading space etc settings use). This seems to be the behavior in MS3.

Change color of selection ->

• I signed the CLA
• The title of the PR describes the problem it addresses
• Each commit's message describes its purpose and effects, and references the issue it resolves
• If changes are extensive, there is a sequence of easily reviewable commits
• The code in the PR follows the coding rules
• There are no unnecessary changes
• The code compiles and runs on my machine, preferably after each commit individually
• I created a unit test or vtest to verify the changes I made (if applicable)

[cbjeukendrup]

I'm afraid it's not safe to just take all the selected elements. This may contain things like InstrumentNames and Brackets, and those are not really safe to work with in the Inspector. See also ElementRepositoryService::exposeRawElements, where such elements are filtered.

A better solution, albeit slightly more difficult perhaps, would be to revise the fact that ElementRepositoryService::exposeRawElements (and thus ElementRepositoryService::takeAllElements()) includes all Chords when a Beam is selected. I don't see why that's desirable, and I do think it's causing trouble here.

So, what I would propose is:

1. Check where takeAllElements is used, and verify whether it is not harmful to remove those extra Chords from it
2. Remove those lines from exposeRawElements that add those extra chords
3. Check how this affects other methods inside ElementsRepositoryService that use m_exposedElementList. In cases where this changes the behaviour of those methods, consider whether adjustments need to be made to counter that change, or that the new behaviour is an improvement.

And ideally, report your findings of each step here, because that makes it much easier to provide proper testing and feedback. Thanks!

[Nekotizimo]

Thanks a lot for the info. I tried to track down the reasoning behind adding the extra chords connected by the beam (it was added in #6149). These extra chords are probably added so that when a beam is selected, you can also change the settings for the stems attached to it in the inspector, which I think is good.

I tried removing those lines, and as far as I can tell, this is probably unwanted behavior for the 3 places that use takeAllElements:

• GeneralSettingsModel - This concerns the 4 general settings:
  • "Play" & "Cue size": It doesn't make sense for any notes beyond the selection to be affected. This also means that when only a beam is selected, these checkboxes will now be disabled, which is good IMO. When only a stem is selected, the checkboxes affect the notes attached to it, which is the same as before.
  • "Auto-place": Currently, selecting only the beam and disabling auto-place will turn off auto-place for notes in the chords and cause staff text to potentially collide with notes in the chords. Furthermore, despite auto-place being seemingly turned off for the notes, the inspector shows auto-place being on for the notes, and unchecking and checking it doesn't seem to do anything. Removing the extra chords seem to solve the problem.
  • "Visible": Previously, hiding a range would hide all the beamed chords (not perfectly since it misses dots outside the range) and cause the horizontal spacing to collapse. After removing the extra chords, this checkbox seems to behave more like pressing "V", where noteheads and articulations are hidden but stems and beams stay, unless the range contains the whole beam.
• PartsSettingsModel - this concerns whether position and styles of elements in part scores are linked to the master score. I don't think it makes sense for these extra chords to be affected by these settings either. For example, if you change the direction of a beam in a part score, it wouldn't make sense for the notes under the beam to be forced to desynchronize with the main score.
• AppearanceSettingsModel - Among color and other settings, I don't think it makes sense for the leading space of the whole beamed chord to be changed. I thought this was an intentional choice when I first looked at this, but I don't think it makes sense for layout outside the range to be affected.

So far, it looks like these including these extra chords are not desirable, but simply removing these lines also cause the settings for note stem and beam to gray out when only a beam is selected. I'll continue to look more into this and see how other stuff that uses m_exposedElementList are affected. I'm guessing a separate, filtered list of "selectedElementList" should be added.

[cbjeukendrup]

The latest version looks very good to me! I have two small suggestions.

@oktophonie It would be good to re-test this, because now it will have slightly more effects; but only positive ones (hopefully)!

[Nekotizimo]

For reference, I've removed the extra ChordRests from m_exposedElementList, which is used in these methods:

• findChords - I presume we want the extra chords here, so that selecting the beam will allow you to change the properties of all the stems attached (Ex. beam mode, stem thickness etc.)
  • findStems, findHooks, findBeams - these go through the elements from findChords()
• findRests - for the same reason i.e. setting beam mode, we would want the extra rests under the selected beam.
• findLines, findStaffs, findSectionBreaks, findText, findBrackets, findOrnaments - these go through m_exposedElementList, but the extra chords don't affect their behavior
• findNoteheads - goes through rawElementsList, with no extra chords
• findNotes - also goes through rawElementsList but also adds the extra notes contained within the selected beam. PS it looks like this function is no longer called after the change in Fix #21285: Only apply playback changes to notes with head selected #22838

I have moved the code that adds the extra chords from takeAllElements to findChords and findRests. This way, the general settings ("Visible", "Cue size" etc.) and appearance settings only affects the (filtered) selected elements, while you can still change the beam mode and stem settings of connected notes by only selecting part of the notes under the beam, which I presume is intended.

[cbjeukendrup]

Thank you!