Not an official review, just a few thoughts:

I suggest to name this package gnome-applet-service-discovery, see
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Addon_Packages_.28General.29

Better name a svn snapshot after the revision and not the date, because then the
reviewer can simply check out the correrct version with
svn co -rXXXXX svn://svn.0pointer.de/service-discovery-applet/trunk
service-discovery-applet

I see you define both python_sitelib and python_sitearch. Please remove the
unneeded definition.

You are missing a requirement:
$ service-discovery-applet 
Traceback (most recent call last):
  File "/usr/bin/service-discovery-applet", line 51, in <module>
    error_msg(_("A required python module is missing!\n%s") % (e))
  File "/usr/bin/service-discovery-applet", line 29, in error_msg
    d = gtk.MessageDialog(parent=None, flags=gtk.DIALOG_MODAL,
NameError: global name 'gtk' is not defined

running service-discovery-config gives a messagebox saying:
"Ein benötigtes Python Modul fehlt!
No module named avahi"

Not sure if we have a python-avahi package

(In reply to comment #1)
> Not an official review, just a few thoughts:
> 
> I suggest to name this package gnome-applet-service-discovery, see
>
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Addon_Packages_.28General.29

Hmm, this is different from upstream. My first search was for
service-discovery-applet so I wouldn't have found it with that name. Would yum
search provides lines to remedy this problem?

> Better name a svn snapshot after the revision and not the date, because then the
> reviewer can simply check out the correrct version with
> svn co -rXXXXX svn://svn.0pointer.de/service-discovery-applet/trunk
> service-discovery-applet

So you can with the date, just use svn co -r{DATE}... Having a date usually
makes it easier to get a feeling how up2date the package really is. I've used
this for other packages already. Just assume someone tried to do this on git or
cvs stuff...

> I see you define both python_sitelib and python_sitearch. Please remove the
> unneeded definition.

I usually keep it for easy spawning new packages by copying. I can't see in the
guidelines that I shouldn't.

> You are missing a requirement:
> $ service-discovery-applet 
> Traceback (most recent call last):
>   File "/usr/bin/service-discovery-applet", line 51, in <module>
>     error_msg(_("A required python module is missing!\n%s") % (e))
>   File "/usr/bin/service-discovery-applet", line 29, in error_msg
>     d = gtk.MessageDialog(parent=None, flags=gtk.DIALOG_MODAL,
> NameError: global name 'gtk' is not defined
> 
> running service-discovery-config gives a messagebox saying:
> "Ein benötigtes Python Modul fehlt!
> No module named avahi"
> 
> Not sure if we have a python-avahi package

Ah, needs to require avahi-tools, which contains the Python bindings for some
reason... Thanks for pointing this out!

I've uploaded a new SPEC (same URL as above) and new SRPM (at
http://fedorapeople.org/~timn/misc/service-discovery-applet-0.4.5-0.3.svn20080609.fc9.src.rpm).

(In reply to comment #2)
> My first search was for
> service-discovery-applet so I wouldn't have found it with that name. Would yum
> search provides lines to remedy this problem?

No, unfortunately not. 'yum install service-discovery-applet' would work but not
'yum search'. IMO this is something that should be fixed in yum. Nevertheless we
should stick to the naming guidelines I think.

> So you can with the date, just use svn co -r{DATE}... Having a date usually
> makes it easier to get a feeling how up2date the package really is. 

Ok, agreed as it's in line with the naming guidelines.

> I usually keep it for easy spawning new packages by copying. I can't see in the
> guidelines that I shouldn't.

It's written in the python spec template from rpmdev-newrpmspec, but in the end
it's your decision.

> Ah, needs to require avahi-tools, which contains the Python bindings for some
> reason... Thanks for pointing this out!

But I still don't see a Requires: on avahi-tools, only the BuildRequires that
has been there before. :(

> I've uploaded a new SPEC (same URL as above) and new SRPM (at
>
http://fedorapeople.org/~timn/misc/service-discovery-applet-0.4.5-0.3.svn20080609.fc9.src.rpm).

Ok, a few more comments on that one:

Why not change 
  # cd service-discovery-applet
  # svn export service-discovery-applet service-discovery-applet-0.4.5.svn`date
+%Y%m%d`
  # cd ..
to 
  # svn export . ../service-discovery-applet-0.4.5.svn`date +%Y%m%d`

  tar cvfz ...
needs to  be
  tar -cvfz ...

BuildRoot tag is wrong, according to the guidelines it should be
  "%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)"

BuildRequires "perl-XML-Parser" should be "perl(XML::Parser)" instead

Are you sure you need gettext-devel and not only gettext?

You have a lot of redundant build requirements that don't need to be specified
explicitly:
- libtool requires autoconf and automake, automake requires autoconf and perl
- GConf2-devel requires automake and glib2-devel
- intltool already requires gettext and perl(XML::Parser)
- perl(XML::Parser) of course requires perl
As long as you don't specify a version it is not necessary to list all these BRs.

Use "export GCONF_DISABLE_MAKEFILE_SCHEMA_INSTALL=1" to avoid error messages due
to missing permissions during %install. You are also missing the necessary
scriptlets to (un)install the gconf schemas, see
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#GConf

Use "make %{?_smp_mflags}", although it won't make much difference.

Please fix %defattr to (-,root,root-). Only cosmetics, I know. ;)

%{_libdir}/bonobo/servers/GNOME_ServiceDiscoveryApplet.server
%{_libdir} will become /usr/lib64 on x86_64, so this package cannot be noarch.

(In reply to comment #3)
> Why not change 
>   # cd service-discovery-applet
>   # svn export service-discovery-applet service-discovery-applet-0.4.5.svn`date
> +%Y%m%d`
>   # cd ..
> to 
>   # svn export . ../service-discovery-applet-0.4.5.svn`date +%Y%m%d`

Oops, I 'fixed' the wrong line. ;( Should read:

Why not change 
  # cd service-discovery-applet
  # svn export . ../service-discovery-applet-0.4.5.svn`date +%Y%m%d`
  # cd ..
to 
  # svn export service-discovery-applet service-discovery-applet-0.4.5.svn`date
+%Y%m%d`

(In reply to comment #3)
> - intltool already requires gettext and perl(XML::Parser)

I have to correct myself: The F-8 intltool package doesn't require gettext (the
Requirements are completely messed up on F-8), so it's better to BuildRequire it
explicitly.

Tim, are there any problems with my suggestions/feedback? If so, please let me know.

I'll be out of the country for the next weeks, I'll be back at the end of July
and then I'll work on this again.

An other small thing...I would suggest to switch the URL from 'http://0pointer.de/cgi-bin/viewcvs.cgi/?root=service-discovery-applet' to 'http://avahi.org', the upstream website.

Is there any progress with this bug?

For what it is worth, I have a replacement for the VNC plugin.  Trying to get it into upstream, and will post here for your convenience.  It handles three of the common VNC viewers, so it is more distro and version independent, and provides an easy means to plugin any other VNC viewer that someone might want.

Created attachment 328742 [details]
Replacement VNC plugin for Service Discovery Applet

This replaces vncviewer.py.in.  It autodetects which vnc viewer is available, and handles the difference in command line parameters including the port / screen difference between XVNC and Vinagre.

I'm currently short in time. I still think the service-discovery-applet should be in Fedora. My time constraints (thesis) will not allow me to do this before March though. Either we wait or someone else takes over.

Please clear NotReady from the whiteboard when this request is ready for a review.

I have lost interest in this, someone else can take over if interested.