Bug 222521 - Review Request: IceWM - Lightweight Window Manager.
Summary: Review Request: IceWM - Lightweight Window Manager.
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Patrice Dumas
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
: 227732 (view as bug list)
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2007-01-13 05:28 UTC by Gilboa Davara
Modified: 2007-11-30 22:11 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-02-20 11:01:54 UTC
Type: ---
Embargoed:
notting: fedora-cvs+


Attachments (Terms of Use)
desktop file for icewm for *dm sessions (179 bytes, text/plain)
2007-01-16 21:17 UTC, Patrice Dumas
no flags Details
icewm.desktop using icewm-session (187 bytes, text/plain)
2007-01-16 21:43 UTC, Patrice Dumas
no flags Details
add --with-icon and --theme options to icewm-xdg-menu (3.13 KB, patch)
2007-01-21 13:37 UTC, Patrice Dumas
no flags Details | Diff
Modified icemw-xdg-menu patch (4.85 KB, patch)
2007-01-25 06:22 UTC, Gilboa Davara
no flags Details | Diff
patch with upstream revisions and icon size command line switch (4.10 KB, patch)
2007-01-25 10:47 UTC, Patrice Dumas
no flags Details | Diff

Description Gilboa Davara 2007-01-13 05:28:49 UTC
Spec URL: http://gilboadavara.thecodergeek.com/icewm.spec
SRPM URL: http://gilboadavara.thecodergeek.com/icewm-1.2.30-1.src.rpm

Description:
IceWM is a window manager for the X Window System (freedesktop, XFree86).
The goal of IceWM is speed, simplicity, and not getting in the user's way.

$ rpmlint -i -v SRPMS/icewm-1.2.30-1.src.rpm
I: icewm checking

- Gilboa

Comment 1 Gilboa Davara 2007-01-13 05:31:53 UTC
Added NEED-REVIEW

Comment 2 Gilboa Davara 2007-01-13 05:48:46 UTC
Spec URL: http://gilboadavara.thecodergeek.com/icewm.spec
SRPM URL: http://gilboadavara.thecodergeek.com/icewm-1.2.30-2.src.rpm

* Tue Jan 11 2007 <gilboad AT gmail DOT com> - 1.2.30-2
- Manually add missing man page.

- Gilboa

Comment 3 Gilboa Davara 2007-01-13 06:04:48 UTC
* Sat Jan 13 2007 <gilboad AT gmail DOT com> - 1.2.30-3
- Fix wrong license. (Was LGPL, should be GPL.)

Spec URL: http://gilboadavara.thecodergeek.com/icewm.spec
SRPM URL: http://gilboadavara.thecodergeek.com/icewm-1.2.30-3.src.rpm

- Gilboa

Comment 4 Peter Gordon 2007-01-13 06:38:35 UTC
(In reply to comment #1)
> Added NEED-REVIEW

The FE-REVIEW blocker should only be added to a bug that is currently in
progress of being reviewed, not to a bug which needs a review.

Also, it appears that you are not yet sponsored (which I am unable to do). I
will therefor add the FE-NEEDSPONSOR blocker. When you are sponsored either from
this package or another of your submissions, the blocker should be manually removed.

Thanks.

Comment 5 Gilboa Davara 2007-01-13 08:40:56 UTC
OK. Thanks.
I misunderstood the how-to-join HOWTO.

- Gilboa

Comment 6 Patrice Dumas 2007-01-13 19:54:55 UTC
Comments:
* for me, tabs result in bad formatting
* For man pages, I prefer something along 
%{_mandir}/man1/icewm.1*
to catch different compression or no compression at all.
* the themes are not packaged. Is it on purpose?
* CXXFLAGS add -fpermissive -Wall -Wpointer-arith -Wconversion -Wwrite-strings -W
inline -Woverloaded-virtual -W -fno-exceptions -fno-rtti
is it right?
* compilation warnings don't seem to be that harmfull to me, but may be
worth reporting upstream (some system return flag may not be checked)
* ldd -u -r on executables leads to many Unused direct dependencies. It is
not very problematic, but it leads to unusefull dependencies on other
libs sonames, which will lead you to unusefull rebuild when the soname
change. Most of the time this is caused by pkgfonfig files not using
corrrectly .private, or project not using pkgconfig it is this case for
unusefull link on X libs in icewm, since icewm don't use pkgconfig for X 
libs.

Needswork:
* patch should be applied to lib/menu.in, not lib/menu
* To have the prefix set correctly, configure and configure.in 
should be patched to set CONFIG_GNOME2_MENU_DIR instead of lib/menu(.in)
        CONFIG_GNOME2_MENU_DIR="${GNOME2_PREFIX}/share/gnome/vfolders/"
should be 
        CONFIG_GNOME2_MENU_DIR="${GNOME2_PREFIX}/share/gnome/desktop-directories/
"
Then lib/menu.in could be patched to remove 
menuprog Gnome folder icewm-menu-gnome1 --list
and use icewm-menu-gnome2 for kde folder too.
* BR (BuildRequires) for kde-config is needed for CONFIG_KDE_MENU_DIR
* things in the default menu should be removed or be Requires. In my
  opinion xterm could stay, but the remaining should go (especially mozilla
  which isn't in fedora). Mozilla could be replaced by htmlview, 
  alternatively.
* files in %{datadir}/icewm are not %config files, these are defaults.
  What is in --with-cfgdir are config file and there are none. 
  My opinion is that %configure should have 
  --with-cfgdir=%{_sysconfdir}/icewm otherwise /etc is hardcoded.
* %{_sysconfdir}/icewm should be created and owned by the package
* locales aren't handled right, it is covered in the guidelines
http://fedoraproject.org/wiki/Packaging/Guidelines#head-8c605ebf8330f6d505f384e671986fa99a8f72ee
* KDEDIR is taken from the environment, so 
/etc/profile.d/kde.sh
should be sourced
* gettext BuildRequires is missing
* VERSION PLATFORMS in %doc seem unuseful to me


Comment 7 Patrice Dumas 2007-01-14 11:23:44 UTC
Another comment, a convention I find useful for patch is to name
them with the package name, package version they were introduced
in first, and add a -b when patching. In your case, this would lead to

Patch0: icewm-1.2.30-gnome_menu.patch

%patch0 -p 0 -b .gnome_menu

Adding the -b helps in case one want to reuse gendiff after modifying
the initial patch. 

This is not mandatory at all, things are fine as they are too.

Comment 8 Mamoru TASAKA 2007-01-15 18:57:27 UTC
Removing NEEDSPONSOR (bug 221873)

Comment 9 Gilboa Davara 2007-01-16 15:57:40 UTC
- Tab size:
I'm using ts=4 - which may cause a problem if/when ts=8.
(I know that there's a world war going on about tab size/stop).
Is there a unified SPEC format that dictates the tab size and/or tabs vs. spaces?

- CXXFLAGS:
AFAICS these are the CXXFLAGS generated by automake.

- Themes:
Due to bandwidth constraints, I don't use mock. At least on my dev
machine, themes are packed into the RPM.

- KDE:
Fedora is GNOME centric. While I'm a KDE user myself, I see little reason to
force users to install the KDE dep-chain when they are looking for a
lightweight WM.
In short, KDE is removed from menu.in.

- Default menu:
* Browser:
AFAIK Firefox is the default browser in Fedora. Added as the default browser
in the main menu. (htmlview is less useful)
Should we consider elinks instead?

* Misc:
I'm also getting idesk and gmrun reviewed - once they are in, I'll add them as
BR/R in an attempt to create a fuller DE environment.

* Tue Jan 16 2007 <gilboad AT gmail DOT com> - 1.2.30-4
- Fix man page name.
- Remove missing menu items.
- Convert GNOME-menu patch to configure.in patch.
- Push default configuration into /etc/icewm
- Remove default KDE support. (GNOME centric distribution -
    no need to drag the KDE dep-chain in a lightweight WM)
- Require firefox (default browser in Fedora).
- Add missing firefox icon. (No source - manual convert)
- Add missing gnome-menus. (required for GNOME2 menus)
- Fix missing gettext BR.
- Fix missing lang support.

Spec URL: http://gilboadavara.thecodergeek.com/icewm.spec
SRPM URL: http://gilboadavara.thecodergeek.com/icewm-1.2.30-4.src.rpm

- Gilboa


Comment 10 Gilboa Davara 2007-01-16 16:01:26 UTC
A couple of issues.
1. I'm using ImageMagick's convert to create the firefox icon. There's no
outside source for 16x16 firefox xpm image. Is it allowed?
2. GNOME-menu seem like it's semi-broken. I'll report upstream.
3. I need to integrate icewm into gdm. I'll do it ASAP.

Thanks for the review.
- Gilboa

Comment 11 Rahul Sundaram 2007-01-16 16:59:07 UTC
(In reply to comment #9)

> - KDE:
> Fedora is GNOME centric. While I'm a KDE user myself, I see little reason to
> force users to install the KDE dep-chain when they are looking for a
> lightweight WM.
> In short, KDE is removed from menu.in.

Seems to be the wrong decision to me. It would be better to split off both GNOME
and KDE support into sub packages so that they might be included in the
different spins for Fedora 7. 

Comment 12 Gilboa Davara 2007-01-16 17:13:56 UTC
#11.

I'm a new member in the extras' world.
If I add kde-config to BR (or actually kdelibs), whouldn't it add REQ in install
time?

- Gilboa

Comment 13 Patrice Dumas 2007-01-16 20:33:34 UTC
(In reply to comment #9)
> - Tab size:
> I'm using ts=4 - which may cause a problem if/when ts=8.
> (I know that there's a world war going on about tab size/stop).
> Is there a unified SPEC format that dictates the tab size and/or tabs vs. spaces?

No, it is your personal preference. I prefer using spaces, but there
is nothing that dictates the appearance of the spec file, except
that it should remain legible, which is the case here.

> - CXXFLAGS:
> AFAICS these are the CXXFLAGS generated by automake.

No, there is no use of automake in icewm. These seems to be
set in ICE_CXX_FLAG_ACCEPT in configure.in.

> - Themes:
> Due to bandwidth constraints, I don't use mock. At least on my dev
> machine, themes are packed into the RPM.

Oops, indeed they are, I missed them.

> - KDE:
> Fedora is GNOME centric. 

No. It is admittedly freedesktop centric, but nothing more.

> While I'm a KDE user myself, I see little reason to
> force users to install the KDE dep-chain when they are looking for a
> lightweight WM.

Nothing from gnome should be installed either. Only used if present.

> In short, KDE is removed from menu.in.

Please try to arrange things such that gnome and kde stuff is used
if they are installed, and that things go smoothly if they aren't.
If I am not wrong this is what is currently done by using 
 /usr/share/desktop-directories/

> - Default menu:
> * Browser:
> AFAIK Firefox is the default browser in Fedora. Added as the default browser
> in the main menu. (htmlview is less useful)

htmlview is much smarter than firefox to chose the right browser.
The fact that firefox is the default browser doesn't means that
it should be used.

> Should we consider elinks instead?

No, let's let htmlview do the work. I guess you can setup things to use
elinks through htmlview... Have a look at the htmlview script, and try to
run it if you are not convinced.

> * Misc:
> I'm also getting idesk and gmrun reviewed - once they are in, I'll add them as
> BR/R in an attempt to create a fuller DE environment.

The window manager is not the place where the desktop should be 
set up. What you need is a group in the comps file, but please try to 
minimize the window manager dependencies.

The other way around is also true, apps should not depend on a particular
window manager and most of the time not on a window manager at all.

> * Tue Jan 16 2007 <gilboad AT gmail DOT com> - 1.2.30-4
> - Fix man page name.

Please don't do the gziping of man pages yourself, just install
them and let rpm handle them.

> - Push default configuration into /etc/icewm

That's not right. Please leave the default config files in %_datadir,
just create and own the %{_sysconfdir}/icewm directory. You can
use complicated things  with noverify, if you like, for files that
are meant to be in that directory, but I don't think it is worth it.

> - Remove default KDE support. (GNOME centric distribution -
>     no need to drag the KDE dep-chain in a lightweight WM)

Please no. I believe kde is used currently, although I haven't tested.
If you feel brave, you could have a look at how pyxdg is used to
create menus in fluxbox-xdg-menu and try to hack something similar... 

> - Add missing firefox icon. (No source - manual convert)

Beware of the trademark issues surrounding firefox.

> - Add missing gnome-menus. (required for GNOME2 menus)

Not sure that it is needed. It would be certainly better if it wasn't
needed, and only desktop neutral freedesktop stuff was needed. In fact
it seems that it is what is done. You should in my opinion depend
on redhat-menus instead (there is a directory ownership issue, I just
filled Bug 222905).

> - Fix missing lang support.

It is not completly fixed.
There should be

%files -f %{name}.lang

and no

%{_datadir}/locale/*/LC_MESSAGES/*


Comment 14 Patrice Dumas 2007-01-16 21:16:38 UTC
(In reply to comment #10)
> A couple of issues.
> 1. I'm using ImageMagick's convert to create the firefox icon. There's no
> outside source for 16x16 firefox xpm image. Is it allowed?

As I said above beware with the firefox trademark issues. Would be better
if you used a more neutral one, like

 /usr/share/pixmaps/redhat-web-browser.png

There are certainly also trademark issues, though.

I also found:
/usr/share/icons/hicolor/48x48/stock/navigation/stock_terminal-webbrowser.png

> 2. GNOME-menu seem like it's semi-broken. I'll report upstream.

It's not semi broken, it is completely broken ;-). At least the
empty submenus shouldn't be shown, there shouldn't be double
submenus and all the gnome apps should be shown.

I think that you should use, instead:
http://lostclus.linux.kiev.ua/scripts/icewm-xdg-menu

In my tests it is much better. Moreover it has less dependencies.
In my opinion something like the following would be nice:

prog xterm xterm xterm
prog Web htmlview htmlview
separator
menuprog Applications - icewm-xdg-menu
menufile Programs folder programs
menufile Tool_bar folder toolbar

And maybe without 
menufile Programs folder programs
it seems to be useless.

> 3. I need to integrate icewm into gdm. I'll do it ASAP.

Indeed, it is a must. I'll attach an icewm.desktop file you 
could drop in %_datadir/xsessions

Comment 15 Patrice Dumas 2007-01-16 21:17:53 UTC
Created attachment 145738 [details]
desktop file for icewm for *dm sessions

Comment 16 Patrice Dumas 2007-01-16 21:29:40 UTC
(In reply to comment #14)

> I think that you should use, instead:
> http://lostclus.linux.kiev.ua/scripts/icewm-xdg-menu

Trouble with that is that it is a bit slow so it would be better
if the menu could be generated once and not everytime the user
clicks. Don't know if it is feasible in icewm, though. In any
case it seems to me to be way better that icewm-menu-gnome2

Comment 17 Patrice Dumas 2007-01-16 21:43:07 UTC
Created attachment 145744 [details]
icewm.desktop using icewm-session

Comment 18 Patrice Dumas 2007-01-16 21:48:29 UTC
(In reply to comment #12)
> #11.
> 
> I'm a new member in the extras' world.
> If I add kde-config to BR (or actually kdelibs), whouldn't it add REQ in install
> time?

not necessarily, it depends whether it is used and needed or not. 
In our case I think it is unuseful at build and install time, instead
icewm-xdg-menu should be used.

In my opinion icewm-menu-gnome2 shouldn't be shipped in the icewm 
main package but in a icewm-gnome subpackage or not shipped at all.

Comment 19 Gilboa Davara 2007-01-17 07:46:17 UTC
* Lang.
Changed to %find_lang %{name} -f %{name}.lang. Done.

* ICE_CXX_FLAG_ACCEPT:
OK. I understand. (Admittedly I'm not that experienced with automake/autoconf -
so bear with me.)
Does the default flag set poses any problem?
I'm reluctant to mock around with the default flags - I've been bitten by GCC
too many times to take this matter lightly.

* htmlview:
Done. Switched to bluecurve icons instead.

* BZ #222905 (gnome-menu):
Ay... I CC'ed myself. Once it changes I'll replace the gnome-menu with redhat-menus.

* Default configuration. (/etc/icewm):
OK, I'll return them to their original position. Never the less, I'll be wise to
sym-link /etc/icewm/xxx to /usr/share/icewm/xxx as it's the logical place for
system-wide configuration files.

* GNOME/KDE:
As I'm using the freedesktop files (/share/desktop-directories/), both menus
should be identical, no?

* icewm-xdg-menu, wm-session:
Thanks. Integrated.
It's a bit (?) slow. I'm thinking about using gnome-menu with a static list of
directories (E.g. Utilities, Office, Development, Games, Internet, Multimedia)

- Gilboa


Comment 20 Gilboa Davara 2007-01-17 07:47:15 UTC
* Wed Jan 17 2007 <gilboad AT gmail DOT com> - 1.2.30-5
- Fix Source0 URL.
- Replace cp with install.
- Do not gzip the man page, just copy it.
- Use htmlview instead of firefox.
- Use BlueCurve icons instead of the mozilla ones.
- Re-fix lang support.
- Return the default configuration files to %%_datadir
- Add gdm session support.
- Remove gnome-menus from default menu - replace it with pyxdg/icewm-xdg-menu.

Spec URL: http://gilboadavara.thecodergeek.com/icewm.spec
SRPM URL: http://gilboadavara.thecodergeek.com/icewm-1.2.30-5.src.rpm

- Gilboa

Comment 21 Patrice Dumas 2007-01-17 19:59:32 UTC
Issues
======

* You should remove %config from default config files. Indeed %config
  is for files to be modified by the user. It is not the case
  for the files in %_datadir/icewm which hold the defaults and therefore
  should not be modified. You, as a packager are the one who can modify
  the files. Users can make system-wide changes in %{_sysconfdir}/icewm/
  files, and each user makes his changes in ~/.icewm/

* with icewm-xdg-menu.py in /usr/bin, rpm creates files used for
  optimization of python. I don't know if it is problematic or
  right. rpmlint complains, but it may be ignorable. 
W: icewm non-executable-in-bin /usr/bin/icewm-xdg-menu.pyo 0644
W: icewm non-executable-in-bin /usr/bin/icewm-xdg-menu.pyc 0644

* /usr/bin/icewm-menu-gnome2
  should in my opinion be in a subpackage, to avoid bringing in 
  gnome dependencies to the main icewm package.

* icewm.desktop should be 0644, so the install line should be
install -p -m 644 %{SOURCE4} %{buildroot}%{_datadir}/xsessions/

* missing
Requires: xterm

* did you check the trademark issues with redhat-web-browser 
  artwork?

* License seems to be LGPL, from what I can see in COPYING,
  and on the sourceforge site.
  Many files lack a license notice, they only have a copyright. 
  No license means a restrictive license (no right to modify 
  or redistribute). Given that all the files which have a notice
  mention the LGPL, I guess there is no real issue. According 
  to the FSF, the authors may not be completely sure that they
  can defend their license in a court unless they have the notice
  that appears at the end of the LGPL. Maybe you could report that
  upstream.
  Some files don't have a copyright notice. Usually it means that
  they are in the public domain (which is not an issue).

Comments
========

* in my opinion the glob for the man page is better like
%{_mandir}/man1/icewm.1*
  to get man pages even without compression

* 
%find_lang %{name}
  seems to be enough, I guess the default is %{name}.lang for the
  file name.

* you should consistently use %{__install} or install

* you can use -b for patches to help those who like gendiff, so for
  example one would have

%patch0 -p1 -b .configure

  and so on

* in general it is not useful to repeat the package name in the summary,
  all the tools get the package name from the package name. Not a big 
  deal.

* I don't think that ' for the X Windows System' is really useful since
  it doesn't add any useful information. I would have choosed 
  something like
Summary:                Light Window Manager


Comment 22 Patrice Dumas 2007-01-17 20:32:34 UTC
(In reply to comment #19)

> * ICE_CXX_FLAG_ACCEPT:
> OK. I understand. (Admittedly I'm not that experienced with automake/autoconf -
> so bear with me.)
> Does the default flag set poses any problem?
> I'm reluctant to mock around with the default flags - I've been bitten by GCC
> too many times to take this matter lightly.

I think that it would be right if you added a comment in the 
specfile to state something along that

# the following compile flags are set in configure:
# -fpermissive -Wall -Wpointer-arith -Wconversion -Wwrite-strings 
# -Winline -Woverloaded-virtual -W -fno-exceptions -fno-rtti

Then you could have a look at gcc man page to verify that nothing
potentially wrong is done.


> Ay... I CC'ed myself. Once it changes I'll replace the gnome-menu with
redhat-menus.

In fact I am not sure that any of these dependency is usefull. They 
will certainly be used if present. It is right in the current spec
file as these deps are not there.

> * Default configuration. (/etc/icewm):
> OK, I'll return them to their original position. Never the less, I'll be wise to
> sym-link /etc/icewm/xxx to /usr/share/icewm/xxx as it's the logical place for
> system-wide configuration files.

Certainly not. /usr/share/icewm/xxx is for defaults from the vendor (ie you)
while /etc/icewm/xxx is for system-wide configuration files.

> * GNOME/KDE:
> As I'm using the freedesktop files (/share/desktop-directories/), both menus
> should be identical, no?

I guess so. 

> * icewm-xdg-menu, wm-session:
> Thanks. Integrated.
> It's a bit (?) slow. I'm thinking about using gnome-menu with a static list of
> directories (E.g. Utilities, Office, Development, Games, Internet, Multimedia)

That could be right. But gnome-menu2 isn't only broken because it adds
empty submenus, it also miss some apps. For example I couldn't find
gnochm nor kchmviewer (I maintain them, that's why I 'test' menus and
mime opening apps on them).

What about asking upstream for a possibility to have something like 
menuprog, but which is called only when starting icewm?

> - Gilboa
It is not wrong to add your name in bugzilla tickets, but it is 
not strictly necessary, since it is already at the top of the tickets 
(with your mail address for logged in people). Sorry if you already
know it and do it on purpose...

Comment 23 Gilboa Davara 2007-01-18 17:26:22 UTC
* CXXFLAGS:
I checked it back when you first pointed it out. Nothing too major. Mostly added
warning and reduced C/Cxx bloat.
The permissive and no-exceptions are... a bit odd, but neither seem to pose any
danger.

* "What about asking upstream for a possibility to have something like 
menuprog, but which is called only when starting icewm?"

I think I have a better solution (at least short-term one): By default
icewm-session tries to execute the "startup" script (if it exists) within the
user's .icewm directory.
While not perfect, I should be able to create a default script that will use the
icewm-xdg-menu.py to generate static menu items.
However, such a solution may require recursive bash scripting and I rather take
the time to think about it.
(Plus, an ideal script should also generate the icon-line for each and every
menu item).

* "did you check the trademark issues with redhat-web-browser artwork?"

OK. I vented it with my sponsor and the icon is GPL. No problems there.
However, I decided it's useless the pack the icon twice.
I just added redhat-artwork to REQ and pointed the toolbar/menu to the original
icon(s).

* GPL/LGPL:

Somehow I got the idea that the xdg menu script was GPL, and therefore I decided
to switched everything to GPL to keep everything kosher.
Can't seem to find it now. My mistake, I guess.
Switching back to LGPL.

* Signature:

Bad habit of mine.
It helps me locate my own posts in long threads.


* Tue Jan 18 2007 <gilboad AT gmail DOT com> - 1.2.30-6
- Change license back to LGPL.
- Change summery.
- New sub-package: -gnome. (GNOME menu support.)
- Missing REQ: xterm.
- Missing REQ: htmlview.
- Remove redundant %%_sysconf section.
- Remove redundant redhat-xxx icons.
- New REQ: redhat-artwork. (icons)
- Better man pages handling.
- Customize keys to better match fedora.
- New REQ: eject. (keys)
- New REQ: alsautils. (keys)


Spec URL: http://gilboadavara.thecodergeek.com/icewm.spec
SRPM URL: http://gilboadavara.thecodergeek.com/icewm-1.2.30-6.src.rpm


Comment 24 Patrice Dumas 2007-01-19 01:17:53 UTC
There is a missing requires for:
/usr/share/pixmaps/gnome-terminal.png
but maybe there could be a more neutral default? Also
from reading the docs it seems that a png in /usr/share/pixmaps/
is used automatically, it would be better to omit the path, then.

Regarding the -gnome subpackage, the BuildRequires should be
in the main package. The main package is the only source package, 
subpackages are binary packages.

Source1 should be with full url.

Your %attr line for icewm-xdg-menu.* isn't completly right, since
the files end up being owned by the user doing the rpm build.
I think it should be
%attr(0755,root,root) %{_bindir}/icewm-xdg-menu.*

regarding man pages and globs, you misunderstood my comment 
(which wasn't clear...). A line of code is better than a long
explanation, so I think the best would be, in %install:

install -p -m 644 doc/icewm.1.man $RPM_BUILD_ROOT/%{_mandir}/man1/icewm.1

And in %files:

%{_mandir}/man1/icewm.1*


Please remove the dot at the end of the -gnome subpackage summary.

The /etc is hardcoded for the System configuration directory. To have
instead %{_sysconfdir} used, you should add to %configure:

   --with-cfgdir=%{_sysconfdir}/icewm



Comment 25 Patrice Dumas 2007-01-19 01:23:03 UTC
(In reply to comment #23)
> * CXXFLAGS:

Right

> * "What about asking upstream for a possibility to have something like 
> menuprog, but which is called only when starting icewm?"
> 
> I think I have a better solution (at least short-term one): By default
> icewm-session tries to execute the "startup" script (if it exists) within the
> user's .icewm directory.
> While not perfect, I should be able to create a default script that will use the
> icewm-xdg-menu.py to generate static menu items.
> However, such a solution may require recursive bash scripting and I rather take
> the time to think about it.
> (Plus, an ideal script should also generate the icon-line for each and every
> menu item).

Also it implies messing with users config file which is not very clean.

> * "did you check the trademark issues with redhat-web-browser artwork?"
> 
> OK. I vented it with my sponsor and the icon is GPL. No problems there.

The copyright of the artwork is a different issue than the trademark
associated with the same artwork. Anyway the issue is moot now.

> However, I decided it's useless the pack the icon twice.
> I just added redhat-artwork to REQ and pointed the toolbar/menu to the original
> icon(s).

Right (but see my comment about the path no required).

> * Signature:
> 
> Bad habit of mine.
> It helps me locate my own posts in long threads.

You can continue, if you find it useful

Comment 26 Patrice Dumas 2007-01-19 01:56:47 UTC
You still use %{__install} and install inconsistently

Comment 27 Gilboa Davara 2007-01-20 07:15:26 UTC
* Icons:

There seem to be an upstream bug about it.
While it -should- use the icons in /usr/share/pixmaps, it doesn't - a full URL
is needed to get it working.
I'll investigate it further once I finish getting the initial review out.
Either way, I'm pointing the menu and toolbar files to BlueCurve to reduce the
dep-chain to redhat-artwork. (Which, AFAIK is installed by default)

* %{__install}:

Fixed.

* Auto-generate menu:

A simple solution will be to add an entry within menu that points to another
file say, "auto-generated-menu-entry-do-not-edit".
If the user wants to disable this script he can either:
A. Delete the entry from his menu file.
B. Create his own startup script that will over-ride the "default" one.

* A couple of simple question: (More of a user-opinion)
- I'm thinking about adding xscreensaver to the dep chain.
Autostart it using startup and add Ctrl+Alt+L to activate it. (xscreensaver-command)
Do you see any problem with auto-starting xscreensaver by default?
- Second, I'm thinking about doing adding menu support to a couple of key
system-config-xxx applets. (desktop, screensaver, mouse, sound, etc)
- Third. How about adding xrdb -load ~/.Xdefaults by default?


* Sat Jan 20 2007 <gilboad AT gmail DOT com> - 1.2.30-7
- Fix source1 URL.
- Fix xdg-menu* owner.
- Replace default terminal icon to reduce dep-chain.
- Fix icewm-gnome description.
- Replace install with %%{_install}
- Push -gnome's BR to main package.
- Change hard-coded sysconf path.

Spec URL: http://gilboadavara.thecodergeek.com/icewm.spec
SRPM URL: http://gilboadavara.thecodergeek.com/icewm-1.2.30-7.src.rpm

Comment 28 Patrice Dumas 2007-01-20 13:58:18 UTC
(In reply to comment #27)

> Either way, I'm pointing the menu and toolbar files to BlueCurve to reduce the
> dep-chain to redhat-artwork. (Which, AFAIK is installed by default)

That seems right to me.

> * Auto-generate menu:
> 
> A simple solution will be to add an entry within menu that points to another
> file say, "auto-generated-menu-entry-do-not-edit".
> If the user wants to disable this script he can either:
> A. Delete the entry from his menu file.
> B. Create his own startup script that will over-ride the "default" one.

That's more or less how it is done for fluxbox. It seems to be right
to me. (in fluxbox the conf is directly copied into the user home
directory, I think that it is preferable to have the default shipped by
fedora in %_datadir/icewm. Also the whole menu is generated, not 
only an entry, but I modified it to have only an entry generated...).

> * A couple of simple question: (More of a user-opinion)
> - I'm thinking about adding xscreensaver to the dep chain.
> Autostart it using startup and add Ctrl+Alt+L to activate it.
(xscreensaver-command)
> Do you see any problem with auto-starting xscreensaver by default?

I don't think this should be done unless it is done upstream.
There could be a comment in startup file, however to indicate 
something along

# to have a screensaver started automatically, install the
# xscreensaver package and uncomment the following line
# xscreensaver &


> - Second, I'm thinking about doing adding menu support to a couple of key
> system-config-xxx applets. (desktop, screensaver, mouse, sound, etc)

No, they should be brought in by pyxdg or similar, if it is not the
case it should be handled at that level, such that it is also right
in fluxbox, pekwm...

> - Third. How about adding xrdb -load ~/.Xdefaults by default?

Isn't that done by session startup scripts?
I have checked, it is done in 
/etc/X11/xinit/xinitrc-common

> * Sat Jan 20 2007 <gilboad AT gmail DOT com> - 1.2.30-7
> - Fix source1 URL.

It is not completely right, since the file isn't there, if I recall well
there is no .py in the original url



Please remove the dot at the end of the -gnome Summary.

Seems like we're not far from being done. 

Comment 29 Gilboa Davara 2007-01-20 17:39:21 UTC
OK.
All the bugs fixed.

Time to introduce new ones.
I broke the icewm-xdg-menu into a new sub-package and created a bash script that
uses its output to generate static menu files. (And locate the best matching icons)
The script itself seems to work just fine on two of my machines (Minus some wine
problems) - it should be stable enough for wide-testing.
For now, it requires manual activation. (icewm-generate-menu
_default_icon_theme_name_)

* Sat Jan 20 2007 <gilboad AT gmail DOT com> - 1.2.30-8
- Fix source0 URL. (2nd is a winner)
- Fix -gnome summery.
- New sub-package: icewm-xdg-menu
- ALPHA: icewm-generate-menu script added to use icewm-xdg-menu to generate
static menus.

Spec URL: http://gilboadavara.thecodergeek.com/icewm.spec
SRPM URL: http://gilboadavara.thecodergeek.com/icewm-1.2.30-8.src.rpm
Script URL: http://gilboadavara.thecodergeek.com/icewm-generate-menu


Comment 30 Gilboa Davara 2007-01-20 17:42:23 UTC
Oh... I didn't test it with xinit, but when I start icewm from gdm, .Xdefaults
doesn't get loaded automatically.

- Gilboa

Comment 31 Patrice Dumas 2007-01-21 13:22:13 UTC
(In reply to comment #30)
> Oh... I didn't test it with xinit, but when I start icewm from gdm, .Xdefaults
> doesn't get loaded automatically.

That's strange. In /usr/share/gdm/defaults.conf:

BaseXsession=/etc/X11/xinit/Xsession

In /etc/X11/xinit/Xsession
. /etc/X11/xinit/xinitrc-common


Comment 32 Patrice Dumas 2007-01-21 13:35:11 UTC
* In %files the second line is redundant:
%attr(0755,root,root) %{_bindir}/icewm-xdg-menu*
%{_bindir}/icewm-xdg-menu*

* Instead of using your script, I think it would be better to 
  modify icewm-xdg-menu. There is already the option --entire-menu.
  I have taken a function findIcon from fluxbox-xdg-menu and added 
  --with-icons and --theme options. I think that with that patch

icewm-xdg-menu --with-icons --entire-menu 

  does the same than your script, but is faster...

* The license for icewm-xdg-menu is missing. I will contact upstream
  to submit my patch and ask him.

* there is no need to split an automenu subpackage for an additional 
  script. The changelog entry is a bit strange since it states
  sub-package: icewm-xdg-menu although the subpackage name is 
  icewm-automenu. 

* there is a missing dot at the end of the -gnome subpackage %description

Comment 33 Patrice Dumas 2007-01-21 13:37:00 UTC
Created attachment 146086 [details]
add --with-icon and --theme options to icewm-xdg-menu

Comment 34 Gilboa Davara 2007-01-25 06:22:08 UTC
Created attachment 146500 [details]
Modified icemw-xdg-menu patch

Patrice,

Did you get the author's permission to mark the icemw-xdg-menu script as GPL?
I've modified your patch (Small bug: broken theme name parsing; added icon_size
support - instead of the default 48x48)
Please review my modification (this is the first time I touch python code...).

- Gilboa

Comment 35 Gilboa Davara 2007-01-25 06:37:05 UTC
As for the package, the automenu subpackage was designed to reduce the dep-chain
in the main icewm package. (xdg-menu)
Changed the sub-package name to xdgmenu

* Thu Jan 25 2007 <gilboad AT gmail DOT com> - 1.2.30-9
- Remove redundant icewm-xdg-menu* %%file entry.
- Change sub-package name to xdgmenu.
- Move icewm-xdg-menu to xdgmenu sub-package.

Spec URL: http://gilboadavara.thecodergeek.com/icewm.spec
SRPM URL: http://gilboadavara.thecodergeek.com/icewm-1.2.30-9.src.rpm

- Gilboa

Comment 36 Gilboa Davara 2007-01-25 06:39:30 UTC
Changelog should include:
- Removed the icewm-generate-menu script.

Comment 37 Patrice Dumas 2007-01-25 10:47:53 UTC
Created attachment 146523 [details]
patch with upstream revisions and icon size command line switch

Comment 38 Patrice Dumas 2007-01-25 11:07:37 UTC
I have checked that the result is nice with unnoticeable delays
with .icewm/startup:

#! /bin/sh

icewm-xdg-menu --entire-menu --with-theme-paths > ~/.icewm/programs

And .icewm/menu:
# This is an example for IceWM's menu definition file.
#
# Place your variants in /etc/icewm or in $HOME/.icewm
# since modifications to this file will be discarded when you
# (re)install icewm.
#
prog xterm /usr/share/icons/Bluecurve/32x32/apps/gnome-terminal.png xterm
prog "Web browser" /usr/share/icons/Bluecurve/32x32/apps/mozilla-icon.png htmlview
separator
menufile Programs folder programs


Comment 39 Patrice Dumas 2007-01-28 09:58:13 UTC
The icewm-xdg-menu patch has been incorporated upstream.

Another comment is that I don't think that icewm-xdg-menu
deserves to be in a separate subpackage, since python is
likely to be installed on every desktop and most servers.

Comment 40 Gilboa Davara 2007-01-28 13:54:49 UTC
While the pyxdg and the gnome-menus packages may not be big enough to warrant a
sub-package, adding 20-30 second to each login is.
Breaking icewm-xdg-menu into a separate sub-package creates a better out of the
box experience to those who need auto-generated menus (by auto-enabling it),
while keeping it KISS, to those who don't. (Low-end machines, servers, etc)

* Sun Jan 28 2007 <gilboad AT gmail DOT com> - 1.2.30-10
- Missing REQ: icewm (both -gnome and -xdgmenu)
- Updated menu.in patch.
- Updated startup script. (-xdgmenu)
- Updated icewm-xdg-menu script. (-xdgmenu)
	
Spec URL: http://gilboadavara.thecodergeek.com/icewm.spec
SRPM URL: http://gilboadavara.thecodergeek.com/icewm-1.2.30-10.src.rpm

- Gilboa


Comment 41 Patrice Dumas 2007-01-29 13:32:21 UTC
Your argument about having separate xdgmenu subpackage seems
relevant to me.

[ -f ~/.Xdefaults ] && xrdb -load ~/.Xdefaults
should be handled by the session opening. This doesn't belong
here. I showed above that this should be handled in xinit and
gdm, if it isn't then you should fill a bug against the relevant
package, not try to workaround it.

Also I think it would be better to have the startup file in 
the main package, but it wouldn't do anything interesting 
if icewm-xdg-menu isn't installed. It could be generated at
%install time, like

cat > %{buildroot}%{_datadir}/icewm/startup <<EOF
[ ! -d ~/.icewm ] && mkdir ~/.icewm
if [ -x %{_bindir}/icewm-xdg-menu ]; then
  %{_bindir}/icewm-xdg-menu --entire-menu --with-theme-paths --icon-size 16
--theme Bluecurve > ~/.icewm/programs.autogen
fi
EOF

chmod 0755 %{buildroot}%{_datadir}/icewm/startup


Also you should use consistently %{buildroot} or $RPM_BUILD_ROOT.

Comment 42 Gilboa Davara 2007-02-04 09:23:43 UTC
I'm a KISS kind of guy. If it's all the same to you, I rather keep the startup
file as a patch/source.

* Sun Feb 04 2007 <gilboad AT gmail DOT com> - 1.2.30-11
- Remove .Xdefaults fix from startup. (reported upstream).
- Replace buildroot with RPM_BUILD_ROOT.

Spec URL: http://gilboadavara.thecodergeek.com/icewm.spec
SRPM URL: http://gilboadavara.thecodergeek.com/icewm-1.2.30-11.src.rpm

- Gilboa


Comment 43 Patrice Dumas 2007-02-05 16:53:36 UTC
(In reply to comment #42)
> I'm a KISS kind of guy. If it's all the same to you, I rather keep the startup
> file as a patch/source.

A HERE-doc isn't less KISS. I would have preferred the startup script
in the main package and not in the xdgmenu subpackage, but I won't make
it a blocker.

Remaining issues:

* Add a dot at the end of the %description    gnome

* in xdgmenu the %files section could be
%defattr(-,root,root,-)
%{_bindir}/icewm-xdg-menu*
%{_datadir}/icewm/startup

with an install command for startup script of
%{__install} -p -m 755 %{SOURCE4} $RPM_BUILD_ROOT%{_datadir}/icewm/startup

* sub packages should depend on a full versioned main package, like
Requires:               icewm = %{version}-%{release}


Comment 44 Patrice Dumas 2007-02-05 17:07:03 UTC
(In reply to comment #43)

> * in xdgmenu the %files section could be

It could be:
%defattr(-,root,root,-)
%{_bindir}/icewm-xdg-menu
%{_datadir}/icewm/startup


Comment 45 Gilboa Davara 2007-02-10 10:45:12 UTC
Fixed.

* Sat Feb 10 2007 <gilboad AT gmail DOT com> - 1.2.30-12
- Add missing dot in the -gnome sub-package description.
- Replace REQ icewm (in both -gnome and -xdgmenu) with icewm-x.x.x.
- Fix -xdgmenu file list and %%install section.
- Preserve the source time-stamp.


Comment 46 Patrice Dumas 2007-02-10 11:29:55 UTC
There is a link to the srpm missing, but I found it anyway...

Comment 47 Patrice Dumas 2007-02-10 11:51:48 UTC
* rpmlint output is ignorable
W: icewm-gnome no-documentation
W: icewm-xdgmenu no-documentation
* license is LGPL, included 
I thing that you should report upsteram what I say in Comment #21
* source match upstream
 md5sum  icewm-1.2.30.tar.gz icewm-xdg-menu
8a302c5e629bb81d87cc02004a694ece  icewm-1.2.30.tar.gz
85eeefb3335e40fcaed392c61892cc72  icewm-xdg-menu
* %files section is right

APPROVED

Just 2 last suggestions, 
* maybe icewm could own %{_sysconfdir}/icewm but leave it empty.

* in the xdgmenu files section, the icewm-xdg-menu line could be
%{_bindir}/icewm-xdg-menu

Since it is approved you can change that after importing to cvs or keep it.

Comment 48 Grant Gainey 2007-02-14 15:04:31 UTC
*** Bug 227732 has been marked as a duplicate of this bug. ***

Comment 49 Pavel Roskin 2007-02-17 04:46:31 UTC
IceWM 1.2.30-12.fc6 depends on obsolete libraries glib 1.x, gtk+ 1.x and imlib
1.x.  All this can be avoided if IceWM is compiled against Xpm library, which is
not obsolete.  This can be done by passing --with-xpm on the configure command
line.  That's how I have been compiling IceWM for many years, and it has always
worked fine for me.  I believe it would only make any difference if images other
than Xpm are used in the themes, which in not the case for any theme I know.

Alternatively, please consider icewm-1.3.0, which uses gtk+ 2.x and supports
NET_WM_ICON.  It's working just fine for me.  I don't think stable/unstable
really matters for extras, especially if the newer version is stable de facto
and doesn't bring obsolete dependencies.

Comment 50 Patrice Dumas 2007-02-17 09:18:31 UTC
(In reply to comment #49)
> IceWM 1.2.30-12.fc6 depends on obsolete libraries glib 1.x, gtk+ 1.x and imlib
> 1.x.  All this can be avoided if IceWM is compiled against Xpm library, which is
> not obsolete.  This can be done by passing --with-xpm on the configure command
> line.  That's how I have been compiling IceWM for many years, and it has always
> worked fine for me.  I believe it would only make any difference if images other
> than Xpm are used in the themes, which in not the case for any theme I know.

imlib is in fedora, and we need to handle png, jpeg at least for menu 
icons, so I don't think using xpm over imlib would be a good idea. And even 
if it is for themes it seems to me that imlib is superior; besides I 
guess (hope) that libxpm is in maintainance mode for many years now.

> Alternatively, please consider icewm-1.3.0, which uses gtk+ 2.x and supports
> NET_WM_ICON.  It's working just fine for me.  I don't think stable/unstable
> really matters for extras, especially if the newer version is stable de facto
> and doesn't bring obsolete dependencies.

unstable/stable doesn't matter for fedora devel (rawhide) but matters for
the other releases. And don't forget that what is in Fedora 6 could land
in EPEL. 

In the case of icewm 1.2.30 versus 1.3.0 I had a look at the diff it is huge,
although most seems to be simple changes. This could be a case where it
could be a good idea to use 1.3.0, not for the new dependency, in my 
opinion, but to give more test coverage for the new icewm version. In my 
opinion this should left to the maintainer decision since both have 
advantages and inconvenients.

Maybe it depends on how Gilboa is near from upstream. Another possibility
would be to keep icewm-1.2.30 for older branches and use 1.3.0 in devel to
have it for fedora 7. And a last possibility would be update it right
after the fedora 7 release to have more time to test it during the whole 
fedora 8 devel.

Comment 51 Gilboa Davara 2007-02-20 11:01:54 UTC
Hello all.
Sorry the late reply.

1.3.0 seems to suffer from a lack of documentation (on what changed between
1.2.30 and 1.3.x) which is not very reassuring.
In my view, people that require IceWM (for old machines, server) will most
likely require a stable DE instead of a cutting edge one.

According to the owner's list, while not longer maintained upstream, imlib,
gtk+ and glib are all actively maintained; hopefully, they'll remain active
long enough for 1.3.x to mature.

- Gilboa



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