Differential D20802

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

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

Details

Reviewers

Group Reviewers

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.Apr 25 2019, 8:35 AM

Restricted Application added a reviewer: KDE Games. · View Herald TranscriptApr 25 2019, 8:35 AM

Restricted Application added a subscriber: kde-games-devel. · View Herald Transcript

krippendorf requested review of this revision.Apr 25 2019, 8:35 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

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.Apr 26 2019, 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.Apr 26 2019, 6:07 AM

In D20802#456368, @aacid wrote:

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.

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

schwarzer added a subscriber: schwarzer.Apr 26 2019, 9:06 AM

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.Apr 26 2019, 11:16 AM

mlaurent added inline comments.

src/gamebackground.cpp
56	const QBrush &background

This revision now requires changes to proceed.Apr 26 2019, 11:16 AM

krippendorf updated this revision to Diff 57129.Apr 28 2019, 1:16 PM

krippendorf marked 2 inline comments as done.

for me it seems ok.

This revision is now accepted and ready to land.Apr 29 2019, 4:45 AM

krippendorf closed this revision.May 5 2019, 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.

Revision Contents
Changeset List

		Path
M		.gitignore (1 line)
M		src/CMakeLists.txt (2 lines)
A	M	src/gamebackground.h (87 lines)
A	M	src/gamebackground.cpp (68 lines)
M		src/gameitem.cpp (20 lines)
A	M	src/gameremovedtiles.h (139 lines)
A	M	src/gameremovedtiles.cpp (221 lines)
M		src/gamescene.h (22 lines)
M		src/gamescene.cpp (64 lines)
M		src/gameview.h (20 lines)
M		src/gameview.cpp (178 lines)
M		src/kmahjongg.cpp (3 lines)
M		src/kmahjongg.kcfg (4 lines)
M		src/settings.ui (7 lines)