Fix sometimes two moves being counted as one
AcceptedPublic

Authored by aacid on Wed, Feb 12, 12:02 AM.

Details

Reviewers
fabiank
Group Reviewers
KDE Games
Summary

BUGS: 416697

Diff Detail

Repository
R410 KPatience
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 22380
Build 22398: arc lint + arc unit
aacid created this revision.Wed, Feb 12, 12:02 AM
Restricted Application added a reviewer: KDE Games. · View Herald TranscriptWed, Feb 12, 12:02 AM
Restricted Application added a subscriber: kde-games-devel. · View Herald Transcript
aacid requested review of this revision.Wed, Feb 12, 12:02 AM

I'm not really sure about this one, it does indeed fix the bug, but i'm not sure if it breaks something else :/

Opinions?

So I think this depends in part on whether we want to have an atomic undo/redo for automatic moves. With your change, every single move in the auto move will be put on the undo stack. The behaviour before was that they were all composed into one "big" move.
If we want to keep the existing behaviour, we would need to modify DealerScene::moveCount() to changes.size() of the undo stack elements and m_currentState into account.

I'm also not sure if auto moves count in all games. So if we decide to keep the current undo behaviour, maybe moveCount should also be virtual, with most classes using the existing implementation, and Freecell doing some special accounting.

aacid added a comment.Wed, Feb 12, 6:58 PM

The behaviour before was that they were all composed into one "big" move.

Not really, start freecell game 9998, it counts 5 moves as 4, only the first 2 are "counted together"

Not really, start freecell game 9998, it counts 5 moves as 4, only the first 2 are "counted together"

Hm, you're right. Now I'm not sure what the intended behaviour is. I've created D27432 as a potential alternative, which preserves the current behaviour, but I wouldn't oppose your change here. As you, I'm slightly worried that this change might break other things, but after playing around with it for a while, I haven't seen anything broken so far.

FWIW i just tried what Microsoft Solitaire does and it let's you undo autodrops one by one, so i'd say that's a +1 for doing it this way and not the other?

fabiank accepted this revision.Sun, Feb 16, 11:15 PM

Indeed, that is a strong indicator to use this approach

This revision is now accepted and ready to land.Sun, Feb 16, 11:15 PM