Bug 228272

Summary: Review Request: amarokFS - Simple, nice looking full screen front-end for Amarok
Product: [Fedora] Fedora Reporter: Francois Aucamp <francois.aucamp>
Component: Package ReviewAssignee: Aurelien Bompard <gauret>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: gauret, wtogami
Target Milestone: ---Flags: gauret: fedora-review+
wtogami: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-02-19 22:13:24 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 163779    

Description Francois Aucamp 2007-02-12 12:49:48 UTC
Spec URL: http://www.snoekie.com/rpm/amarokFS.spec
SRPM URL: http://www.snoekie.com/rpm/amarokFS-0.4.2-1.src.rpm
Description:
AmarokFS (Amarok Full Screen) is a full screen front-end for Amarok that
provides a simple and nice looking graphical user interface. It is very
suitable for parties and public terminals. The front-end's appearance can be
customized using themes.

This package also contains a script that allows AmarokFS to be launched
from within Amarok itself.


Package builds in mock (fc6/i386) and on x86_64. rpmlint is silent.

Comment 1 Aurelien Bompard 2007-02-12 22:11:34 UTC
Needs work:
* Source URLs are:
http://www.kde-apps.org/CONTENT/content-files/52641-amarokFS-qt3-0.4.2.tar.gz
* An icon is installed directly under %_datadir/icons
(/usr/share/icons/amarokFS.png). Please use the hicolor directory and its
sub-directories, as required by the spec (al least a 48x48 icon)
* Scriptlets: missing "gtk-update-icon-cache" in %post and %postun (wiki:
ScriptletSnippets)

Minor:
* QT environment variable are not sourced
* Desktop file: the Categories tag should not contain Application any more
  (wiki: PackagingGuidelines#desktop)
* The package should contain the text of the license, please ask the author to
include it in the tarball. It's not a blocker for the package, but it would be
better.


Comment 2 Francois Aucamp 2007-02-13 13:53:32 UTC
Thanks for the feedback!

New build:
Spec URL: http://www.snoekie.com/rpm/amarokFS.spec
SRPM URL: http://www.snoekie.com/rpm/amarokFS-0.4.2-2.src.rpm

Changes:
- Added BuildRequires: desktop-file-utils
- Fixed source URLs
- Removed "Application" from .desktop file's "Categories" tag
- Install application icon to correct location
- Added KDE/GTK icon cache update scriptlets
- Cleaned up the application's qmake file a bit


(In reply to comment #1)
> Needs work:
> * QT environment variable are not sourced

I am not following you here - the package is being built using qmake; AFAIK the
"CONFIG += qt" line in the qmake project file takes care of this?
I have cleaned up the .pro file accordingly to make it more clear, though. 

> * The package should contain the text of the license, please ask the author to
> include it in the tarball. It's not a blocker for the package, but it would be
> better.

I agree. The developer seems to be away on holiday at the moment, but I will
mail him about this; I also plan on submitting some of the patches/bugfixes I
added to this application once the package is approved.


Comment 3 Aurelien Bompard 2007-02-14 06:29:00 UTC
Please also install a 48x48 icon, it is required by the spec. Just use convert
on the 128x128 one (and remember to add ImageMagick as a BuildRequirement).
Otherwise it looks fine, good job.


Comment 4 Francois Aucamp 2007-02-14 07:51:54 UTC
Done.

New build:
Spec URL: http://www.snoekie.com/rpm/amarokFS.spec
SRPM URL: http://www.snoekie.com/rpm/amarokFS-0.4.2-3.src.rpm

Changes:
- Added BuildRequires: ImageMagick
- Create and install a 48x48 icon


Comment 5 Aurelien Bompard 2007-02-14 22:04:46 UTC
Review for release 3.fc6:
* RPM name is OK
* Source 52641-amarokFS-qt3-0.4.2.tar.gz is the same as upstream
* Source 53125-amarokFS.amarokscript.tar is the same as upstream
* Builds fine in mock
* rpmlints look OK
* File lists look OK
* Works fine

APPROVED 

(35 automatic checks have been run by fedora-qa)


Comment 6 Francois Aucamp 2007-02-15 07:08:38 UTC
Thanks for the review!
I will close the review ticket as soon as all branches requested from 
"CVSSyncNeeded" are granted.


Comment 7 Francois Aucamp 2007-02-19 22:13:24 UTC
Built successfully on -devel; closing ticket and requesting additional branches
via CVS flags:

FC-5 FC-6


Comment 8 Warren Togami 2007-02-19 22:17:20 UTC
I believe your package already has those branches as empty directories.   You
should be able to check it out fresh with those directories, add your files,
checkin, tag and build.  Is this not the case?

Comment 9 Francois Aucamp 2007-02-19 22:23:49 UTC
Wow, that was quick ;-)

Nope, sadly all I have is a devel branch:

$ cvs co amarokFS
cvs checkout: Updating amarokFS
U amarokFS/Makefile
U amarokFS/import.log
U amarokFS/pkg.acl
cvs checkout: Updating amarokFS/devel
U amarokFS/devel/.cvsignore
U amarokFS/devel/Makefile
U amarokFS/devel/amarokFS-0.4.2-config_dialog_layout.patch
U amarokFS/devel/amarokFS-0.4.2-fedora_paths.patch
U amarokFS/devel/amarokFS-0.4.2-large_cover_images.patch
U amarokFS/devel/amarokFS-0.4.2-start_amarok.patch
U amarokFS/devel/amarokFS-0.4.2-theme_howto.patch
U amarokFS/devel/amarokFS-0.4.2-theme_prev_button.patch
U amarokFS/devel/amarokFS.spec
U amarokFS/devel/sources
cvs checkout: Updating common
U common/Makefile
U common/Makefile.common
U common/branches
U common/cvs-import.sh


Comment 10 Warren Togami 2007-02-19 22:50:54 UTC
oops, OK fixing this.