Bug 222994 - Review Request: koverartist - Create CD/DVD covers
Review Request: koverartist - Create CD/DVD covers
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Rex Dieter
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-17 08:27 EST by Sebastian Vahl
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-04-12 17:09:33 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
rdieter: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Sebastian Vahl 2007-01-17 08:27:57 EST
Spec URL: http://www.deadbabylon.de/fedora/extras/koverartist/koverartist.spec
SRPM URL: http://www.deadbabylon.de/fedora/extras/koverartist/koverartist-0.5-3.fc6.src.rpm

Description: KoverArtist is a program for the fast creation of covers for
cd/dvd cases and boxes. The main idea behind it is to be able
to create decent looking covers with some mouseclic.
Comment 1 Rex Dieter 2007-01-17 09:58:01 EST
One item I see immediately: use the %find_lang macro instead of manually 
including the locale file(s).
Comment 2 Sebastian Vahl 2007-01-17 10:15:58 EST
Ups. Must have missed this on my checklist (spec is quite old). Sry.

Spec URL: http://www.deadbabylon.de/fedora/extras/koverartist/koverartist.spec
SRPM URL:
http://www.deadbabylon.de/fedora/extras/koverartist/koverartist-0.5-4.fc6.src.rpm
Comment 3 Christoph Wickert 2007-01-17 10:20:40 EST
3 more comments:
- "mouseclic" in description should be "mouseclick" I think. Shouldn't it be the
plural "mouseclicks" anyway?

- desktop-file-install doesn't look ok: Category "X-Fedora" is obsolete. Main
category is "AudioVideo", while the changelog says "changed group to
Applications/Publishing".

- Not sure if
%{_datadir}/applications/fedora-koverartist.desktop should be
%{_datadir}/applications/kde/fedora-koverartist.desktop instead
Comment 4 Sebastian Vahl 2007-01-17 11:08:30 EST
(In reply to comment #3)
> 3 more comments:
> - "mouseclic" in description should be "mouseclick" I think. Shouldn't it be the
> plural "mouseclicks" anyway?

Mhh. Think so. (Note to myself: no more c&p in the descriptions)

> - desktop-file-install doesn't look ok: Category "X-Fedora" is obsolete. Main
> category is "AudioVideo", while the changelog says "changed group to
> Applications/Publishing".

Because I cannot say I've understood the desktop-file-usage completly I've used
a modified entry from the package "kover" (also available in extras - and with
X-Fedora). And also the entry "Group: Applications/Publishing".

I'm not really sure where to put it. The rpm-group "Applications/Multimedia"
doesn't seem the best for me. "Applications/Publishing" could be better (because
it is a  printing program) but the corresponding desktop category* "Office"
would not describe it clearly.

* http://standards.freedesktop.org/menu-spec/latest/apa.html


> - Not sure if
> %{_datadir}/applications/fedora-koverartist.desktop should be
> %{_datadir}/applications/kde/fedora-koverartist.desktop instead

Good question. On my installation only amarok and kaffeine (as single packages)
are in %{_datadir}/applications/kde/ but digikam, kompose, konversation,
kshutdown, and some more use /usr/share/applications/fedora-${name}.desktop

Comment 5 Christoph Wickert 2007-01-17 12:06:19 EST
(In reply to comment #4)
> 
> Because I cannot say I've understood the desktop-file-usage completly I've used
> a modified entry from the package "kover" (also available in extras - and with
> X-Fedora).

Most of our packages still have X-Fedora, because it used to be in the wiki.
AFAIK in October packaging committee voted to stop using this category, although
this never has been announced officially. Maybe Rex can give us some more
details on this?

> I'm not really sure where to put it. The rpm-group "Applications/Multimedia"
> doesn't seem the best for me. "Applications/Publishing" could be better (because
> it is a  printing program) but the corresponding desktop category* "Office"
> would not describe it clearly.

I don't care where you put it but changelog and desktop file should match. IMHO
Applications/Publishing is ok.

> On my installation only amarok and kaffeine (as single packages)
> are in %{_datadir}/applications/kde/ but digikam, kompose, konversation,
> kshutdown, and some more use /usr/share/applications/fedora-${name}.desktop

If the program runs fine in Gnome/XFCE too, I suggest %{_datadir}/applications,
but Gnome/XFCE will also find it inside of %{_datadir}/applications/kde.
Comment 6 Rex Dieter 2007-01-17 12:14:51 EST
> this never has been announced officially.

It was announced to fedora-maintainers.

Regardless, you should do you best to follow the guidelines as it is *now*. (:

Re: %{_datadir}/applications vs. %{_datadir}/applications/kde
You shouldn't mix-n-match --vendor and %{_datadir}/applications/kde.  Either use:
--vendor=fedora --dir=%{_datadir}/applications
or
--vendor="" --dir=%{_datadir}/applications/kde
in this case, probably the former.
Comment 7 Sebastian Vahl 2007-01-17 12:42:46 EST
(In reply to comment #5)
> (In reply to comment #4)
> > 
> 
> I don't care where you put it but changelog and desktop file should match. IMHO
> Applications/Publishing is ok.
> 

The changelog entry belongs to a change in the rpm-group. First I'd put this
into "Groups: User Interface/Desktops" and forgotten to change it. It was not
meant to be a change in the desktop-file.
Comment 8 Sebastian Vahl 2007-01-18 16:52:36 EST
Next round (vendor is still "fedora" 
in /usr/share/applications/fedora-koverartist.desktop):

Spec URL: http://www.deadbabylon.de/fedora/extras/koverartist/koverartist.spec
SRPM URL: 
http://www.deadbabylon.de/fedora/extras/koverartist/koverartist-0.5-5.fc6.src.rpm

Changelog:
- some cleanup in description
- dropped X-Fedora in desktop-file-install
Comment 9 Rex Dieter 2007-04-11 13:28:23 EDT
1.  MUST add/use scriptlets for icons:
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-7103f6c38d1b5735e8477bdd569ad73ea2c49bda

2.  MUST add scriptlets for mimetypes
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-de6770dd9867fcd085a73a4700f6bcd0d10294ef

3.  SHOULD omit
# Set correct entry for "DocPath" in desktop-file
workaround.  It's a workaround for a harmless rpmlint/desktop-file-install warning

4.  (imo) SHOULD set/revert
Group: Applications/Multimedia
(Group isn't really used anywhere, so it's not a big deal)

5.  SHOULD use
desktop-file-install --vendor "" \
 --dir %{buildroot}%{_datadir}/applications/kde

I'll kick off a mock build, and if it builds/runs ok, then fix at least the MUST
items, and I'll APPROVE this.
Comment 10 Rex Dieter 2007-04-11 13:42:53 EDT
mock build succeeded, and runs ok.
Comment 11 Sebastian Vahl 2007-04-11 14:47:10 EDT
Fixed 1,2,3
To 4: Applications/Publishing is also used in kover. So I've taken it from there
(maybe it should be changed in both)
To 5: See comment #5 and comment #6

Spec URL: http://www.deadbabylon.de/fedora/extras/koverartist/koverartist.spec
SRPM URL:
http://www.deadbabylon.de/fedora/extras/koverartist/koverartist-0.5-6.fc6.src.rpm

Changelog:
- run gtk-update-icon-cache on post and postun
- run update-desktop-database on post and postun
- added BR: desktop-file-utils
Comment 12 Rex Dieter 2007-04-12 09:29:46 EDT
> To 4: (maybe it should be changed in both)

imo, yes, shouldn't be in that comps group either, but that's just me.

> To 5: See comment #5 and comment #6

not a valid justification.  imo, --vendor=fedora should only be used if 
we(fedora) are generating our own .desktop file (which isn't the case here).

These aren't a blocker tho, I'll leave the final decisions up to you.

APPROVED.
Comment 13 Sebastian Vahl 2007-04-12 12:58:11 EDT
(In reply to comment #12)
> imo, --vendor=fedora should only be used if 
> we(fedora) are generating our own .desktop file (which isn't the case here).

Haven't thought about it this way. I will change it to --vendor "".

> APPROVED.

Thanks!
Comment 14 Sebastian Vahl 2007-04-12 12:59:10 EDT
New Package CVS Request
=======================
Package Name: koverartist
Short Description: Create CD/DVD covers
Owners: fedora@deadbabylon.de
Branches: FC-5 FC-6
InitialCC: 

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