Fix accidental revealing of items when the game is restarted after lost from a middle click.
ClosedPublic

Authored by yuyichao on Feb 28 2020, 2:56 AM.

Details

Summary

When a flag is misplaced, a middle click could cause the game to lost.
Currently, if the user choose to restart the game in this case, there is a high chance
that a few pieces around the one middle clicked are revealed right after the game restarted.

The reason is that when the user restart the game in such case,
it happens within checkLost/onItemRevealed which means that the
game might not be the same one anymore after onItemRevealed returns
and the loop handling all neighboring pieces are now operating on the now restarted game.

The fix is to make sure the loop stops when the game is finished. Checking m_gameOver is
unreliable and won't work in this case since the restart of the game would have reset it.
Instead, the finished status is returned from all functions that trigger such a condition
and the information is propagated back to the caller to terminate the loop appropriately.
This also improved another unreliable use of m_gameOver that is currently harmless.

Debugging was possible thanks to rr.

Diff Detail

Repository
R404 KMines
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
yuyichao created this revision.Feb 28 2020, 2:56 AM
Restricted Application added a reviewer: KDE Games. · View Herald TranscriptFeb 28 2020, 2:56 AM
Restricted Application added a subscriber: kde-games-devel. · View Herald Transcript
yuyichao requested review of this revision.Feb 28 2020, 2:56 AM
yuyichao edited the summary of this revision. (Show Details)Feb 28 2020, 2:59 AM

P.S. is 404 really the number of this repo?.... I seriously thought there was an error when searching for kmine here showed me R404: KMines .....

yuyichao updated this revision to Diff 76595.Feb 28 2020, 3:04 AM
yuyichao edited the summary of this revision. (Show Details)

Change comment style.

yurchor accepted this revision.Feb 28 2020, 2:50 PM

Works for me as expected.

minefielditem.cpp
442

Typo: .->,

443

Typo: are finished -> is finished

This revision is now accepted and ready to land.Feb 28 2020, 2:50 PM
yuyichao updated this revision to Diff 76643.Feb 28 2020, 3:34 PM
yuyichao edited the summary of this revision. (Show Details)

Grammar & format.

yuyichao marked 2 inline comments as done.Feb 28 2020, 3:34 PM
aacid accepted this revision.Feb 29 2020, 8:53 AM
aacid added a subscriber: aacid.

It would be better if it came with an autotest but given we have no autotests at all seems wrong to block on the autotest being added :)

Yeah I was also looking for a test and didn't find any...

Wasn't going to get myself into "trouble" by bringing it up ;-p

I'm planing to push soon assuming I can do it correctly. (Haven't done any after the move to phabricator from review board a few years ago = = ....)

This revision was automatically updated to reflect the committed changes.