Bug 516717 - Review Request: viewnior - Elegant Image Viewer
Summary: Review Request: viewnior - Elegant Image Viewer
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 599230 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-08-11 09:35 UTC by Christoph Wickert
Modified: 2010-06-20 09:43 UTC (History)
7 users (show)

Fixed In Version: 0.7-1.fc11
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-09-24 11:08:03 UTC
mtasaka: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Christoph Wickert 2009-08-11 09:35:31 UTC
Spec URL: http://cwickert.fedorapeople.org/review/viewnior.spec
SRPM URL: http://cwickert.fedorapeople.org/review/viewnior-0.6-1.fc12.src.rpm
Description: Viewnior is an image viewer program. Created to be simple, fast and elegant. It's minimalistic interface provides more screenspace for your images. Among its features are:

* Fullscreen & Slideshow
* Rotate, flip, save, delete images
* Animation support
* Browse only selected images
* Navigation window
* Set image as wallpaper (only under GNOME)
* Simple interface
* Configurable mouse actions

Comment 1 caius.chance 2009-08-14 02:17:16 UTC
rpmlint is done:

$ rpmlint viewnior.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

Comment 2 caius.chance 2009-08-14 02:19:32 UTC
mock is done:

INFO: Done(viewnior-0.6-1.fc12.src.rpm) Config(fedora-rawhide-i386) 4 minutes 35 seconds

Comment 3 Martin Gieseking 2009-08-15 11:56:15 UTC
Christoph, is there any deeper reason for the lengthy loop body that fixes the file permissions? Why not simply use "chmod 0644 $file"? But maybe I'm missing something here.

Comment 4 Christoph Wickert 2009-08-16 08:59:09 UTC
I'm preserving the timestamps. For the stuff inside it's not really important, but for content like docs the original timestamps should be preserved.

See: https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

Comment 5 Susi Lehtola 2009-08-16 09:07:39 UTC
chmod does not change the time stamps (try it!). You can safely drop the loop.

Comment 6 Susi Lehtola 2009-08-16 09:08:29 UTC
Drop the conditional
 %if 0%{?_with_gnome:1}
as we don't use these in Fedora.

Comment 7 Christoph Wickert 2009-08-16 09:19:40 UTC
(In reply to comment #5)
> chmod does not change the time stamps.

Oops, ok (one of these days again...)

(In reply to comment #6)
> Drop the conditional
>  %if 0%{?_with_gnome:1}
> as we don't use these in Fedora. 

We do use similar conditionals and I see no reason why I should drop this. It's an additional feature for people who want to rebuild the package and it allows me to easily change the default behavior.

Comment 8 Susi Lehtola 2009-08-16 09:29:21 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > Drop the conditional
> >  %if 0%{?_with_gnome:1}
> > as we don't use these in Fedora. 
> 
> We do use similar conditionals and I see no reason why I should drop this. It's
> an additional feature for people who want to rebuild the package and it allows
> me to easily change the default behavior.  

I haven't seen any other packages using conditionals for support of packages. In SuSe it's common behavior and I think that some packages in RPMFusion, too, use them, but not in Fedora. Might you give some examples?

You should enable support for everything in the package in Fedora.

Comment 9 Christoph Wickert 2009-08-16 10:03:20 UTC
(In reply to comment #8)

> I haven't seen any other packages using conditionals for support of packages.
> In SuSe it's common behavior and I think that some packages in RPMFusion, too,
> use them, but not in Fedora. Might you give some examples?

With "similar" I meant https://fedoraproject.org/wiki/Packaging/DistTag#Conditionals
Another example, not a particulary good one since it is my package is gwget:
http://cvs.fedoraproject.org/viewvc/rpms/gwget/F-11/gwget.spec?view=markup
The epiphany extension was broken and I had to disable it during the F12 mass rebuild. I was able to do this by changing 1 to 0 instead of commenting out large parts of the spec.

> You should enable support for everything in the package in Fedora.

I surely wont do this because I consider the "enable everything" policy one of the biggest mistakes in Fedora. It introduces long dependency chains, just look at totem or pidgim, that require evolution-data-server and all it's dependencies.

Currently we are thinking about viewnior to replace gpicview and become the default image viewer of LXDE. Having a gconf dependency is a no-go there.

Comment 11 Mamoru TASAKA 2009-08-17 16:48:17 UTC
Disabling some option on configure is allowed (however
the reason should be commented on the spec file so that
other people can find the reason easily)

Comment 12 Christoph Wickert 2009-09-06 01:39:08 UTC
Removed the loop and added a comment on the conditional.

http://cwickert.fedorapeople.org/review/viewnior.spec
http://cwickert.fedorapeople.org/review/viewnior-0.6-2.fc12.src.rpm

Comment 13 Christoph Wickert 2009-09-07 01:48:02 UTC
V 0.7: http://cwickert.fedorapeople.org/review/viewnior-0.7-1.fc12.src.rpm

Comment 14 Mamoru TASAKA 2009-09-22 17:19:01 UTC
Well,

* GConf2 dependency
  - If you don't want to remove GConf2 dependency, I guess the
    following line in %description should be removed ( because
    from configure.ac the following feature can be enabled
    only when compiled with GConf2 )
----------------------------------------------------------------
* Set image as wallpaper (only under GNOME)
----------------------------------------------------------------

Other things seem good.
----------------------------------------------------------------
    This package (viewnior) is APPROVED by mtasaka
----------------------------------------------------------------

Comment 15 Mamoru TASAKA 2009-09-22 17:23:36 UTC
(In reply to comment #14)
> * GConf2 dependency
>   - If you don't want to remove GConf2 dependency, I guess the

... If you don't want to "add" GConf2 dependency, ...

Comment 16 Christoph Wickert 2009-09-22 23:32:07 UTC
(In reply to comment #14)
>     I guess the
>     following line in %description should be removed

Good catch. Thanks for the review!

New Package CVS Request
=======================
Package Name: viewnior
Short Description: Elegant Image Viewer
Owners: cwickert
Branches: F-10 F-11
InitialCC:

Comment 17 Kevin Fenzi 2009-09-24 04:24:41 UTC
cvs done.

Comment 18 Fedora Update System 2009-09-24 09:48:11 UTC
viewnior-0.7-1.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/viewnior-0.7-1.fc11

Comment 19 Fedora Update System 2009-09-24 09:48:54 UTC
viewnior-0.7-1.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/viewnior-0.7-1.fc10

Comment 20 Mamoru TASAKA 2009-09-24 11:08:03 UTC
Closing.

Comment 21 Fedora Update System 2009-10-06 09:59:32 UTC
viewnior-0.7-1.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 22 Fedora Update System 2009-10-06 10:08:20 UTC
viewnior-0.7-1.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 23 Christoph Wickert 2010-06-20 09:43:50 UTC
*** Bug 599230 has been marked as a duplicate of this bug. ***


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