Skip to content

Commit

Permalink
Avoid copying Selection objects
Browse files Browse the repository at this point in the history
This also fixes probably an error with not updating a selection
in ScoreView::cmdChangeEnharmonic since the own copy was modified.
  • Loading branch information
dmitrio95 committed Nov 29, 2018
1 parent af128b5 commit aeb4919
Show file tree
Hide file tree
Showing 8 changed files with 14 additions and 17 deletions.
6 changes: 2 additions & 4 deletions libmscore/cmd.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2479,16 +2479,14 @@ void Score::cmdExplode()
lastStaff = qMin(nstaves(), srcStaff + n);
}

// make our own copy of selection, since pasting modifies actual selection
Selection srcSelection(selection());

const QByteArray mimeData(selection().mimeData());
// copy to all destination staves
Segment* firstCRSegment = startMeasure->tick2segment(startMeasure->tick());
for (int i = 1; srcStaff + i < lastStaff; ++i) {
int track = (srcStaff + i) * VOICES;
ChordRest* cr = toChordRest(firstCRSegment->element(track));
if (cr) {
XmlReader e(srcSelection.mimeData());
XmlReader e(mimeData);
e.setPasteMode(true);
pasteStaff(e, cr->segment(), cr->staffIdx());
}
Expand Down
2 changes: 1 addition & 1 deletion libmscore/edit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1254,7 +1254,7 @@ void Score::cmdAddTie()

void Score::cmdAddOttava(OttavaType type)
{
Selection sel = selection();
const Selection& sel = selection();
// add on each staff if possible
if (sel.isRange() && sel.staffStart() != sel.staffEnd() - 1) {
for (int staffIdx = sel.staffStart() ; staffIdx < sel.staffEnd(); ++staffIdx) {
Expand Down
5 changes: 2 additions & 3 deletions mscore/palette.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -451,7 +451,7 @@ void Palette::applyPaletteElement(PaletteCell* cell, Qt::KeyboardModifiers modif
Score* score = mscore->currentScore();
if (score == 0)
return;
Selection sel = score->selection(); // make a copy of the list
const Selection& sel = score->selection(); // make a copy of the list

This comment has been minimized.

Copy link
@MarcSabatella

MarcSabatella Dec 8, 2018

Contributor

This much creates crashes applying palette elements to list selections - see https://musescore.org/en/node/279662.

In general, the reason we do these copies is because you get crashes and other very problems when looping over a list if the code within the loop modifies the list. That's why we are so careful to create copies of lists before iterating over them in general. Probably most of these were done for that reason.

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Dec 9, 2018

Author Contributor

In most of these cases copying of selection is really not necessary, but I indeed seem to overlook some cases when some information from it needs copying. In general I believe it is better to copy not the whole selection but only the necessary information from it as Selection objects are now too closely bound to Score, though in this exact case copying the whole selection may be easier.
I added this commit to one PR in conjunction with 247802b as I was afraid that copying of selection in general could create the same sort of problems that commit fixes but right now some changes from it can be harmlessly reverted or modified. I will revise these changes soon.

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Dec 9, 2018

Author Contributor

See #4358 for the fix for this commit.

if (sel.isNone())
return;

Expand Down Expand Up @@ -753,8 +753,7 @@ void Palette::mouseDoubleClickEvent(QMouseEvent* ev)
Score* score = mscore->currentScore();
if (score == 0)
return;
Selection sel = score->selection(); // make a copy of the list
if (sel.isNone())
if (score->selection().isNone())
return;

applyPaletteElement(cellAt(i), ev->modifiers());
Expand Down
4 changes: 2 additions & 2 deletions mscore/pianotools.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ void HPiano::releasePitch(int pitch)
// changeSelection
//---------------------------------------------------------

void HPiano::changeSelection(Selection selection)
void HPiano::changeSelection(const Selection& selection)
{
for (PianoKeyItem* key : keys) {
key->setHighlighted(false);
Expand Down Expand Up @@ -533,7 +533,7 @@ bool HPiano::gestureEvent(QGestureEvent *event)
// changeSelection
//---------------------------------------------------------

void PianoTools::changeSelection(Selection selection)
void PianoTools::changeSelection(const Selection& selection)
{
_piano->changeSelection(selection);
}
Expand Down
4 changes: 2 additions & 2 deletions mscore/pianotools.h
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ class HPiano : public QGraphicsView {
void pressPitch(int pitch);
void releasePitch(int pitch);
void clearSelection();
void changeSelection(Selection selection);
void changeSelection(const Selection& selection);
void updateAllKeys();
virtual QSize sizeHint() const;

Expand Down Expand Up @@ -110,7 +110,7 @@ class PianoTools : public QDockWidget {
void releasePitch(int pitch) { _piano->releasePitch(pitch); }
void heartBeat(QList<const Note*> notes);
void clearSelection();
void changeSelection(Selection selection);
void changeSelection(const Selection& selection);
};


Expand Down
4 changes: 2 additions & 2 deletions mscore/pianoview.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -937,7 +937,8 @@ void PianoView::selectNotes(int startTick, int endTick, int lowPitch, int highPi
//score->masterScore()->cmdState().reset(); // DEBUG: should not be necessary
score->startCmd();

Selection selection(score);
Selection& selection = score->selection();
selection.deselectAll();

for (int i = 0; i < noteList.size(); ++i) {
PianoItem* pi = noteList[i];
Expand Down Expand Up @@ -967,7 +968,6 @@ void PianoView::selectNotes(int startTick, int endTick, int lowPitch, int highPi
selection.add(pi->note());
}

score->setSelection(selection);
for (MuseScoreView* view : score->getViewer())
view->updateAll();

Expand Down
2 changes: 1 addition & 1 deletion mscore/scoreview.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3308,7 +3308,7 @@ void ScoreView::cmdAddNoteLine()
void ScoreView::cmdChangeEnharmonic(bool both)
{
_score->startCmd();
Selection selection = _score->selection();
Selection& selection = _score->selection();
QList<Note*> notes = selection.uniqueNotes();
for (Note* n : notes) {
Staff* staff = n->staff();
Expand Down
4 changes: 2 additions & 2 deletions mscore/timeline.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1726,8 +1726,8 @@ void Timeline::drawSelection()

std::set<std::tuple<Measure*, int, ElementType>> meta_labels_set;

const Selection selection = _score->selection();
QList<Element*> el = selection.elements();
const Selection& selection = _score->selection();
const QList<Element*>& el = selection.elements();
for (Element* element : el) {
if (element->tick() == -1)
continue;
Expand Down

0 comments on commit aeb4919

Please sign in to comment.