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.

Sign up for GitHub

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

System object correction #26588

Merged
merged 9 commits into from
Feb 21, 2025
+24 −7
Merged

System object correction #26588

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7a2e27a
Fix wrong order of instrument and system object layer
mike-spa Feb 19, 2025
757ecc8
Fix system object layer not updated on undo move up
mike-spa Feb 19, 2025
c0b2ae5
Ensure non-selectable items can't be selected
mike-spa Feb 19, 2025
f5941a4
Unlink element before removing
mike-spa Feb 20, 2025
841026f
Collect system objects again after move
mike-spa Feb 20, 2025
81ec8e7
Disable move down for system object layer on last instrument
mike-spa Feb 20, 2025
e611748
code review
mike-spa Feb 20, 2025
427ab3b
Update system objects in LayoutPanelTreeModel::updateSystemobjectLayers
mike-spa Feb 20, 2025
1e50918
codestyle
mike-spa Feb 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/instrumentsscene/view/layoutpaneltreemodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,9 @@ void LayoutPanelTreeModel::onBeforeChangeNotation()
QList<muse::ID> partIdList;

for (const AbstractLayoutPanelTreeItem* item : m_rootItem->childItems()) {
if (item->type() == LayoutPanelItemType::ItemType::SYSTEM_OBJECTS_LAYER) {
continue;
}
partIdList << item->id();
}

Expand Down Expand Up @@ -869,6 +872,9 @@ void LayoutPanelTreeModel::setItemsSelected(const QModelIndexList& indexes, bool
{
for (const QModelIndex& index : indexes) {
if (AbstractLayoutPanelTreeItem* item = modelIndexToItem(index)) {
if (!item->isSelectable()) {
continue;
}
item->setIsSelected(selected);
}
}
Expand Down Expand Up @@ -994,7 +1000,7 @@ void LayoutPanelTreeModel::updateSystemObjectLayers()
const int partRow = partItem->row();
const int layerRow = layerItem->row();

if (partRow < layerRow) {
if (layerRow != partRow - 1) {
beginMoveRows(QModelIndex(), layerRow, layerRow, QModelIndex(), partRow);
m_rootItem->moveChildren(layerRow, 1, m_rootItem, partRow, false /*updateNotation*/);
endMoveRows();
Expand Down
1 change: 1 addition & 0 deletions src/instrumentsscene/view/systemobjectslayertreeitem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ void SystemObjectsLayerTreeItem::onUndoStackChanged(const mu::engraving::ScoreCh
}

if (shouldUpdateState) {
m_systemObjectGroups = collectSystemObjectGroups(m_staff);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid this change will have a negative impact on performance. It scans the entire score for system objects, and we will now call it every time when the user adds / removes / changes a system object. On a small score it doesn't matter as much, but on a larger one it may be more noticeable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

20250220_153116.mp4

This was the problem I was trying to solve. Dragging a system object layer on another should cancels one of them, which means the remaining one needs to update its elements and state. I've tried a different solution in the last commit, could you tell me if that looks better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks better, thank you! However, it can be optimized even further: instead of scanning each staff one by one, we can call collectSystemObjectGroups for all of them (outside of the for loop) and update the state of the items. But it's rather nice to have :)

updateState();
}
}
Expand Down
1 change: 1 addition & 0 deletions src/notation/internal/notationparts.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,6 +814,7 @@ void NotationParts::moveSystemObjects(const ID& sourceStaffId, const ID& destina
if (item->staff() == srcStaff) {
item->undoChangeProperty(Pid::TRACK, staff2track(dstStaffIdx, item->voice()));
} else {
item->undoUnlink();
score()->undoRemoveElement(item, false /*removeLinked*/);
}
}
Expand Down
Loading