fifteen puzzle game #5

Merged
SlavekB merged 7 commits from q15 into master 3 years ago
denis commented 4 years ago
Collaborator

A simple fifteen puzzle game

Signed-off-by: Denis Kozadaev denis@dilos.org

A simple fifteen puzzle game Signed-off-by: Denis Kozadaev <denis@dilos.org>
Ghost commented 4 years ago

This answers bug 1049 I guess.

This answers bug [1049](https://bugs.trinitydesktop.org/show_bug.cgi?id=1049) I guess.
denis commented 4 years ago
Poster
Collaborator

I did not convert anything from an existing code.
The program was written by a FreeBSD porter query long time ago
and I offer it to you

I did not convert anything from an existing code. The program was written by a FreeBSD porter query long time ago and I offer it to you
Owner

This answers bug 1049 I guess.

It seems unrelated to bug 1049.

Thanks Denis, looks interesting. Nice to see other people proposing new code and applications!

> This answers bug [1049](https://bugs.trinitydesktop.org/show_bug.cgi?id=1049) I guess. It seems unrelated to bug 1049. Thanks Denis, looks interesting. Nice to see other people proposing new code and applications!
Owner

@denis I had a super quick look. Would you be interested into making it a proper TDE application instead of a TQApplication? So that it would fit even more naturally into TDE? 😄

@denis I had a super quick look. Would you be interested into making it a proper TDE application instead of a TQApplication? So that it would fit even more naturally into TDE? :smile:
Owner

This answers bug 1049 I guess.

It seems unrelated to bug 1049.

Correction, it would solve bug 1049 as pointed out by Greg. Sorry for the noise :-(

> > This answers bug [1049](https://bugs.trinitydesktop.org/show_bug.cgi?id=1049) I guess. > > It seems unrelated to bug 1049. > Correction, it would solve bug 1049 as pointed out by Greg. Sorry for the noise :-(
denis commented 4 years ago
Poster
Collaborator

It is possible but in some time because I don't know KDE/TDE API at all.

It is possible but in some time because I don't know KDE/TDE API at all.
Owner

It is possible but in some time because I don't know KDE/TDE API at all.

ok, no problem at all 👍

> It is possible but in some time because I don't know KDE/TDE API at all. > ok, no problem at all :+1:
Owner

I did a quick local build test, it builds fine and it works!

Here is a list of few improvements to do in future, as reference (thanks Greg too!)

  1. prepare tde-packaging related files for DEB (MicheleC or SlavekB)
  2. merge code (MicheleC or SlavekB)
  3. convert cmake stuff to standard TDE cmake stuff (Greg)
  4. drop qt4 stuff (whoever)
  5. add .desktop file (Greg?)
  6. convert to TDE app (either Denis or MicheleC, in the long term)

Anything else I missed?

I did a quick local build test, it builds fine and it works!<br> Here is a list of few improvements to do in future, as reference (thanks Greg too!) 1. prepare tde-packaging related files for DEB (MicheleC or SlavekB) 2. merge code (MicheleC or SlavekB) 3. convert cmake stuff to standard TDE cmake stuff (Greg) 4. drop qt4 stuff (whoever) 5. add .desktop file (Greg?) 6. convert to TDE app (either Denis or MicheleC, in the long term) Anything else I missed?
Owner

I have discussed with Slavek and Greg. For the time being we will not merge this and chess (#6) application until R14.0.8 is released. After that we will decide whether to merge for R14.1.0 or in subsequent R14.1.x releases, since we want to prioritize R14.1.0 and make sure we release before the end of the year.

In the meantime, feel free to work on the required code and update these 2 PRs so that the code comes closer to the TDE standard (See points in previous comments).

I will mark the PR as WIP for the time being.

@denis thanks for making these apps available, please be patient and sooner or later they will be merged 😄

I have discussed with Slavek and Greg. For the time being we will not merge this and chess (#6) application until R14.0.8 is released. After that we will decide whether to merge for R14.1.0 or in subsequent R14.1.x releases, since we want to prioritize R14.1.0 and make sure we release before the end of the year. In the meantime, feel free to work on the required code and update these 2 PRs so that the code comes closer to the TDE standard (See points in previous comments). I will mark the PR as WIP for the time being. @denis thanks for making these apps available, please be patient and sooner or later they will be merged :smile:
MicheleC changed title from fifteen puzzle game to WIP: fifteen puzzle game 4 years ago
Ghost commented 3 years ago

For those interrested in the history of those kind of puzzles: https://en.wikipedia.org/wiki/15_puzzle

For those interrested in the history of those kind of puzzles: https://en.wikipedia.org/wiki/15_puzzle
Ghost closed this pull request 3 years ago
Ghost reopened this pull request 3 years ago
Ghost added the PR/wip label 3 years ago
Ghost added PR/rfc and removed PR/wip labels 3 years ago
Ghost commented 3 years ago

I have removed the WIP status of this PR, please test, let me know if It's good enough for a basic application.

I wished I could add a man page but I noticed that there is none for the other games in the module (tdegames), so I prefer handling this in an other branch/PR.

I'd also like to add icons (https://commons.wikimedia.org/wiki/File:15-puzzle.svg) this one is Public Domain released.
It just fits the bill in my view but I've been strugling to remove the border of the image (gimp stuff) as a result this will be done in an other PR (when I'm done if any).

I have removed the WIP status of this PR, please test, let me know if It's good enough for a basic application. I wished I could add a man page but I noticed that there is none for the other games in the module (tdegames), so I prefer handling this in an other branch/PR. I'd also like to add icons (https://commons.wikimedia.org/wiki/File:15-puzzle.svg) this one is Public Domain released. It just fits the bill in my view but I've been strugling to remove the border of the image (gimp stuff) as a result this will be done in an other PR (when I'm done if any).
Ghost changed title from WIP: fifteen puzzle game to fifteen puzzle game 3 years ago
Ghost commented 3 years ago

@denis What is the copyright for q15 source if I may ask? GPL, MIT, BSD...

@denis What is the copyright for q15 source if I may ask? GPL, MIT, BSD...
denis commented 3 years ago
Poster
Collaborator

The code was relicensed from BSD to GPL when I pushed it to TDE.

The code was relicensed from BSD to GPL when I pushed it to TDE.
Ghost commented 3 years ago

@MicheleC I've added icons, passed the app name to lower case, made the copyright clear according to Denis's comment above.

All yours to test/comment. 😉

Man page in an other branch (I wish to add all the man pages you've got in the Deb packaging).

@MicheleC I've added icons, passed the app name to lower case, made the copyright clear according to Denis's comment above. All yours to test/comment. :wink: Man page in an other branch (I wish to add all the man pages you've got in the Deb packaging).
MicheleC requested changes 3 years ago
MicheleC left a comment
Owner

Few things to adjust here and there, as commented. Nothing major, mostly good practices.

Few things to adjust here and there, as commented. Nothing major, mostly good practices.
CMakeLists.txt Outdated
option( BUILD_KTUBERLING "Build ktuberling" ${BUILD_ALL} )
option( BUILD_LSKAT "Build lskat" ${BUILD_ALL} )
option( BUILD_TWIN4 "Build twin4" ${BUILD_ALL} )
option( BUILD_FIFTEEN "Build TDEFifteenPieces" ${BUILD_ALL} )
Owner

The option and the name don't match, so it is different from the other options. Would be better to have consistency. Like BUILD_FIFTEENPIECES.

The option and the name don't match, so it is different from the other options. Would be better to have consistency. Like BUILD_FIFTEENPIECES.
Owner

BUILD_FIFTEENPIECES sounds good to me.

BUILD_FIFTEENPIECES sounds good to me.
##### Import libtdegames
##### All these games require libtdegames
Owner

Does TDEFifteenPieces requires libtdegames? Just asking :-)

Does TDEFifteenPieces requires libtdegames? Just asking :-)
##### icons
tde_install_icons( TDEFifteenPieces )
Owner

Icon names should be lower case if possible

Icon names should be lower case if possible
###### other data
tde_create_translated_desktop( TDEFifteenPieces.desktop )
Owner

the desktop file should be in lower cases like other games

the desktop file should be in lower cases like other games
GenericName=puzzle-solving game
Exec=tdefifteenpieces
Comment=sliding puzzle with one missing tile
Icon=TDEFifteenPieces
Owner

Icon names should be lower case if possible

Icon names should be lower case if possible
/*
Owner

The first 10 lines of comments don't say much other than the author. We could make a single line. The rest is no use and waste time to read and realize they are no use.

// Author: Denis Kozadaev (denis@tambov.ru)

Same applies to the other source files.

The first 10 lines of comments don't say much other than the author. We could make a single line. The rest is no use and waste time to read and realize they are no use. ``` // Author: Denis Kozadaev (denis@tambov.ru) ``` Same applies to the other source files.
SlavekB reviewed 3 years ago
SlavekB left a comment
Owner

Most of the comments have already been made by @MicheleC

For me, a note that a commit with renaming to lowercase could be a squashed with a commit for a change to a TDE application, so that there is no unnecessarily double renaming within a single PR.

Most of the comments have already been made by @MicheleC For me, a note that a commit with renaming to lowercase could be a squashed with a commit for a change to a TDE application, so that there is no unnecessarily double renaming within a single PR.
Name=TDEFifteenPieces
GenericName=puzzle-solving game
Exec=tdefifteenpieces
Comment=sliding puzzle with one missing tile
Owner

GenericName and Comment could start with the first capital letter.

GenericName and Comment could start with the first capital letter.
Owner

Oh yes, I also spotted that. I must have forgot to "save" the comment I made. Thanks Slavek!

Oh yes, I also spotted that. I must have forgot to "save" the comment I made. Thanks Slavek!
Ghost removed the PR/rfc label 3 years ago
Owner

Looks good to me now. Ok to merge?

Looks good to me now. Ok to merge?
MicheleC approved these changes 3 years ago
MicheleC left a comment
Owner

LGTM 👍

LGTM :+1:
SlavekB merged commit dd4e287280 into master 3 years ago
SlavekB deleted branch q15 3 years ago
SlavekB added this to the R14.0.10 release milestone 3 years ago

Reviewers

MicheleC approved these changes 3 years ago
The pull request has been merged as dd4e287280.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
4 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TDE/tdegames#5
Loading…
There is no content yet.