Add a layer for showing the removed tiles of the game.
ClosedPublic

Authored by krippendorf on Thu, Apr 25, 8:35 AM.

Details

Reviewers
mlaurent
Group Reviewers
KDE Games
Summary

This patch adds an extra layer showing the removed tiles of a kmahjongg game. It only shows a fitting number of last removed tiles.

The feature can be en/disabled in the general settings dialog. The undo operation works for removing tiles.

The style is plain without 3D functionality, to keep the focus on the main board layout.

Test Plan

Just give it a try.

Diff Detail

Repository
R403 KMahjongg
Lint
Lint Skipped
Unit
Unit Tests Skipped
krippendorf created this revision.Thu, Apr 25, 8:35 AM
Restricted Application added a reviewer: KDE Games. · View Herald TranscriptThu, Apr 25, 8:35 AM
Restricted Application added a subscriber: kde-games-devel. · View Herald Transcript
krippendorf requested review of this revision.Thu, Apr 25, 8:35 AM
aacid added a subscriber: aacid.Thu, Apr 25, 9:41 PM

Is there any chance you could use arc in the future to upload the patches? Makes for easier reviewing since instead of the "Context not available" you get the option to extend the code to read up/down

When starting a new game sometimes it goes wonky
Before pressing new game https://i.imgur.com/AWZAb7o.jpg
After pressing new game https://i.imgur.com/m9Talax.jpg

src/gamebackground.cpp
55

remove commented line?

src/gamebackground.h
49

const QBrush

81

Any reason this is a pointer and not just a QBrush?

src/gamescene.cpp
85

don't do yoda comparisons (unless it's the style in the file, which i think it's not)

mlaurent requested changes to this revision.Fri, Apr 26, 6:07 AM
mlaurent added a subscriber: mlaurent.
mlaurent added inline comments.
src/gameremovedtiles.cpp
140

m_itemFaces->size() <= 0 => isEmpty()

src/gameview.cpp
636

compare old version and new version to avoid to updateItemsPosition() all the time.

src/gameview.h
83

don't const'ref for bool

This revision now requires changes to proceed.Fri, Apr 26, 6:07 AM

Is there any chance you could use arc in the future to upload the patches? Makes for easier reviewing since instead of the "Context not available" you get the option to extend the code to read up/down

For the next patch/feature I will give it a try with arc.

krippendorf marked 7 inline comments as done.

Update all comments including the 'sometimes bad size on new game aacid'.

schwarzer added inline comments.
src/gameview.cpp
70–81

This looks weird indentation-wise. Is that just here in phabricator or did you use tabs for indentation?

156–168

Just a quick glance ... but it might be possible to const'ify many of the variables in this function.

mlaurent requested changes to this revision.Fri, Apr 26, 11:16 AM
mlaurent added inline comments.
src/gamebackground.cpp
56

const QBrush &background

This revision now requires changes to proceed.Fri, Apr 26, 11:16 AM
krippendorf marked 2 inline comments as done.
mlaurent accepted this revision.Mon, Apr 29, 4:45 AM

for me it seems ok.

This revision is now accepted and ready to land.Mon, Apr 29, 4:45 AM
krippendorf closed this revision.Sun, May 5, 5:37 PM
krippendorf added inline comments.
src/gameview.cpp
156–168

Yes, this might be possible. But I would like to implement this in a separate task not here in this feature.