Bug 461776 - Review Request: kde-plasma-quickaccess - Plasma applet for quick access to the most used folders
Review Request: kde-plasma-quickaccess - Plasma applet for quick access to t...
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Orcan Ogetbil
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-10 10:53 EDT by Jaroslav Reznik
Modified: 2008-10-28 23:10 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-10-28 23:10:57 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
oget.fedora: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Jaroslav Reznik 2008-09-10 10:53:08 EDT
Spec URL: http://rezza.hofyland.cz/fedora/packages/kde-plasma-quickaccess/kde-plasma-quickaccess.spec
SRPM URL: http://rezza.hofyland.cz/fedora/packages/kde-plasma-quickaccess/kde-plasma-quickaccess-0.7.1-1.fc9.src.rpm
Description: QuickAccess is a small applet designed for the panel to have quick access to the most used folders.
Comment 1 Orcan Ogetbil 2008-10-05 04:27:42 EDT
The package is (almost) perfect. Koji build is good too.
http://koji.fedoraproject.org/koji/taskinfo?taskID=861703
Here are my notes:
-------------------------------------------------------------------------
You include a .desktop file in the package. Shouldn't you call some update-database script to let KDE know that you added something? I'm just asking (not requiring).
-------------------------------------------------------------------------
According to
   http://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags
packages should honor Fedora-specific compiler flags. Make sure that $RPM_OPT_FLAGS is being honored and used.
-------------------------------------------------------------------------
This is one of my favorite plasma applets. IMHO It should be included in KDE. Thanks for packaging.
Comment 2 Milos Jakubicek 2008-10-05 06:57:22 EDT
(In reply to comment #1)
> You include a .desktop file in the package. Shouldn't you call some
> update-database script to let KDE know that you added something? I'm just
> asking (not requiring).

You have to, see:

https://fedoraproject.org/wiki/Packaging/Guidelines#desktop-file-install_usage
Comment 3 Orcan Ogetbil 2008-10-05 13:38:36 EDT
Dear Milos,
I was aware of that page and actually it's that page which got me confused.

This is not a regular desktop file. It is a KDE4 desktop file.  I'm not sure if it should be treated differently or not. For instance, the spec file of the approved kde-plasma-lancelot package doesn't have such a call.

Well, maybe you are right.
Comment 4 Rex Dieter 2008-10-05 15:46:06 EDT
update-database is required only if the .desktop file includes the MimeType= key (which I don't think is the case here).
Comment 5 Milos Jakubicek 2008-10-05 16:34:18 EDT
I'm not sure whether we're speaking about the same thing now: I didn't say anything about update-desktop-database. The question was about "some update-database script" a my answer concerns the usage of desktop-file-install, where the current guidelines state:

"It is not simply enough to just include the .desktop file in the package, one MUST run desktop-file-install OR desktop-file-validate in %install (and have BuildRequires: desktop-file-utils), to help ensure .desktop file safety and spec-compliance."

If this is not necessary anymore generally (what I doubt strongly) or not necessary in this particular case (if so, please explain to me why -- thanks), then its a topic for FPC to change the guidelines accordingly.
Comment 6 Rex Dieter 2008-10-05 18:06:10 EDT
I was only responding to comment #2, "ie, to let system know"

Yes, desktop-file-install usage *is* required, and is a separate issue (used for .desktop file validation purposes primarily).
Comment 7 Orcan Ogetbil 2008-10-05 23:54:39 EDT
OK then. What he needs is just a desktop-file-install call and that's it, as far as the .desktop file is concerned. I hope we all got over with the confusion now.
Comment 8 Jaroslav Reznik 2008-10-06 04:26:33 EDT
I'm not sure about desktop-file-install usage. There are no modification of desktop file, this desktop file is used for special purpose only for Plasma, so no application menu stuff and even desktop-file-validate is not developed for KDE specific desktop files (but warnings are aware of KDE specific values - see --warn-kde options.
Comment 9 Orcan Ogetbil 2008-10-06 04:57:58 EDT
Well I never saw examples opposing what you say (e.g. the spec file of kde-plasma-lancelot, which is approved, doesn't use dekstop-file-install). When I install the package it shows in the Add Widget list, and when I uninstall the package it vanishes from that list. So in short, it works as it is supposed to.

Let's hear from the other auditers then we can approve the package since it seems otherwise flawless.

(In your spec file I just saw the usage of %{cmake} for configuring the package which takes care of the %{optflags}. I must have missed it in the first pass. Hence you can ignore about my comment about %{optflags})
Comment 10 Rex Dieter 2008-10-06 07:44:04 EDT
desktop-file-install (or desktop-file-validate) usage is *required*.  Homework: if that's not clear to anyone, go read that section of the guidelines again. :)  One primary purpose of that is to ensure correctness/validation of the .desktop files.
Comment 11 Jaroslav Reznik 2008-10-06 08:04:25 EDT
As I understand it:
- desktop-file-install is not required for this package - desktop-file-install MUST be used if the package does not install the file or there are changes desired to the .desktop file (such as add/removing categories, etc)
- desktop-file-validate can check desktop file only partially and complaints about KDE specific extensions (so these are unchecked).

I can add desktop-file-validate to SPEC file but then we need to update other Plasma (KDE?) packages without at least d-f-validate.
Comment 12 Rex Dieter 2008-10-06 08:13:48 EDT
> As I understand it: - desktop-file-install is not required for this package

As a (co)author of the dfi guideline, I can tell you that is contrary to the wording and intent of it :)  
https://fedoraproject.org/wiki/Packaging/Guidelines#desktop-file-install_usage 
says bluntly:
"It is not simply enough to just include the .desktop file in the package, one MUST run desktop-file-install OR desktop-file-validate in %install (and have BuildRequires: desktop-file-utils)"

Feel free to contact me off-list/bugzilla, to see where any misunderstanding comes from and to find ways to reword the guideline to make it more clear.
Comment 13 Rex Dieter 2008-10-06 08:29:06 EDT
OK, my bad for being like a slashdot'er and commenting without actually looking at the spec.

At issue here is whether the app appears in the usual menus and/or in %_datadir/applications.  Answer: no.  Conclusion: no need for desktop-file-install
Comment 14 Orcan Ogetbil 2008-10-06 12:17:50 EDT
No problem rdieter, we all do mistakes :)

Well... This package is good to go:

---------------------------------------------------------
This package (kde-plasma-quickaccess) is APPROVED by oget
---------------------------------------------------------
Comment 15 Jaroslav Reznik 2008-10-06 16:36:10 EDT
New Package CVS Request
=======================
Package Name: kde-plasma-quickaccess
Short Description: Plasma applet for quick access to the most used folders
Owners: jreznik
Branches: F-9
InitialCC:
Comment 16 Kevin Fenzi 2008-10-07 13:39:43 EDT
cvs done.
Comment 17 Orcan Ogetbil 2008-10-28 23:10:57 EDT
package in rawhide. closing the bug.

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