[Bug 494965] Review Request: pianobooster - A MIDI file player that teaches you how to play the piano

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=494965





--- Comment #3 from Christian Krause <chkr at plauener.de>  2009-04-11 18:01:40 EDT ---
(In reply to comment #1)
> Here are my notes for this package (* need definite attention. ! are
> suggestions):

Thank you for the review.

> * We need to package rtmidi seperately and link to it. I already packaged
> rtaudio before. Things should be similar (you can take rtaudio SPEC file as
> your starting point)

I've checked the upstream rtmidi package (
http://www.music.mcgill.ca/~gary/rtmidi/ ) but unfortunately the package
doesn't provide any Makefiles so that it is not easily possible to build a
library out of the box. Since other projects also use rtmidi just by adding the
3 files, I would vote for not creating an extra library for rtmidi. Once
rtmidi's upstream will provide a full-featured package to build dynamic (or
static) libs this can be re-considered.

> ! Please make use of the %{name} macro.

Done. Besides the URL tag the specfile was changed to use the %{name} tag.

> ! I find it better to supply the .desktop file separately, to preserve the
> original creation date. But that's a matter of taste.

Done. I've added the desktop file as separate source file.

> ! I prefer using sed+iconv instead of dos2unix to save a BR. But then again
> this is a matter of taste. If you are going to use dos2unix, could you use the
> -k flags to preserve timestamps?

Done. (Using sed now. However I haven't found an option to preserve the
timestamp when using sed...)

> * Similarly, please use the -p flag with install to preserve the timestamps (of
> the .png files in this case)

Done.

> ! Please add a GenericName to the .desktop file. Also AudioVideo needs to be
> added to the Category key, if you want this application to appear in Multimedia
> group.

Done. I've used your suggested "Piano Teacher" as GenericName and added the
AudioVideo category.

> * There is a typo on the installation of the 64x64 icon. (32x32 should be
> 64x64)

Done.

> * Parallel make must be supported whenever possible. If it is not supported,
> this should be noted in the SPEC file as a comment.

Done. Changed to parallel build. I've seen no problems so far (in local as well
as in koji/mock builds).

> ! Summary seemed too long to me. It can be just something like "Piano Teacher"

I've checked other specfiles too and I've seen a bunch of packages with similar
summaries. I would rather leave the summary as it is. However, I've re-used the
suggestion as the GenericName in the .desktop file. ;-)

> ! I think the comma should be removed from the end of this line (for proper
> English):
>    "The difference between playing along to a CD or a standard MIDI file,"  

Done.

The modified packages can be found here:

Spec URL: http://chkr.fedorapeople.org/review/pianobooster.spec
SRPM URL: http://chkr.fedorapeople.org/review/pianobooster-0.6.2-3.fc10.src.rpm

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.