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.

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

Fix automation crash (#6711) #6718

Merged
merged 5 commits into from
Jun 2, 2023

Conversation

michaelgregorius
Copy link
Contributor

Fix for the crash that occurs when the zooming or snapping model of the Song Editor is used for automation.

Fix a crash that occurs if the zooming or snapping model of the Song
Editor is used for automation.

The crash is caused by a failed static_cast to a Model* in
Model::parentModel(). The cast fails because in the constructor of
SongEditor the SongEditor is set as the parent of the zooming and
snapping model:
m_zoomingModel->setParent(this);
m_snappingModel->setParent(this);

This commit is rather a "band aid" fix because it only fixes the crash
by changing the static_cast to a dynamic_cast. However this means that
the name of the automation clip is initially empty.

A better solution would be to not use the Qt parent/child relationships
to implement the model hierarchies.
Move the implementation of the Model methods into Model.cpp so that
recompiles after changes are much quicker. Make Model::
isDefaultConstructed const.
@superpaik
Copy link
Contributor

Coding style: I think you should remove underscore on function parameters.
Other than that it works ok.

Remove underscores and whitespace in the Model files.
Simplify the logic of the method Model::fullDisplayName.

Please note that this shows that with the current logic if the parent
model has a non-empty name like "parentmodel" and the name of the child
model is empty the result is the name "parentmodel >" which might not be
what's wanted.
@michaelgregorius
Copy link
Contributor Author

Underscores and whitespace has been removed with commit fda938f.

I have also simplified the logic of Model::fullDisplayName with commit 9fe7fbd. This might also have made more obvious a potential problem if the parent has a non-empty name "xyz" but the child has an empty one. This will result in the name "xyz >" being returned.

Copy link
Member

@messmerd messmerd left a comment

Choose a reason for hiding this comment

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

Just some very minor style suggestions.
Everything else looks good to me and it worked when I tested it. I've also confirmed that the logic you simplified is still correct.

include/Model.h Outdated Show resolved Hide resolved
src/core/Model.cpp Outdated Show resolved Hide resolved
src/core/Model.cpp Outdated Show resolved Hide resolved
src/core/Model.cpp Outdated Show resolved Hide resolved
src/core/Model.cpp Outdated Show resolved Hide resolved
More whitespace adjustments as proposed in the code review.
@michaelgregorius
Copy link
Contributor Author

Thanks for the code review and double check @messmerd! I have implemented all your proposals with commit 0ebc189.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants