Jojo-Schmitz / MuseScore

MuseScore is a open source and free music notation software. For support, contribution, bug reports, visit MuseScore.org. Fork and make pull requests!
http://musescore.org
Other
36 stars 3 forks source link

Fix #414: Crash when re-applying time signature to parts (out of bounds) #637

Closed worldwideweary closed 2 weeks ago

worldwideweary commented 2 weeks ago

Resolves: #414

Test it out, seems to work okay

worldwideweary commented 2 weeks ago

Obviously I'm not spot on because there's

27 - tst_measure (Failed)

... don't have local testing here though so not sure what that is yet

worldwideweary commented 2 weeks ago

What's the deal with the failing mtest?

I dunno man. I don't know much about the unit testing... so I'm curious myself. I don't see how something that is out of bounds could be "appropriate" as if that would alter some results, so maybe it's having to do with using the "score" instead of this pointer

worldwideweary commented 2 weeks ago

There's the defined constructor which this code calls (takes a score pointer), and the old one which used the "default" constructor which doesn't have any code here (but can of course take a pointer to its own type) ... the one this uses and the only one showing in the codebase does:

TimeSig::TimeSig(Score* s)
  : Element(s, ElementFlag::ON_STAFF | ElementFlag::MOVABLE)
      {
      initElementStyle(&timesigStyle);

      _showCourtesySig = true;
      _stretch.set(1, 1);
      _sig.set(0, 1);               // initialize to invalid
      _timeSigType      = TimeSigType::NORMAL;
      _largeParentheses = false;

So maybe these default attributes are not in accord with the default construction in some way that alters the mtest results... like maybe the styles are diferent from initElementStyle, or showcourtesy sig is false or something

Jojo-Schmitz commented 2 weeks ago
  _sig.set(0, 1);               // initialize to invalid

Esp. this, leads to:

--- /home/runner/work/MuseScore/MuseScore/mtest/libmscore/measure/measure-6-ref.mscx    2024-09-12 10:58:03.497486946 +0000
+++ measure-6.mscx  2024-09-12 11:19:15.121886888 +0000
@@ -59,8 +59,10 @@
       <Measure>
         <voice>
           <TimeSig>
-            <sigN>4</sigN>
-            <sigD>4</sigD>
+            <sigN>0</sigN>
+            <sigD>1</sigD>
+            <stretchN>0</stretchN>
+            <stretchD>1</stretchD>
             </TimeSig>
           <Chord>
             <durationType>quarter</durationType>
Jojo-Schmitz commented 2 weeks ago

Here's what that mtest does:

void TestMeasure::spanner_A()
      {
      MasterScore* score = readScore(DIR + "measure-6.mscx");

      score->select(score->firstMeasure());
      score->startCmd();
      score->localTimeDelete();
      score->endCmd();
      score->doLayout();
      QVERIFY(saveCompareScore(score, "measure-6.mscx", DIR + "measure-6-ref.mscx"));
      delete score;
      }

I don't claim to understand this test... Esp. how this test differs from 5, 6, 7, 8 and 9

worldwideweary commented 2 weeks ago

You know what, I think I see what the problem is already... The results of the dude's score gives:

image

0/1

Something's wrong elsewhere... but it still doesn't make sense yet

worldwideweary commented 2 weeks ago

Oh, maybe it's the stretch: nts->setStretch(nts->sig() / mAfterSel->timesig());

nts->sig() is whatever was defaulted. I'll set it to the lastDeletedForThisStaff and see how the tests go

worldwideweary commented 2 weeks ago

OK, hopefully that takes care of it. If there are any problems in the future having to do with this, alternatively the code could just use the "this" pointer like before but then specifically setScore(score) afterwards.