Launch Timewise Input Mode causes crash

Yes, exactly that change caused this. More precisely, this code. I changed the code there to deal better with gaps and full-measure rests but my check on full measure rest presence is apparently incorrect for the case when note gets inserted to the beginning of measure since segments have already shifted ticks to the moment this check takes place. Maybe that line can be replaced with a call to Measure::first() like it is done inside Measure::isFullMeasureRest() (which I could not use without changes because that function performs this check for all staves unlike it is needed in note input context). Still this needs to be checked, I'll do that tomorrow if nobody takes this to that moment.

yes, can reproduce by inputting a note by clicking on the score. I get:

Exception thrown: read access violation.
Ms::Measure::findFirst(...) returned nullptr.
If there is a handler for this exception, the program may be safely continued.

that is from this line 579 in Score::localInsertChord():
Element* maybeRest = msMeasure->findFirst(SegmentType::ChordRest, /* rel. tick */ 0)->element(track);

I see dmitis made suggestion to change msMeasure->findFirst(SegmentType::ChordRest, /* rel. tick */ 0) into msMeasure->first()...but I'm still confused why this findFirst call is failing...

somehow the _tick for the first() element is 480...I would think _tick should be 0 for first().

Following dmitri's suggestion to change to just first() does seem to work. But I'm still a bit perplexed why the current version of the line doesn't work. Surely the tick of the first element should be 0???

I see what is happening. There is a prior call to InsertTime...which I guess is inserting 480 ticks to all segments in that measure to make space. And so findFirst fails to grab the first segment because the first segment is no longer at tick 0.

And that is basically what dmitiro was saying in his earlier comment.

So simply changing that line to msMeasure->first(SegmentType::ChordRest) doesn't seem to be quite correct...because if I try to insert note on a multi-measure rest, then get a crash in Score::setNoteRest line 606 call to segment->tick() because segment is NULL.

Exception thrown at 0x00007FF680A570BF in MuseScore3.exe: 0xC0000005: Access violation reading location 0x0000000000000028.

> MuseScore3.exe!Ms::Score::setNoteRest(Ms::Segment * segment, int track, Ms::NoteVal nval, Ms::Fraction sd, Ms::Direction stemDirection, bool rhythmic) Line 606 C++
MuseScore3.exe!Ms::Score::putNote(const Ms::Position & p, bool replace) Line 401 C++
MuseScore3.exe!Ms::Score::localInsertChord(const Ms::Position & pos) Line 591 C++
MuseScore3.exe!Ms::Score::insertChord(const Ms::Position & pos) Line 541 C++
MuseScore3.exe!Ms::Score::putNote(const QPointF & pos, bool replace, bool insert) Line 300 C++
MuseScore3.exe!Ms::ScoreView::mousePressEvent(QMouseEvent * ev) Line 401 C++

So I guess need to also check if first segment is a multi-measure rest in addition to just being a full-measure rest.

So the problem with inserting at beginning of multi-measure rest is that the targetMeasureLen gets set to the total duration of the entire multi-measure rest. So maybeRest turns into a very long rest. Which is not what I think want to do.

It seems this code needs to be added with special handling for multi-measure rests...I guess by first breaking up the multi-measure rest into an initial whole measure and then into a multi-measure rest for the subsequent measures (or if just one subsequent measure a whole measure rest). And then can perform the insertion.

Fixed with dmitirio's original suggestion...which seems to work. Although it doesn't fix the problem when at the start of multi-measure rest.
And I couldn't figure out how to solve that, so I've disabled that ability. Here is my PR: https://github.com/musescore/MuseScore/pull/4458

If someone (dmitrio?) knows better way to handle insert at start of mmrest, then please reject my PR and do the correct implementation.

Tested with my PR and was able to erroneously insert a note at start without having a rest:

There should be a rest before that first note on first staff.

Nope...my pr doesn't fix it correctly...it's not converting the measure rest into it's corresponding duration rest...that is how I produced that erroneous insert.

I've given up for today. Someone else can probably do better. There are basically 3 problems with current code:

1. Previous code crashed when doing a timewise insert at the beginning of a measure. This was because the call to msMeasure->findFirst searches for a segment at tick 0, but segment ticks had already been shifted forward in time with the earlier call to InsertTime. Using msMeasure->first(SegmentType::ChordRest) avoids that problem.

2. Crash when inserting to beginning of a multi-measure rest. This was fixed by testing for isMMRest() and using the multi-measure rest if true.

3. But there was a third problem not discussed and I don't think people have noticed...which is that current code leaves measure rest elements around after inserting new elements before a measure rest. These measure rests will need to be changed to an equivalent set of normal rests. However unfortunately I don't know enough about how to do this with the undo stack because I don't know the undo stack protocol. My best solution is to first convert all measure-rests into normal (non-measure) rests(s) of equivalent duration first before inserting any additional space (https://github.com/ericfont/MuseScore/commit/6481023d05f595067916b91caa…) but unfortunately if user performs undo then I'm left with invalid (empty) measures.

I expanded the fix developed by @ericfontainejazz to fix some most prominent remaining issues, see https://github.com/musescore/MuseScore/pull/4462.

Fixed in branch master, commit 31a6aafaca

fix #280364: measure start timewise input

Previous code crashed when doing a timewise insert at the beginning of
a measure. This was because the call to msMeasure->findFirst searches
for a segment at tick 0, but segment ticks had already been shifted
forward in time with the earlier call to InsertTime. Using
msMeasure->first(SegmentType::ChordRest) avoids that problem.

Second problem was a crash when inserting to beginning of a
multi-measure rest. This was fixed by testing for isMMRest() and using
the multi-measure rest if true.

I also had to change behavior to convert all measure-rests into normal
(non-measure) rests(s) of equivalent duration first before inserting
any additional space. Unfortunately it would have been ideal to
simply keep measure rests and exapand them to fill the new space, but
I couldn't figure out a clean way to do that. So I opted for the
simpler and safer method of converting them into...

Additional note from dmitrio95:
Added handling of situations where other tracks contain non-measure
rests or notes overlapping with the input area. Ensured that all
changes are undoable.

Fixed in branch master, commit 12d0e18b5d

Merge pull request #4462 from dmitrio95/280364-timewise-input-crash

fix #280364: measure start timewise input

Automatically closed -- issue fixed for 2 weeks with no activity.