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

ColorLabel::setColor should emit colorChanged #6005

Merged
merged 1 commit into from
May 2, 2020

Conversation

SKefalidis
Copy link
Contributor

@SKefalidis SKefalidis commented Apr 28, 2020

Resolves: none
Related to: #5922

ColorLabel should emit a colorChanged signal in setColor ( required for PR #5922 )
Without the signal blocks, MuseScore crashes.

  • I signed CLA
  • I made sure the code in the PR follows the coding rules
  • I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code
  • I made sure the title of the PR reflects the core meaning of the issue you are solving
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
  • I made sure the commit message title starts with "fix #424242:" if there is a related issue
  • [n\a] I created the test (mtest, vtest, script test) to verify the changes I made

@@ -43,8 +43,11 @@ ColorLabel::~ColorLabel()

void ColorLabel::setColor(const QColor& c)
{
bool changed = _color != c;
Copy link
Contributor

Choose a reason for hiding this comment

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

const bool please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you wish :-)

@anatoly-os
Copy link
Contributor

@SKefalidis

Without the signal blocks, MuseScore crashes.

Why? Fixing mysterious crashes by adding random checks is not the best way of writing a software :)

Do you understand the nature of the crash happening?

@anatoly-os anatoly-os added the work in progress not finished work or not addressed review label Apr 29, 2020
@SKefalidis
Copy link
Contributor Author

SKefalidis commented Apr 29, 2020

@anatoly-os The crash happens because MixerDetails::trackColorChanged is called which in turn calls _mti->setColor(col.rgb()); but _mti is empty at this stage.

There are 2 ways to solve this, either add a check in MixerDetails::trackColorChanged (it will also reduce the file size by a few lines) or do what I did (which is consistent with the rest of the file, if you look around every trackColorLabel->setColor is surrounded by instructions to temporarily block signals).

I chose the second way to make sure that nothing breaks (even though the first way should not break something either).

@anatoly-os anatoly-os removed the work in progress not finished work or not addressed review label May 2, 2020
@anatoly-os anatoly-os merged commit 91fe56c into musescore:master May 2, 2020
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.

3 participants