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

Jump to bottom

Fixed trigger assert on remove acc from ambitus #9831

Merged
merged 1 commit into from Nov 25, 2021
Merged

Fixed trigger assert on remove acc from ambitus #9831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/engraving/libmscore/ambitus.cpp
View file
Open in desktop
Expand Up @@ -679,6 +679,16 @@ Ambitus::Ranges Ambitus::estimateRanges() const
return result;
}

void Ambitus::remove(EngravingItem* e)
{
if (e->type() == ElementType::ACCIDENTAL) {
//! NOTE Do nothing (removing _topAccid or _bottomAccid)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why doing nothing? Via Inspector the Ambitus can get changed to a non-accidental note, why not via deleting the accidental here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because they are deleted in

Ambitus::~Ambitus()
{
    delete _topAccid;
    delete _bottomAccid;
}

They hide somehow differently (I did not investigate in detail)

Copy link
Contributor

Choose a reason for hiding this comment

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

But, using you patch, they don't disapear, so don't delete at all, unless you delete the entire Ambitus (only that would call the destructor)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had no plans to change the implementation and logic of Ambitus.
I just avoided triggering the assertion, that is, I didn’t make worse than it was, I did it a little better.
Do you think it is necessary to reconsider the implementation of Ambitus?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, doing nothing here is certainly better than a crash

return;
}

EngravingItem::remove(e);
}

//---------------------------------------------------------
// getProperty
//---------------------------------------------------------
Expand Down
2 changes: 2 additions & 0 deletions src/engraving/libmscore/ambitus.h
View file
Open in desktop
Expand Up @@ -111,6 +111,8 @@ class Ambitus final : public EngravingItem
bool readProperties(XmlReader&) override;
QString accessibleInfo() const override;

void remove(EngravingItem*) override;

// properties
mu::engraving::PropertyValue getProperty(Pid) const override;
bool setProperty(Pid propertyId, const mu::engraving::PropertyValue&) override;
Expand Down