Add Icon for Rocksndiamonds #955

dash102 · 2016-12-25T03:40:02Z

No description provided.

fbrosson

Thanks @dash102 for this PR!

minor fix for INSTALL.
suggestions for the rdef file.

fbrosson · 2016-12-25T08:55:08Z

games-arcade/rocksndiamonds/rocksndiamonds-3.3.1.2.recipe

@@ -56,6 +56,9 @@ BUILD()

 INSTALL()
 {
+	addResourcesToBinaries $portDir/additional-files/rocksndiamonds.rdef \
+		$binDir/rocksndiamonds


I haven't tested it yet, but it looks like you need to move these two lines:

addResourcesToBinaries $portDir/additional-files/rocksndiamonds.rdef \ $binDir/rocksndiamonds

to the end of the INSTALL function (or at least after the call to cp) and replace, in the second line, the $binDir by $destdir:

addResourcesToBinaries $portDir/additional-files/rocksndiamonds.rdef \ $destdir/rocksndiamonds

BTW, you also need to add

ADDITIONAL_FILES=" rocksndiamonds.iom rocksndiamonds.rdef "

after the PATCHES line.

$binDir/rocksndiamonds isn't correct here, it should be:
$appsDir/"Rocks'n'Diamonds"/rocksndiamonds

PS @fbrosson does the iom file need to go into the ADDITIONEL_FILES? as it's not needed for the build ...

fbrosson · 2016-12-25T08:55:08Z

games-arcade/rocksndiamonds/additional-files/rocksndiamonds.rdef

+resource app_version {
+	major = 0,
+	middle = 0,
+	minor = 0,


It might be a good idea to replace

major = 0, middle = 0, minor = 0,

by

major = 3, middle = 3, minor = 1,

but that's not very important.
BTW, in a few of our recipes, e.g. media-video/vlc/vlc-2.2.1.recipe or net-p2p/qbittorrent/qbittorrent-3.3.4.recipe, we have added a few extra steps to replace the hard-coded values (in major, middle and minor) by a pattern which we edit on the fly when we create a copy of rdef with these patterns replaced by the correct values.
It is not worth doing that for programs that do not get new releases, but it might be usefull the programs gets frequent updates.
I'm suggesting this in case other contributors think it could be a good idea.

Oops, wait, the game did not change since 2013, so using hard-coded values will be perfect. No need to use patterns. Sorry for the noise. (Keeping the message as you might find it usefull for other recipes ;-)

Hmm, Rocks'n'Diamonds 4.0.0.0 was released two days ago ;-)
So I guess it can be a good idea to use avoid hard-coded values.
This way, when we update the game, we won't need to edit the rdef file.

BTW, if you wish to update the game, you can do it in this task, or in a new task that we can create.

looks like I needed to submit my comments before they are shown ;)

PS, I took some time to create one for the 4.0.0.0.rc3 a while back, maybe that could be used as a base? IIRC there were some isseus with that one, hence why I didn't creat a pr back then :)
https://github.com/Begasus/haikuports/tree/rocksndiamonds

Yep, I was a little bit puzzled, because I could not find your comments ;-)
BTW, when I started writing my review your message on GCI was not there yet; And I only saw it after I submitted my message about my review. So this was a big coincidence!

@dash102; as you can see, we are all happy mentors!

@Begasus, how come did I forget about #745 !
Maybe we could create a task for providing feedback to your PR and make it build 4.0.0.0 or fixing the remaining issues in your recipe and updating it to 4.0.0.0.
Well, this is just a suggestion. I'll let you create the task if you wish ;-)

@fbrosson looks like I did file a reques for it then, but yes, it seems like a task in the making :-)

fbrosson · 2016-12-25T08:55:08Z

games-arcade/rocksndiamonds/additional-files/rocksndiamonds.rdef

+	internal = 0;
+
+	short_info = "",
+	long_info = ""


You might wish to replace

short_info = "", long_info = ""

by

short_info = "Rocks'n'Diamonds", long_info = "An arcade style game by Artsoft Entertainment"

I'm not sure if the string I'm suggesting for long_info is consistent with what is expected.
Maybe it should be "Copyright 2013, Artsoft Entertainment". Any ideas?

That seems to be what is wanted, based on what I've seen in other rdef files :)

It looks like some rdefs have short descriptions, some others have short copyright notices.
What about:

short_info = "Rocks'n'Diamonds", long_info = "Rocks'n'Diamonds, an arcade style game.\n©2001-2013 Artsoft Entertainment"

Begasus · 2016-12-25T09:28:11Z

games-arcade/rocksndiamonds/additional-files/rocksndiamonds.rdef

+	major = 0,
+	middle = 0,
+	minor = 0,
+


You could add the version for rocksndiamonds here, check out other files like for instance scummvm, tuxmmath

Begasus · 2016-12-25T09:28:11Z

games-arcade/rocksndiamonds/rocksndiamonds-3.3.1.2.recipe

@@ -56,6 +56,9 @@ BUILD()

 INSTALL()
 {
+	addResourcesToBinaries $portDir/additional-files/rocksndiamonds.rdef \
+		$binDir/rocksndiamonds
+


Move this section just before addAppDeskbarSymlink, and add ADDITIONAL_FILES="" to the recipe for the rdef file, check the generic app file in haikuporters tree to see where they should go to

humdingerb · 2016-12-25T09:53:38Z

games-arcade/rocksndiamonds/additional-files/rocksndiamonds.rdef

@@ -0,0 +1,39 @@
+// Rocks 'n' Diamonds icon by Stephanie Fu
+
+resource app_signature "application/x-vnd.rocksndiamonds";


To be 100% totally correct, the app sig should be in the format: application/x-vnd.vendor-appname.

Begasus · 2016-12-25T10:11:51Z

games-arcade/rocksndiamonds/additional-files/rocksndiamonds.rdef

+	minor = 0,
+
+	variety = B_APPV_DEVELOPMENT,
+	internal = 0;


this should be "," instead of ";"

(I'm replying here because GitHub is not showing the "Reply" button on your next comment for some unknown reason to me.)

$binDir/rocksndiamonds isn't correct here, it should be:
$appsDir/"Rocks'n'Diamonds"/rocksndiamonds

Yep, I know. That's why I suggested using this in my review:

addResourcesToBinaries $portDir/additional-files/rocksndiamonds.rdef \ $destdir/rocksndiamonds

PS @fbrosson does the iom file need to go into the ADDITIONEL_FILES? as it's not needed for the build ...

Exact. If it's not needed for the build (or install), then we don't need it in ADDITIONEL_FILES.
But wait, where is that file used, if it's not required in the recipe?
Should it be part of the package? If yes then we need to add steps for it (and keep it in ADDITIONEL_FILES, of course.)

ah ok, so $destdir can be used also :) (overlooked that one)
the iom file isn't used by haikuporter, but could be handy if someone else in the future would try to enhance or change it (if needed) :)

Begasus · 2016-12-25T10:11:51Z

games-arcade/rocksndiamonds/rocksndiamonds-3.3.1.2.recipe

@@ -56,6 +56,9 @@ BUILD()

 INSTALL()
 {
+	addResourcesToBinaries $portDir/additional-files/rocksndiamonds.rdef \
+		$binDir/rocksndiamonds


$binDir/rocksndiamonds isn't correct here, it should be:
$appsDir/"Rocks'n'Diamonds"/rocksndiamonds

PS @fbrosson does the iom file need to go into the ADDITIONEL_FILES? as it's not needed for the build ...

Begasus · 2016-12-25T10:33:14Z

PS, the original recipe also needs a new patch for gcc5, I already linked Stephanie yesterday on IRC to the sollution, it just needs to be added :)

Begasus · 2016-12-25T10:34:59Z

@dash102 the icon looks nice +1

dash102 · 2016-12-25T15:47:09Z

Wow, thanks for all the feedback! Glad you liked the icon :D
I'll make these changes today - is the patch something I should look into doing also for this task?
Edit: Oh wait, I think that is a separate task, I'll see if I can figure out patch-writing after I make these icon changes XD

Begasus · 2016-12-25T20:51:00Z

@fbrosson I guess this is where you best step in, probably needs to revert some changes :) (as I did in the past) :)

fbrosson · 2016-12-25T21:04:47Z

games-arcade/rocksndiamonds/rocksndiamonds-3.3.1.2.recipe

@@ -59,5 +62,9 @@ INSTALL()
 	destdir=$appsDir/"Rocks'n'Diamonds"
 	mkdir -p $destdir
 	cp -r rocksndiamonds sounds graphics levels music $destdir
+
+	addResourcesToBinaries $portDir/additional-files/rocksndiamonds.rdef \


Begasus · 2016-12-25T21:07:45Z

games-arcade/rocksndiamonds/additional-files/rocksndiamonds.rdef

+
+	variety = B_APPV_DEVELOPMENT,
+	internal = 0,
+


Maybe you could replace this one also as:
internal = @Internal@

@Begasus, there's something strange about internal. Why do we put it after variety and not after minor in the rdef file?
In other words, are you sure we can use it the way you suggest?

Begasus · 2016-12-25T21:07:45Z

games-arcade/rocksndiamonds/additional-files/rocksndiamonds.rdef

+resource app_signature "application/x-vnd.Artsoft-rocksndiamonds";
+
+resource app_flags B_EXCLUSIVE_LAUNCH;
+


Any reason here to use the exclusive launch?

I would say that if the game is already running (but got minimized or is not visible) then it would be nice to switch to it instead of launching a new instance.

Begasus · 2016-12-25T21:07:45Z

games-arcade/rocksndiamonds/rocksndiamonds-3.3.1.2.recipe

@@ -59,5 +62,9 @@ INSTALL()
 	destdir=$appsDir/"Rocks'n'Diamonds"
 	mkdir -p $destdir
 	cp -r rocksndiamonds sounds graphics levels music $destdir
+


Hmm, I did not know that internal could be used like this.
That's a good idea!

I first tried it by adding "2" in the rdef file, then checked it against FileTypes afterwords to see it worked, then I changed the line according to the previous lines that you handed to us in the past 👍

fbrosson

Maybe it could be nice to:

cd games-arcade/rocksndiamonds/additional-files
git mv rocksndiamonds.rdef rocksndiamonds.rdef.in
git commit --amend rocksndiamonds.rdef rocksndiamonds.rdef.in

and do the changes suggested in this small review?

fbrosson · 2016-12-25T21:23:57Z

games-arcade/rocksndiamonds/rocksndiamonds-3.3.1.2.recipe

@@ -15,6 +15,9 @@ REVISION="2"
 SOURCE_URI="http://www.artsoft.org/RELEASES/unix/rocksndiamonds/rocksndiamonds-3.3.1.2.tar.gz"
 CHECKSUM_SHA256="c117c20026299c6c935bd531ef9b0dc767731f600881d12ceb80c831483755f3"
 PATCHES="rocksndiamonds-$portVersion.patchset"
+ADDITIONAL_FILES="
+	rocksndiamonds.rdef


Shall we replace

ADDITIONAL_FILES=" rocksndiamonds.rdef "

by

ADDITIONAL_FILES=" rocksndiamonds.rdef.in "

fbrosson · 2016-12-25T21:27:31Z

games-arcade/rocksndiamonds/rocksndiamonds-3.3.1.2.recipe

+		-e "s|@MIDDLE@|$MIDDLE|" \
+		-e "s|@MINOR@|$MINOR|" \
+		-e "s|@INTERNAL@|$INTERNAL|" \
+		$portDir/additional-files/rocksndiamonds.rdef > rocksndiamonds.rdef


Shall we replace

$portDir/additional-files/rocksndiamonds.rdef > rocksndiamonds.rdef

by

$portDir/additional-files/rocksndiamonds.rdef.in > rocksndiamonds.rdef

(I had attached this comment to the wrong line in my previous review, so I removed it and added a single message, on the right line.)

fbrosson · 2016-12-25T22:13:30Z

@waddlesplash, something strange happened with this PR. The merge does not correspond to what we were reviewing.

And when I look at the commits in branch "b2" that was being used for this PR, https://github.com/dash102/haikuports/commits/b2 I don't see anything about rocksndiamonds recipe or rdef.

@waddlesplash, could it be that you clicked merge just when @dash102 was updating the PR?

@dash102, please do not delete you local branch, as it seems all your work about rocksndiamonds has disappeared on GitHub. I get "No changes to show." and "This commit has no content." whenever I try to browse any of the commits of this PR.

dash102 · 2016-12-25T22:20:14Z

Thanks to @Begasus I got my local changes back, ready to push when this pr is reopened :)

fbrosson · 2016-12-25T22:30:09Z

I really don't know what happened. I don't even know if @waddlesplash really tried to merge this PR.
I think he did, but I suspect that his merge and your update happened simultaneously at 9:45 PM.
And I think it won't be possible to reopen this PR. So I guess the best thing to do is open a new PR.

BTW, @waddlesplash, I think this is the first time I see so many PRs being merged so quickly ;-)

dash102 · 2016-12-25T22:32:20Z

On irc @waddlesplash said he didn't merge the pr, github did something weird. He also said it could be reopened, if I understood correctly. Should I wait for that, or should I open a new pr?

fbrosson · 2016-12-25T22:35:14Z

That's funny, did you notice the message next to the purple "Merged" above?
It says "waddlesplash merged 0 commits into haikuports:master from dash102:b2"
Yes, zero commits.

So I think the merge took place just after GitHub removed the previous top commit (in your remote branch), and before it could receive the new top commit from your local branch.

The good news is that the repo is perfectly OK.

fbrosson · 2016-12-25T22:36:44Z

I would wait. That will be fun to see if GitHub can allow us to reopen this PR ;-)

waddlesplash · 2016-12-26T00:25:50Z

@waddlesplash, could it be that you clicked merge just when @dash102 was updating the PR?

Nope, I didn't do anything.

waddlesplash · 2016-12-26T00:26:41Z

I would wait. That will be fun to see if GitHub can allow us to reopen this PR ;-)

No, @dash102 needs to push first and then reopen the PR.

dash102 · 2016-12-26T00:54:37Z

Pushed :D how can I reopen?

waddlesplash · 2016-12-26T03:29:03Z

Looks like GitHub is confused :/. Just make a new PR.

dash102 changed the title ~~Add Icon~~ Add Icon for Rocksndiamonds Dec 25, 2016

fbrosson reviewed Dec 25, 2016

View changes

Begasus reviewed Dec 25, 2016

View changes

humdingerb reviewed Dec 25, 2016

View changes

Begasus reviewed Dec 25, 2016

View changes

fbrosson reviewed Dec 25, 2016

View changes

Begasus reviewed Dec 25, 2016

View changes

fbrosson reviewed Dec 25, 2016

View changes

dash102 closed this Dec 25, 2016

waddlesplash merged commit 03b4f03 into haikuports:master Dec 25, 2016

		@@ -0,0 +1,39 @@
		// Rocks 'n' Diamonds icon by Stephanie Fu

		resource app_signature "application/x-vnd.rocksndiamonds";

		resource app_signature "application/x-vnd.Artsoft-rocksndiamonds";

		resource app_flags B_EXCLUSIVE_LAUNCH;

haikuports / haikuports

Join GitHub today

Add Icon for Rocksndiamonds #955

Add Icon for Rocksndiamonds #955

Conversation

dash102 commented Dec 25, 2016

fbrosson left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Begasus commented Dec 25, 2016

Begasus commented Dec 25, 2016

dash102 commented Dec 25, 2016 • edited

Begasus commented Dec 25, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

fbrosson left a comment

This comment has been minimized.

This comment has been minimized.

fbrosson commented Dec 25, 2016

dash102 commented Dec 25, 2016

fbrosson commented Dec 25, 2016

dash102 commented Dec 25, 2016

fbrosson commented Dec 25, 2016

fbrosson commented Dec 25, 2016

waddlesplash commented Dec 26, 2016

waddlesplash commented Dec 26, 2016

dash102 commented Dec 26, 2016

waddlesplash commented Dec 26, 2016

dash102 commented Dec 25, 2016 •

edited