Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions src/engraving/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,8 @@ target_sources(engraving PRIVATE
editing/editmeasures.h
editing/editnote.cpp
editing/editnote.h
editing/editpagelocks.cpp
editing/editpagelocks.h
editing/editparentheses.cpp
editing/editparentheses.h
editing/editpart.cpp
Expand Down
1 change: 1 addition & 0 deletions src/engraving/api/v1/apitypes.h
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,7 @@ enum class ElementType {
TAB_DURATION_SYMBOL = int(mu::engraving::ElementType::TAB_DURATION_SYMBOL),
FSYMBOL = int(mu::engraving::ElementType::FSYMBOL),
PAGE = int(mu::engraving::ElementType::PAGE),
PAGE_LOCK_INDICATOR = int(mu::engraving::ElementType::PAGE_LOCK_INDICATOR),
HAIRPIN = int(mu::engraving::ElementType::HAIRPIN),
OTTAVA = int(mu::engraving::ElementType::OTTAVA),
PEDAL = int(mu::engraving::ElementType::PEDAL),
Expand Down
6 changes: 3 additions & 3 deletions src/engraving/api/v1/elements.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -505,11 +505,11 @@ void System::setIsLocked(bool locked)
return;
}
Transaction& tx = system()->score()->transactionManager()->currentOrDummyTransaction();
const mu::engraving::SystemLock* currentLock = system()->systemLock();
const mu::engraving::RangeLock* currentLock = system()->systemLock();
if (currentLock && !locked) {
EditSystemLocks::undoRemoveSystemLock(tx, currentLock);
EditSystemLocks::undoRemoveSystemLock(tx, system()->score(), currentLock);
} else if (!currentLock && locked) {
EditSystemLocks::undoAddSystemLock(tx, system()->score(), new mu::engraving::SystemLock(system()->first(), system()->last()));
EditSystemLocks::undoAddSystemLock(tx, system()->score(), new mu::engraving::RangeLock(system()->first(), system()->last()));
}
}

Expand Down
8 changes: 6 additions & 2 deletions src/engraving/dom/dom.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,8 @@ set(DOM_SRC
${CMAKE_CURRENT_LIST_DIR}/ottava.h
${CMAKE_CURRENT_LIST_DIR}/page.cpp
${CMAKE_CURRENT_LIST_DIR}/page.h
${CMAKE_CURRENT_LIST_DIR}/pagelockindicator.cpp
${CMAKE_CURRENT_LIST_DIR}/pagelockindicator.h
${CMAKE_CURRENT_LIST_DIR}/parenthesis.cpp
${CMAKE_CURRENT_LIST_DIR}/parenthesis.h
${CMAKE_CURRENT_LIST_DIR}/partialtie.cpp
Expand All @@ -237,6 +239,8 @@ set(DOM_SRC
${CMAKE_CURRENT_LIST_DIR}/property.h
${CMAKE_CURRENT_LIST_DIR}/range.cpp
${CMAKE_CURRENT_LIST_DIR}/range.h
${CMAKE_CURRENT_LIST_DIR}/rangelock.cpp
${CMAKE_CURRENT_LIST_DIR}/rangelock.h
${CMAKE_CURRENT_LIST_DIR}/rasgueado.cpp
${CMAKE_CURRENT_LIST_DIR}/rasgueado.h
${CMAKE_CURRENT_LIST_DIR}/realizedharmony.cpp
Expand Down Expand Up @@ -316,12 +320,12 @@ set(DOM_SRC
${CMAKE_CURRENT_LIST_DIR}/symbol.h
${CMAKE_CURRENT_LIST_DIR}/synthesizerstate.cpp
${CMAKE_CURRENT_LIST_DIR}/synthesizerstate.h
${CMAKE_CURRENT_LIST_DIR}/systemlock.cpp
${CMAKE_CURRENT_LIST_DIR}/systemlock.h
${CMAKE_CURRENT_LIST_DIR}/system.cpp
${CMAKE_CURRENT_LIST_DIR}/system.h
${CMAKE_CURRENT_LIST_DIR}/systemdivider.cpp
${CMAKE_CURRENT_LIST_DIR}/systemdivider.h
${CMAKE_CURRENT_LIST_DIR}/systemlockindicator.cpp
${CMAKE_CURRENT_LIST_DIR}/systemlockindicator.h
${CMAKE_CURRENT_LIST_DIR}/systemtext.cpp
${CMAKE_CURRENT_LIST_DIR}/systemtext.h
${CMAKE_CURRENT_LIST_DIR}/tapping.cpp
Expand Down
5 changes: 4 additions & 1 deletion src/engraving/dom/engravingobject.h
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ class Ornament;
class Ottava;
class OttavaSegment;
class Page;
class PageLockIndicator;
class PalmMute;
class PalmMuteSegment;
class Parenthesis;
Expand Down Expand Up @@ -438,6 +439,7 @@ class EngravingObject
CONVERT(FretDiagram, FRET_DIAGRAM)
CONVERT(HarpPedalDiagram, HARP_DIAGRAM)
CONVERT(Page, PAGE)
CONVERT(PageLockIndicator, PAGE_LOCK_INDICATOR)
CONVERT(Text, TEXT)
CONVERT(MeasureNumber, MEASURE_NUMBER)
CONVERT(MMRestRange, MMREST_RANGE)
Expand Down Expand Up @@ -587,7 +589,7 @@ class EngravingObject
return isArticulationFamily() || isFermata();
}

bool isIndicatorIcon() const { return isSystemLockIndicator() || isStaffVisibilityIndicator(); }
bool isIndicatorIcon() const { return isSystemLockIndicator() || isPageLockIndicator() || isStaffVisibilityIndicator(); }
};

//---------------------------------------------------
Expand Down Expand Up @@ -770,6 +772,7 @@ CONVERT(ChordLine)
CONVERT(FretDiagram)
CONVERT(HarpPedalDiagram)
CONVERT(Page)
CONVERT(PageLockIndicator)
CONVERT(SystemText)
CONVERT(BracketItem)
CONVERT(Staff)
Expand Down
15 changes: 13 additions & 2 deletions src/engraving/dom/factory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@
#include "ornament.h"
#include "ottava.h"
#include "page.h"
#include "pagelockindicator.h"
#include "palmmute.h"
#include "parenthesis.h"
#include "partialtie.h"
Expand Down Expand Up @@ -102,7 +103,7 @@
#include "stringtunings.h"
#include "system.h"
#include "systemdivider.h"
#include "systemlock.h"
#include "systemlockindicator.h"
#include "systemtext.h"
#include "soundflag.h"
#include "tapping.h"
Expand Down Expand Up @@ -302,6 +303,7 @@ EngravingItem* Factory::doCreateItem(ElementType type, EngravingItem* parent)
case ElementType::FIGURED_BASS_ITEM:
case ElementType::DUMMY:
case ElementType::SYSTEM_LOCK_INDICATOR:
case ElementType::PAGE_LOCK_INDICATOR:
case ElementType::HAMMER_ON_PULL_OFF_SEGMENT:
case ElementType::HAMMER_ON_PULL_OFF_TEXT:
case ElementType::TAPPING_HALF_SLUR:
Expand Down Expand Up @@ -477,6 +479,15 @@ MAKE_ITEM_IMPL(NoteLine, Note);

CREATE_ITEM_IMPL(Page, RootItem, isAccessibleEnabled)

PageLockIndicator* Factory::createPageLockIndicator(System * parent, const RangeLock * lock, bool isAccessibleEnabled)
{
PageLockIndicator* pli = new PageLockIndicator(parent, lock);
pli->setAccessibleEnabled(isAccessibleEnabled);
return pli;
}

COPY_ITEM_IMPL(PageLockIndicator)

CREATE_ITEM_IMPL(PartialTie, Note, isAccessibleEnabled)
COPY_ITEM_IMPL(PartialTie)

Expand Down Expand Up @@ -747,7 +758,7 @@ CREATE_ITEM_IMPL(TimeTickAnchor, Segment, isAccessibleEnabled)

CREATE_ITEM_IMPL(StaffVisibilityIndicator, System, isAccessibleEnabled)

SystemLockIndicator* Factory::createSystemLockIndicator(System * parent, const SystemLock * lock, bool isAccessibleEnabled)
SystemLockIndicator* Factory::createSystemLockIndicator(System * parent, const RangeLock * lock, bool isAccessibleEnabled)
{
SystemLockIndicator* sli = new SystemLockIndicator(parent, lock);
sli->setAccessibleEnabled(isAccessibleEnabled);
Expand Down
7 changes: 5 additions & 2 deletions src/engraving/dom/factory.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
namespace mu::engraving {
class Instrument;
class RootItem;
class SystemLock;
class RangeLock;
class TremoloSingleChord;
class TremoloTwoChord;

Expand Down Expand Up @@ -120,7 +120,7 @@ class Factory

static StaffVisibilityIndicator* createStaffVisibilityIndicator(System* parent, bool isAccessibleEnabled = true);

static SystemLockIndicator* createSystemLockIndicator(System* parent, const SystemLock* lock, bool isAccessibleEnabled = true);
static SystemLockIndicator* createSystemLockIndicator(System* parent, const RangeLock* lock, bool isAccessibleEnabled = true);
static SystemLockIndicator* copySystemLockIndicator(const SystemLockIndicator& src);

static Lyrics* createLyrics(ChordRest* parent, bool isAccessibleEnabled = true);
Expand Down Expand Up @@ -148,6 +148,9 @@ class Factory

static Page* createPage(RootItem* parent, bool isAccessibleEnabled = true);

static PageLockIndicator* createPageLockIndicator(System* parent, const RangeLock* lock, bool isAccessibleEnabled = true);
static PageLockIndicator* copyPageLockIndicator(const PageLockIndicator& src);

static Parenthesis* createParenthesis(EngravingItem* parent, bool isAccessibleEnabled = true);
static Parenthesis* copyParenthesis(const Parenthesis& src);

Expand Down
97 changes: 90 additions & 7 deletions src/engraving/dom/measurebase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -100,14 +100,13 @@ MeasureBase::~MeasureBase()

System* MeasureBase::prevNonVBoxSystem() const
{
bool mmRests = score()->style().styleB(Sid::createMultiMeasureRests);
System* curSystem = system();
IF_ASSERT_FAILED(curSystem) {
return nullptr;
}

System* prevSystem = curSystem;
for (const MeasureBase* mb = this; mb && prevSystem == curSystem; mb = mmRests ? mb->prevMM() : mb->prev()) {
for (const MeasureBase* mb = this; mb && prevSystem == curSystem; mb = mb->prevMM()) {
if (mb->isMeasure() || mb->isHBox()) {
prevSystem = mb->system();
} else {
Expand All @@ -120,14 +119,13 @@ System* MeasureBase::prevNonVBoxSystem() const

System* MeasureBase::nextNonVBoxSystem() const
{
bool mmRests = score()->style().styleB(Sid::createMultiMeasureRests);
System* curSystem = system();
IF_ASSERT_FAILED(curSystem) {
return nullptr;
}

System* nextSystem = curSystem;
for (const MeasureBase* mb = this; mb && nextSystem == curSystem; mb = mmRests ? mb->nextMM() : mb->next()) {
for (const MeasureBase* mb = this; mb && nextSystem == curSystem; mb = mb->nextMM()) {
if (mb->isMeasure() || mb->isHBox()) {
nextSystem = mb->system();
} else {
Expand All @@ -138,6 +136,41 @@ System* MeasureBase::nextNonVBoxSystem() const
return nextSystem != curSystem ? nextSystem : nullptr;
}

Page* MeasureBase::page() const
{
return system() ? system()->page() : nullptr;
}

Page* MeasureBase::prevPage() const

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are prevPage() and nextPage() used? I can't find how and where they are used

{
Page* curPage = page();
IF_ASSERT_FAILED(curPage) {
return nullptr;
}

Page* prevPage = curPage;
for (const MeasureBase* mb = this; mb && prevPage == curPage; mb = mb->prevMM()) {
prevPage = mb->system()->page();
}

return prevPage != curPage ? prevPage : nullptr;
}

Page* MeasureBase::nextPage() const
{
Page* curPage = page();
IF_ASSERT_FAILED(curPage) {
return nullptr;
}

Page* nextPage = curPage;
for (const MeasureBase* mb = this; mb && nextPage == curPage; mb = mb->nextMM()) {
nextPage = mb->system()->page();
}

return nextPage != curPage ? nextPage : nullptr;
}
Comment on lines +144 to +172

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Potential null dereference in page navigation loops.

In both prevPage() and nextPage(), the loop body accesses mb->system()->page() without checking if mb->system() is null. While the initial check validates this->system(), subsequent MeasureBase instances encountered via prevMM()/nextMM() may have null systems (e.g., unattached measures, frames outside systems).

🛡️ Proposed fix to add null guard
 Page* MeasureBase::prevPage() const
 {
     Page* curPage = system() ? system()->page() : nullptr;
     IF_ASSERT_FAILED(curPage) {
         return nullptr;
     }
 
     Page* prevPage = curPage;
     for (const MeasureBase* mb = this; mb && prevPage == curPage; mb = mb->prevMM()) {
-        prevPage = mb->system()->page();
+        if (mb->system()) {
+            prevPage = mb->system()->page();
+        }
     }
 
     return prevPage != curPage ? prevPage : nullptr;
 }
 
 Page* MeasureBase::nextPage() const
 {
     Page* curPage = system() ? system()->page() : nullptr;
     IF_ASSERT_FAILED(curPage) {
         return nullptr;
     }
 
     Page* nextPage = curPage;
     for (const MeasureBase* mb = this; mb && nextPage == curPage; mb = mb->nextMM()) {
-        nextPage = mb->system()->page();
+        if (mb->system()) {
+            nextPage = mb->system()->page();
+        }
     }
 
     return nextPage != curPage ? nextPage : nullptr;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Page* MeasureBase::prevPage() const
{
Page* curPage = system() ? system()->page() : nullptr;
IF_ASSERT_FAILED(curPage) {
return nullptr;
}
Page* prevPage = curPage;
for (const MeasureBase* mb = this; mb && prevPage == curPage; mb = mb->prevMM()) {
prevPage = mb->system()->page();
}
return prevPage != curPage ? prevPage : nullptr;
}
Page* MeasureBase::nextPage() const
{
Page* curPage = system() ? system()->page() : nullptr;
IF_ASSERT_FAILED(curPage) {
return nullptr;
}
Page* nextPage = curPage;
for (const MeasureBase* mb = this; mb && nextPage == curPage; mb = mb->nextMM()) {
nextPage = mb->system()->page();
}
return nextPage != curPage ? nextPage : nullptr;
}
Page* MeasureBase::prevPage() const
{
Page* curPage = system() ? system()->page() : nullptr;
IF_ASSERT_FAILED(curPage) {
return nullptr;
}
Page* prevPage = curPage;
for (const MeasureBase* mb = this; mb && prevPage == curPage; mb = mb->prevMM()) {
if (mb->system()) {
prevPage = mb->system()->page();
}
}
return prevPage != curPage ? prevPage : nullptr;
}
Page* MeasureBase::nextPage() const
{
Page* curPage = system() ? system()->page() : nullptr;
IF_ASSERT_FAILED(curPage) {
return nullptr;
}
Page* nextPage = curPage;
for (const MeasureBase* mb = this; mb && nextPage == curPage; mb = mb->nextMM()) {
if (mb->system()) {
nextPage = mb->system()->page();
}
}
return nextPage != curPage ? nextPage : nullptr;
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/engraving/dom/measurebase.cpp` around lines 144 - 172, The loop bodies in
both prevPage() and nextPage() methods dereference mb->system()->page() without
first checking if mb->system() is null. While the initial check validates
this->system(), subsequent MeasureBase instances from prevMM() or nextMM() may
have null systems. Add a null check for mb->system() before accessing page() in
both loop bodies of prevPage() and nextPage() methods to prevent null pointer
dereference.


//---------------------------------------------------------
// add
/// Add new EngravingItem \a el to MeasureBase
Expand Down Expand Up @@ -638,20 +671,37 @@ bool MeasureBase::isBefore(const MeasureBase* other) const
return false;
}

const SystemLock* MeasureBase::systemLock() const
const RangeLock* MeasureBase::systemLock() const
{
return score()->systemLocks()->lockContaining(this);
}

bool MeasureBase::isStartOfSystemLock() const
{
const SystemLock* lock = score()->systemLocks()->lockStartingAt(this);
const RangeLock* lock = score()->systemLocks()->lockStartingAt(this);
return lock != nullptr;
}

bool MeasureBase::isEndOfSystemLock() const
{
const SystemLock* lock = systemLock();
const RangeLock* lock = systemLock();
return lock && lock->endMB() == this;
}

const RangeLock* MeasureBase::pageLock() const
{
return score()->pageLocks()->lockContaining(this);
}

bool MeasureBase::isStartOfPageLock() const
{
const RangeLock* lock = score()->pageLocks()->lockStartingAt(this);
return lock != nullptr;
}

bool MeasureBase::isEndOfPageLock() const
{
const RangeLock* lock = pageLock();
return lock && lock->endMB() == this;
}

Expand Down Expand Up @@ -902,6 +952,39 @@ Measure* MeasureBaseList::measureByTick(int tick) const
return nullptr;
}

MeasureBase* MeasureBaseList::firstMeasureBaseAtTick(int tick) const
{
if (empty() || tick > m_last->endTick().ticks()) {
return nullptr;
}

auto it = m_tickIndex.upper_bound(tick);

if (it == m_tickIndex.begin()) {
MeasureBase* mb = it->second;

return mb;
}

--it;
for (;; --it) {
if (it == m_tickIndex.begin()) {
MeasureBase* mb = it->second;

return mb;
}

MeasureBase* mb = it->second;
if (!mb) {
break;
}

return mb;
}

return nullptr;
}

std::vector<MeasureBase*> MeasureBaseList::measureBasesAtTick(int tick) const
{
std::vector<MeasureBase*> result;
Expand Down
12 changes: 10 additions & 2 deletions src/engraving/dom/measurebase.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class LayoutBreak;
class Measure;
class Score;
class System;
class SystemLock;
class RangeLock;

//---------------------------------------------------------
// Repeat
Expand Down Expand Up @@ -82,6 +82,9 @@ class MeasureBase : public EngravingItem
System* system() const { return toSystem(explicitParent()); }
System* prevNonVBoxSystem() const;
System* nextNonVBoxSystem() const;
Page* page() const;
Page* prevPage() const;
Page* nextPage() const;
void setParent(System* s) { EngravingItem::setParent((EngravingObject*)(s)); }

virtual void scanElements(std::function<void(EngravingItem*)> func) override;
Expand Down Expand Up @@ -160,10 +163,14 @@ class MeasureBase : public EngravingItem
bool isAfter(const MeasureBase* other) const { return !isBeforeOrEqual(other); }
bool isAfterOrEqual(const MeasureBase* other) const { return !isBefore(other); }

const SystemLock* systemLock() const;
const RangeLock* systemLock() const;
bool isStartOfSystemLock() const;
bool isEndOfSystemLock() const;

const RangeLock* pageLock() const;
bool isStartOfPageLock() const;
bool isEndOfPageLock() const;

protected:
MeasureBase(const ElementType& type, System* system = 0);
MeasureBase(const MeasureBase&);
Expand Down Expand Up @@ -203,6 +210,7 @@ class MeasureBaseList
void updateTickIndex();

Measure* measureByTick(int tick) const;
MeasureBase* firstMeasureBaseAtTick(int tick) const;
std::vector<MeasureBase*> measureBasesAtTick(int tick) const;

private:
Expand Down
Loading
Loading