Bug 238235 - (tastymenu) Review Request: TastyMenu - KMenu replacement aiming to provide the maximum usability
Review Request: TastyMenu - KMenu replacement aiming to provide the maximum u...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Christopher Stone
Fedora Package Reviews List
: Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-04-27 23:55 EDT by Kelly Miller
Modified: 2007-11-30 17:12 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-06-01 11:50:07 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
chris.stone: fedora‑review+
dennis: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Kelly Miller 2007-04-27 23:55:40 EDT
Spec URL: http://crystalsanctuary.rpgsource.net/packages/specs/tastymenu.spec
SRPM URL: http://crystalsanctuary.rpgsource.net/packages/source/tastymenu-0.7.1-0.nixa.2.src.rpm
Description:
Tasty Menu is a KMenu replacement aiming to provide the maximum usability, or
at least to be a testbed for usability concepts and ideas for a future KMenu 
replacement.
 
The left part of the menu is very similar to the Novell idea 
(http://www.novell.com/products/desktop/preview.html); you have a search box 
that is always selected when the menu is opened (the search results are 
displayed in the leftmost listview), followed by a combobox that decides what 
appears in the following listview: favorite applications (default), most used
applications, recently used applications or recent documents.
 
The right part contains the whole KMenu and takes the aspect from KBFX 
(http://www.kbfx.org); the middle column contains the top level categorization
(plus in the current KMenu arrangment the Control Center, Home 
folder and Find Files, but I think only categories should be present). 
and the left-most listview contains the content of the category currently 
selected in the middle column. I think in this way even if it has the same 
number of items it "appears" less huge than with a popup menu/submenu 
structure.

Every item has two rows, for the name and for the description, in order to 
make it more informative. On each selected item an action icon appears
on the right; at the moment they are "add bookmark" on application icons and
"remove bookmark" on favorite apps list.
 
The bottom buttons are the usual switch user, lock session and logout. At first
I didn't want to put them, I thought that 
these functions should be delegated to another applet (like 
http://kde-apps.org/content/show.php?content=26150), but it seems that this
doesn't work very well in practice.
 
The left-most button contains the user name and icon, and clicking on it 
opens the Profile Editor. I know it seems silly, but it's only an experiment, 
probably it will be merged with the switch user button.
Comment 1 Trond Danielsen 2007-04-28 04:30:11 EDT
- The release tag is incorrect. Use a single digit unless to do something else.
- The description tag is very long; I would suggest reducing it to one or two
paragraphs.
- According to the list of files, a static library is included in the package.
Take a look at this:
http://fedoraproject.org/wiki/Packaging/Guidelines#head-2302ec1e1f44202c9cc4bcce24cb711266557ad7
Comment 2 Laurent Rineau 2007-04-28 10:45:45 EDT
Kelly needs sponsor. As a KDE user, I would like to participate to the review 
of that package, but there is a tradition in Fedora that the review of the 
first package of a new contributor is done by the sponsor.

I can however give several remarks.
- I second remarks of comment #1 about the release and descriptions tags. 
However, the file list shows no static lib. The .la file is a libtool 
auxiliary file, needed by the dlopen implementation used in KDE3.
- The tarball shipped in the src.rpm is the same as the upstream one.
- Read the guidelines about the use of desktop-file-install:
http://fedoraproject.org/wiki/Packaging/Guidelines#head-d559ee7363418a5840ce63090c608c991cd39ce6
- There is something special to do with the HTML documentation. See the spec 
file of kdebluetooth 
(https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=235203) for an example. 

Rex, I thought there was a project for Packaging/KDE in the Wiki. Do you have 
a draft somewhere? I am quite disappointed not to be able to link a wiki page 
about the KDE HTML doc packaging.
Comment 3 Laurent Rineau 2007-04-28 11:08:49 EDT
(In reply to comment #2)
> - I second remarks of comment #1 about the release and descriptions tags. 

As regards the release tag, read the guidelines:
   http://fedoraproject.org/wiki/Packaging/NamingGuidelines

It would also nice that you use a disttag:
   http://fedoraproject.org/wiki/Packaging/DistTag
Comment 4 Kelly Miller 2007-04-28 12:53:46 EDT
> - Read the guidelines about the use of desktop-file-install:
> 
http://fedoraproject.org/wiki/Packaging/Guidelines#head-d559ee7363418a5840ce63090c608c991cd39ce6

I was actually wondering about this one; do I still have to use 
desktop-file-install, even though it's installing the .desktop file to Kicker 
and not to the menu proper?
Comment 5 Kelly Miller 2007-04-28 13:04:19 EDT
Okay, now I'm confused.  I changed the Release tag to 1%{?dist} as suggested 
in the DistTag guide, but instead of calling the package -1.fcX.i386.rpm, it's 
calling it -1fcX.i386.rpm.
Comment 6 Rex Dieter 2007-04-28 13:31:55 EDT
> do I still have to use desktop-file-install...

no.

Re: %{?dist} usage, 1%{?dist} is correct.  and dist *should* = .fcX (not fcX).
Comment 7 Kelly Miller 2007-04-28 13:46:13 EDT
Okay, since I'm not sure what the problem with the dist naming is, I've 
uploaded the updated spec and source files.

Spec URL: http://crystalsanctuary.rpgsource.net/packages/specs/tastymenu.spec
SRPM URL: 
http://crystalsanctuary.rpgsource.net/packages/source/tastymenu-0.7.1-1fc6.src.rpm
Comment 8 Christopher Stone 2007-04-28 14:32:19 EDT
Kelly, I can sponser you, however I really prefer it if you can log onto EFNet
IRC #fedora-devel as communication this way is much faster and easier.  My nick
there is XulChris.

Some initial comments on your spec file:
- Release tag needs a dist tag, for example:
Release: 2%{?dist}

Do not use 0.nixa. stuff, also remove dist tag stuff from your changelog
comments, it should just be:

* Fri Apr 27 2007 Kelly Miller <lightsolphoenix@gmail.com> 0.7.1-2

- The URL tag should just be: http://www.notmart.org
- Your BR lists redundant packages, change it to:
BuildRequires: qt-devel >= 3.0.0, kdelibs-devel >= 3.0.0
- I would recommend just using the first paragraph for summary
- Change %files to %{_docdir}/HTML/en/tastymenu so that you own the directory,
do not bother listing each file under this directory, the directory is all you
need to list, anything under it gets picked up automatically.

Comment 9 Christopher Stone 2007-04-28 14:36:59 EDT
oops, sorry for lack of sleep last night I mentioned the wrong IRC network :)

#fedora-devel is on freenode.net, not EFNet

Also, please shorten the Summary tag, all you need is:
Summary: KMenu replacement


Comment 10 Kelly Miller 2007-04-28 14:40:49 EDT
> - Your BR lists redundant packages, change it to:
> BuildRequires: qt-devel >= 3.0.0, kdelibs-devel >= 3.0.0

Actually, that was my original BuildRequires list, but when I tested the build 
using mock, the system complained about qt and kdelibs not being there.  When 
I added them to the BuildRequires list, the package compiled under mock.
Comment 11 Christopher Stone 2007-04-28 15:04:56 EDT
Kelly: qt-devel and kdelibs-devel will bring in those other packages
automatically.  I imagine you made some other mistake previously.  I tested it
myself and it works here just fine. If you are getting build failures in mock
check your build.log and root.log.  The root.log should show those packages
being added.

Also, I tested this build on a 64bit system and there are 64bit rpath problems,
please add this to your %prep section:

# Avoid lib64 rpaths
sed -i -e 's|"/lib /usr/lib|"/%{_lib} %{_libdir}|' configure

Also, please run rpmlint on your packages after they are built, there are some
rpmlint output which can be easily fixed, for example:

W: tastymenu mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 46)
W: tastymenu dangling-relative-symlink /usr/share/doc/HTML/en/tastymenu/common
../common
W: tastymenu incoherent-version-in-changelog 1.fc6 0.7.1-1.fc6
E: tastymenu-debuginfo script-without-shebang
/usr/src/debug/tastymenu-0.7.1/src/misc.cpp


Rex: I'm not sure about the following rpmlint error, is this typical of KDE apps?

E: tastymenu invalid-soname /usr/lib64/tastymenu_panelapplet.so
tastymenu_panelapplet.so
Comment 12 Christopher Stone 2007-04-28 15:08:49 EDT
W: tastymenu dangling-relative-symlink /usr/share/doc/HTML/en/tastymenu/common
../common

Actually, this can be ignored.

Also, in comment #8 I said:
- I would recommend just using the first paragraph for summary

I actually meant the description tag, not summary tag.
Comment 13 Laurent Rineau 2007-04-28 15:31:08 EDT
Nice work!  tastymenu-0.7.1-1fc6.src.rpm seems a far better packaging than the 
previous one. There is still the issue of ${_libdir}/ tastymenu_panelapplet.so

And your BuidlRequires are a little too long. "BuildRequires: qt-devel, 
kdelibs-devel" should be enough, even though it is not a critical issue. I 
have not looked that the compilation itself, but why kdebase is 
build-required?

I copy-paste the issues found by rpmlint below:

lrineau@tsetse ~/RPM $ rpmlint SRPMS/tastymenu-0.7.1-1.amoi_rineau.src.rpm
W: tastymenu mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 42)
lrineau@tsetse ~/RPM $ rpmlint tastymenu
W: tastymenu incoherent-version-in-changelog 1 0.7.1-1.amoi_rineau
E: tastymenu invalid-soname /usr/lib/tastymenu_panelapplet.so 
tastymenu_panelapplet.so
W: tastymenu undefined-non-weak-symbol /usr/lib/tastymenu_panelapplet.so 
_ZTI8KService
W: tastymenu undefined-non-weak-symbol /usr/lib/tastymenu_panelapplet.so 
_ZTI13KServiceGroup
W: tastymenu undefined-non-weak-symbol /usr/lib/tastymenu_panelapplet.so 
_ZN8KService20serviceByDesktopPathERK7QString
W: tastymenu undefined-non-weak-symbol /usr/lib/tastymenu_panelapplet.so 
_ZN13KServiceGroup7entriesEbbbb
W: tastymenu undefined-non-weak-symbol /usr/lib/tastymenu_panelapplet.so 
_ZN9KDirWatch7addFileERK7QString
W: tastymenu undefined-non-weak-symbol /usr/lib/tastymenu_panelapplet.so 
_ZN4KRun10runCommandE7QString
W: tastymenu undefined-non-weak-symbol /usr/lib/tastymenu_panelapplet.so 
_ZNK9KFileItem4timeEjRb
W: tastymenu undefined-non-weak-symbol /usr/lib/tastymenu_panelapplet.so 
_ZN13KServiceGroup4rootEv
W: tastymenu undefined-non-weak-symbol /usr/lib/tastymenu_panelapplet.so 
_ZN13KServiceGroup5groupERK7QString
W: tastymenu undefined-non-weak-symbol /usr/lib/tastymenu_panelapplet.so 
_ZN15KRecentDocument15recentDocumentsEv
W: tastymenu undefined-non-weak-symbol /usr/lib/tastymenu_panelapplet.so 
_ZN13KServiceGroup10childCountEv
W: tastymenu undefined-non-weak-symbol /usr/lib/tastymenu_panelapplet.so 
_ZN10KDirListerC1Eb
W: tastymenu undefined-non-weak-symbol /usr/lib/tastymenu_panelapplet.so 
_ZN11KIconButtonC1EP7QWidgetPKc
W: tastymenu undefined-non-weak-symbol /usr/lib/tastymenu_panelapplet.so 
_ZN18KDEDesktopMimeType3runERK4KURLb
W: tastymenu undefined-non-weak-symbol /usr/lib/tastymenu_panelapplet.so 
_ZNK8KService6menuIdEv
W: tastymenu undefined-non-weak-symbol /usr/lib/tastymenu_panelapplet.so 
_ZNK8KService6pixmapEN5KIcon5GroupEiiP7QString
W: tastymenu undefined-non-weak-symbol /usr/lib/tastymenu_panelapplet.so 
_ZN9KDirWatchC1EP7QObjectPKc
W: tastymenu 
unused-direct-shlib-dependency /usr/lib/tastymenu_panelapplet.so /lib/libm.so.6
Comment 14 Christopher Stone 2007-04-28 18:58:21 EDT
indeed, there seems to be some linking problems.  Also, I get a segfault when
trying to configure tastymenu.
Comment 15 Christopher Stone 2007-04-28 21:05:25 EDT
it appears this needs to be linked against libkio as well.  Kelly, you might
want to contact upstream author and ask him about this.  It seems tastymenu uses
KServiceGroup class which is part of the kio library, but he does not link
against libkio.  I've fixed this by adding this line after %configure:

sed -i -e 's|$(LIB_KDEUI)|$(LIB_KDEUI) $(LIB_KIO)|' src/Makefile

The unused-direct-shlib-dependency warnings (I get two of them) are caused by
libraries brought in through libtool from this line:
postdeps="-lstdc++ -lm -lgcc_s -lc -lgcc_s"

This cannot be worked-around by using Fedora's libtool because it does the same
thing.  I'm not sure how to avoid this warning other than adding this after
%configure:
sed -i -e 's|"-lstdc++ -lm -lgcc_s -lc -lgcc_s|"-lstdc++ -lc|' libtool

But I'm not entirely sure what is going on here and if this is the proper fix. 
It might be okey to just ignore those warnings.
Comment 16 Kelly Miller 2007-04-29 00:23:53 EDT
After a bunch of adjustments, here's the updated file list:

Spec URL: http://crystalsanctuary.rpgsource.net/packages/specs/tastymenu.spec
SRPM URL: 
http://crystalsanctuary.rpgsource.net/packages/source/tastymenu-0.7.1-4fc6.src.rpm
Comment 17 Christopher Stone 2007-04-29 16:24:56 EDT
Please remove the qt-devel from BuildRequires, this is redundant and is picked
up by kdelibs-devel.  There is also a new version out.  I'm not sure yet what to
do with the .so name.  Perhaps Rex might know more.

I found another bug with new subfolders not showing up.  For example if you
install an application that creates a new subfolder, the application will not
show up in tastymenu, but shows up in kmenu.
Comment 18 Kelly Miller 2007-04-29 17:07:47 EDT
Done, and I also updated the package to version 0.8.

Spec URL: http://crystalsanctuary.rpgsource.net/packages/specs/tastymenu.spec
SRPM URL: 
http://crystalsanctuary.rpgsource.net/packages/source/tastymenu-0.8-1fc6.src.rpm
Comment 19 Christopher Stone 2007-04-30 14:01:33 EDT
Kelly, to fix the last remaining rpmlint issue, please compile with:
%configure --disable-rpath --libdir=%{_libdir}/kde3

and changes your %files to read:
%{_libdir}/kde3/*
Comment 21 Kelly Miller 2007-04-30 19:20:07 EDT
Another quick update; I added the script to remove the unnecessary .la files 
from the package.

Spec URL: http://crystalsanctuary.rpgsource.net/packages/specs/tastymenu.spec
SRPM URL: 
http://crystalsanctuary.rpgsource.net/packages/source/tastymenu-0.8-4fc6.src.rpm
Comment 22 Rex Dieter 2007-05-01 07:41:38 EDT
> I added the script to remove the unnecessary .la files 

Most kde apps really *do* need those for proper function at runtime.
Comment 23 Kelly Miller 2007-05-01 13:09:24 EDT
Okay; I wasn't sure, but I removed the line again.

Spec URL: http://crystalsanctuary.rpgsource.net/packages/specs/tastymenu.spec
SRPM URL: 
http://crystalsanctuary.rpgsource.net/packages/source/tastymenu-0.8-5fc6.src.rpm
Comment 24 Christopher Stone 2007-05-01 19:57:36 EDT
==== REVIEW CHECKLIST ====
- rpmlint output:
W: tastymenu dangling-relative-symlink /usr/share/doc/HTML/en/tastymenu/common
../common

This is okay, package requires kdelibs which owns this file.

- package named according to package naming guidelines
- spec filename matches %{name}
- package meets packaging guidelines
- licensed with open source compatible license
- license field matches actual license
- license file included in %doc
- spec written in American english
- spec legible
- sources match upstream
d58492c17fe97615e912d28399fce2ef  tastymenu-0.8.tar.bz2
- successfully compiles and builds on FC6 x86_64
- all build dependencies listed in BR
- locales handled properly
- no shared library in default path
- package not relocatable
- package owns all directories it creates
X package Requires packages to bring in all other used directories
- no duplicates in %files
- file permissions set properly
- contains proper %clean section
- macro usage consistent
- contains code
- no large documentation
- files in %doc do not affect runtime
- no header files
- no static libraries
- no pkgconfig files
- no library with suffix
- no need for devel subpackage
- package contains libtool archive 
This is a KDE package which is an exception to the .la requirement
- contains proper .desktop file
- package does not own files or directories owned by other packages
- contains proper %install
- all filenames valid utf-8


==== MUST FIX =====
- Add:  Requires: kdebase
This brings in the /usr/share/apps/kicker/applets/ directory.
Comment 26 Christopher Stone 2007-05-08 19:15:34 EDT
Hi Kelly, can you do a review of someone else's package and link to the bug #
here?  Want to make sure you know the review procedures and can do a capable
review.  :)

Thanks.
Comment 27 Kelly Miller 2007-05-08 22:36:48 EDT
I can try, but I'm not a real fantastic reviewer of other people's work.
Comment 28 Kelly Miller 2007-05-08 22:37:28 EDT
I can try, but I'm not a real fantastic reviewer of other people's work.
Comment 29 Christopher Stone 2007-05-08 22:51:12 EDT
just go through the items listed here:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines

and mark off the items as you check them.  see comment #24 for an example. 
click on the FE-NEEDSPONSOR link below to find a list of packages that probably
have a couple errors in them. :)

I try to do atleast 1 review for every package I submit.  The more you do the
better you get at it.

Thanks.
Comment 30 Kelly Miller 2007-05-09 17:22:17 EDT
Done.

https://bugzilla.redhat.com/bugzilla/process_bug.cgi#c8
Comment 31 Christopher Stone 2007-05-15 23:01:20 EDT
I was hoping to see you at the KDE SIG meeting today, pop in IRC when you have
some free time and msg me and we'll get you set up with sponsership! :)
Comment 32 Christopher Stone 2007-05-16 15:46:53 EDT
All MUST FIX items fixed, package looks good.

**** APPROVED ****

Feel free to ping me on IRC if you have any questions.
Comment 33 Kelly Miller 2007-05-16 19:54:38 EDT
New Package CVS Request
=======================
Package Name: tastymenu
Short Description: KMenu replacement
Owners: lightsolphoenix@gmail.com
Branches: FC-6
InitialCC: lightsolphoenix@gmail.com
Comment 34 Dennis Gilmore 2007-05-17 00:22:22 EDT
cvs done
Comment 35 Kelly Miller 2007-05-28 16:39:50 EDT
Package Change Request
======================
Package Name: tastymenu
New Branches: FC-7
Comment 36 Dennis Gilmore 2007-05-28 16:59:04 EDT
cvs done

Please note the Fedora 7 branch is F-7 not FC-7
Comment 37 Laurent Rineau 2007-06-01 11:46:03 EDT
As far as I know, this bug should be closed.
Comment 38 Kelly Miller 2007-06-01 11:50:07 EDT
Yeah.  My first reaction was "It's not closed?"

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