This service will be undergoing maintenance at 00:00 UTC, 2016-09-28. It is expected to last about 1 hours
Bug 244702 - Review Request: xzgv - A GTK+/Imlib-based picture viewer for X
Review Request: xzgv - A GTK+/Imlib-based picture viewer for X
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-06-18 13:19 EDT by Terje Røsten
Modified: 2007-11-30 17:12 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-07-04 13:07:32 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
kevin: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Terje Røsten 2007-06-18 13:19:10 EDT
Spec URL: http://web.phys.ntnu.no/~terjeros/xzgv/xzgv.spec
SRPM URL: http://web.phys.ntnu.no/~terjeros/xzgv/xzgv-0.8-1.fc6.src.rpm
Description:
A picture viewer for X, with a thumbnail-based file selector. It uses
GTK+ and Imlib. Most file formats are supported, and the thumbnails
used are compatible with xv, zgv, and the Gimp.
Comment 1 Kevin Fenzi 2007-06-20 01:55:19 EDT
I'd be happy to review this package. Look for a full review in a few here...
Comment 2 Kevin Fenzi 2007-06-20 02:22:23 EDT
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (GPL)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
e392277f1447076402df2e3d9e782cb2  xzgv-0.8.tar.gz
e392277f1447076402df2e3d9e782cb2  xzgv-0.8.tar.gz.1
See below - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install
OK - Package is a GUI app and has a .desktop file
OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
OK - No rpmlint output.
OK - final provides and requires are sane.

SHOULD Items:

See below - Should build in mock.
OK - Should build on all supported archs
OK - Should have dist tag
OK - Should package latest version

Issues:

1. You might add "|| :" to the end of your install-info calls.
See:http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-47896da5fb2662d75deefeb9ba75145a398515db

2. Might add AUTHORS and NEWS as doc files?

3. You should use %{?_smp_mflags} with your make call unless it doesn't work.
If it doesn't please add a comment that it's not being used.

4. You seem to be missing a
BuildRequires: desktop-file-utils
Which causes the build to fail. Adding that gets it working.

5. The "Application" Categories in your desktop file is no longer used.
It should be removed. See:
http://standards.freedesktop.org/menu-spec/latest/apa.html

6. Is redhat-graphics an appropriate icon for this app?
Comment 3 Terje Røsten 2007-06-20 17:50:41 EDT
> 1. You might add "|| :" to the end of your install-info calls. 
> 2. Might add AUTHORS and NEWS as doc files?
> 3. You should use %{?_smp_mflags} 
> 4. You seem to be missing  BuildRequires: desktop-file-utils
> 5. The "Application" Categories in your desktop file is no longer used.
> 6. Is redhat-graphics an appropriate icon for this app?

All should be fixed, I switch icon to eog in absent of something better.

SPEC: http://web.phys.ntnu.no/~terjeros/xzgv/xzgv.spec
SRPM:  http://web.phys.ntnu.no/~terjeros/xzgv/xzgv-0.8-2.fc7.src.rpm



Comment 4 Kevin Fenzi 2007-06-20 20:53:59 EDT
Thanks for the quick fixes. I see 2 issues now: 

1. You added AUTHORS and NEWS to your chmod line, but didn't add them in %doc
under files, so they are still not shipped. 

2. Not sure using the eog icon is a good idea. This is after all not that
application. Perhaps you could ask upstream? Or possibly request an icon from 
http://fedoraproject.org/wiki/Artwork/DesignService ?
Perhaps for now this shouldn't have any icon? 

Comment 5 Terje Røsten 2007-07-01 16:51:32 EDT
> 1. You added AUTHORS and NEWS to your chmod line, but didn't add them in %doc
> under files, so they are still not shipped. 

:-) fixed
 
> 2. perhaps for now this shouldn't have any icon? 

What about image-viewer.png from gnome-icon-theme? Seems like a good fit.

I see that gv is doing  the same thing (using postscript-viewer.png from
gnome-icon-theme).


Also added xterm add to req, as the help is based on forking a xterm.

Comment 7 Kevin Fenzi 2007-07-03 16:41:23 EDT
Yeah, using that icon seems like a good solution. 
However, should you not then require gnome-icon-theme? Or is there some
dependency here that would bring it in? I don't see one off hand... 
Comment 8 Terje Røsten 2007-07-03 16:58:11 EDT
> However, should you not then require gnome-icon-theme? 

Done.

> Or is there some
> dependency here that would bring it in? I don't see one off hand... 

only gtk2 and hicolor-icon-theme which in turn requires coreutils.

gnome-session also need gnome-icon-theme, hence g-i-t is most
likely installed on most systems already.

spec: http://web.phys.ntnu.no/~terjeros/xzgv/xzgv.spec
srpms:  http://web.phys.ntnu.no/~terjeros/xzgv/xzgv-0.8-4.fc7.src.rpm


Comment 9 Kevin Fenzi 2007-07-03 17:12:50 EDT
ok, that looks good. I see no further blockers here... so this package is APPROVED. 

Don't forget to close this review request once it's been imported and built. 
Also, do consider going and reviewing some other packages waiting in the queue. 
Comment 10 Terje Røsten 2007-07-03 17:47:10 EDT
> ok, that looks good. I see no further blockers here... so this package is
APPROVED. 

Thanks!
 
> Also, do consider going and reviewing some other packages waiting in the queue.

yes, just did one yesterday. 

BTW: could you please have a look at #229521 .


New Package CVS Request
=======================
Package Name: xzgv
Short Description: A GTK+/Imlib-based picture viewer for X
Owners: terjeros@phys.ntnu.no
Branches: FC-6 F-7
Comment 11 Kevin Fenzi 2007-07-03 17:51:09 EDT
Is there a reason you removed the fedora-review + flag?
That should stay to show this package is approved. 

> BTW: could you please have a look at #229521

Apparently not. "You are not authorized to access bug #229521"

Comment 12 Terje Røsten 2007-07-03 17:59:48 EDT
> Is there a reason you removed the fedora-review + flag?
> That should stay to show this package is approved. 

Mistake, I set fedora-review to ?, realised that was wrong,
fedora-cvs should be ?, then set fedora-review to <blank>
which is of course also wrong. Sorry.

> Apparently not. "You are not authorized to access bug #229521"

I need some sleep (past midnight here..), typo it's: #229591 .
Comment 13 Kevin Fenzi 2007-07-03 21:47:32 EDT
cvs done.

Note You need to log in before you can comment on or make changes to this bug.