Some notes:

* About Patch1
  - Would you explain why you want to change Name= item
    from "xine" to "Xine"?

?? About dlopen'ing libX11.so in src/aaui/main.c
  - ... however:
--------------------------------------------------------
[tasaka1@localhost bin]$ ldd -r ./aaxine  | grep libX11
	libX11.so.6 => /usr/lib/libX11.so.6 (0x00793000)
--------------------------------------------------------
    I may be wrong, however would you explain why this
    dlopen() is really needed?

* Timestamps
  - Add INSTALL="install -p" on 'make install' to keep timestamps
    on installed files

* desktop-file-install
  - You don't have to delete desktop file under %_builddir
    (--delete-original). Note that currently
    $ rpmbuild -bi --short-circuit will fail due to this.

* Scriptlets
  - Update GTK icon cache update scriptlets:
    https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache

* %files
  - Would you explain why two different desktop files are installed
    under different places?
----------------------------------------------------------
/usr/share/applications/xine.desktop
/usr/share/xine/desktop/xine.desktop
----------------------------------------------------------
    Note that these two differs slightly.

(In reply to comment #1)
> Some notes:
> 
> * About Patch1
>   - Would you explain why you want to change Name= item
>     from "xine" to "Xine"?

Good question. This was in the rpmfusion package. I dropped the patch altogether.

> ?? About dlopen'ing libX11.so in src/aaui/main.c
>   - ... however:
> --------------------------------------------------------
> [tasaka1@localhost bin]$ ldd -r ./aaxine  | grep libX11
>  libX11.so.6 => /usr/lib/libX11.so.6 (0x00793000)
> --------------------------------------------------------
>     I may be wrong, however would you explain why this
>     dlopen() is really needed?

I don't know, this is from the rpmfusion package and I don't know why it is necessary. The sed is needed however since the unversioned .so file is provided by libX11-devel instead of libX11.

> * Timestamps
>   - Add INSTALL="install -p" on 'make install' to keep timestamps
>     on installed files

Fixed, thanks for the reminder (I was going to fix this earlier but forgot).

I also fixed the charset conversion to preserve timestamps.


> * desktop-file-install
>   - You don't have to delete desktop file under %_builddir
>     (--delete-original). Note that currently
>     $ rpmbuild -bi --short-circuit will fail due to this.

Done.

> * Scriptlets
>   - Update GTK icon cache update scriptlets:
>     https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache

This was already partly done, but I rewrote the part to use the snippets above. Also added the missing mimetype refresh.

> 
> * %files
>   - Would you explain why two different desktop files are installed
>     under different places?
> ----------------------------------------------------------
> /usr/share/applications/xine.desktop
> /usr/share/xine/desktop/xine.desktop
> ----------------------------------------------------------
>     Note that these two differs slightly.  

Good question. Removed the other one.


http://theory.physics.helsinki.fi/~jzlehtol/rpms/xine-ui.spec
http://theory.physics.helsinki.fi/~jzlehtol/rpms/xine-ui-0.99.5-8.fc10.src.rpm

Is this  xine-ui.spec taken from RPM Fusion?

If it is, then why we must reviewing again something what is reviewed earlier?

Because Fedora and rpmfusion are different...

Assigning.

For -8:

* Desktop file
  - Remove "Application" category. This category is marked as
    deprecated. ref:
    http://www.redhat.com/archives/fedora-extras-list/2006-October/msg00723.html

* desktop-file-install
  - Again "--remove-original" is not needed for this case.

?? About aaxine
  - Well, currently launching aaxine on CUI (with framebuffer, vga=791)
    by $ aaxine <some_file>
    just displays nothing and only sound can be heard.
    Also with GUI launching aaxine on terminal emulator (such as
    gnome-terminal) launching another Window and show some AA images
    there, not on the terminal emulator.
    Is this the expected behavior?

(In reply to comment #6)
> For -8:
> 
> * Desktop file
>   - Remove "Application" category. This category is marked as
>     deprecated. ref:
>    
> http://www.redhat.com/archives/fedora-extras-list/2006-October/msg00723.html

Removed.

> * desktop-file-install
>   - Again "--remove-original" is not needed for this case.

Sorry, didn't see it since it was on the same line as desktop-install :) Removed.

> ?? About aaxine
>   - Well, currently launching aaxine on CUI (with framebuffer, vga=791)
>     by $ aaxine <some_file>
>     just displays nothing and only sound can be heard.
>     Also with GUI launching aaxine on terminal emulator (such as
>     gnome-terminal) launching another Window and show some AA images
>     there, not on the terminal emulator.
>     Is this the expected behavior?  

After toying around for some time I found out that aalib output requires support from xine-lib. This is in the xine-lib-extras package, which I have now added as a Requires: and aaxine works.

http://theory.physics.helsinki.fi/~jzlehtol/rpms/xine-ui.spec
http://theory.physics.helsinki.fi/~jzlehtol/rpms/xine-ui-0.99.5-9.fc10.src.rpm

Im getting this issue when pressing DVD or DVB navigator buttons:

cannot find input plugin for MRL [dvd:/]
input plugin cannot open MRL [dvd:/]

but I did not try the latest buid.

Is this OK (at config time)?

checking for NVTVSIMPLE... *** nvtvsimple support will be disabled ***

(In reply to comment #8)
> Im getting this issue when pressing DVD or DVB navigator buttons:
> 
> cannot find input plugin for MRL [dvd:/]
> input plugin cannot open MRL [dvd:/]
> 
> but I did not try the latest buid.  

For dvb you need to have a working channels.conf in your ~/.xine directory.
For dvd you must have a dvd in your drive.

Otherwise you get these errors.

Stupid, yes. A packaging bug, no.


(In reply to comment #9)
> Is this OK (at config time)?
> 
> checking for NVTVSIMPLE... *** nvtvsimple support will be disabled ***  

Yes, since there's been no support in rpmfusion either and xine works without it. NVTV is for enabling TV-out on Nvidia cards.

nvtv upstream is here: http://sourceforge.net/project/showfiles.php?group_id=33758

nvtvsimple has been updated last in 2004 (nvtv-runtime in 2006), so I don't think it's an important addition.

(In reply to comment #10)
> 
> Stupid, yes. A packaging bug, no.
> 
> 
> nvtvsimple has been updated last in 2004 (nvtv-runtime in 2006), so I don't
> think it's an important addition.  


OK than. Now it's clear to me, thanks :)

(In reply to comment #6)
> ?? About aaxine
>   - Well, currently launching aaxine on CUI (with framebuffer, vga=791)
>     by $ aaxine <some_file>

! Well, I found that aaxine accepts the option used
  by aalib as shown in src/aaui/main.c:
---------------------------------------------------------
   638  #ifdef AA
   639    /* 
   640     * AALib help and option-parsing
   641     */
   642    if (!aa_parseoptions(NULL, NULL, &argc, argv)) {
   643      print_usage();
   644      goto failure;
   645    }
   646  #endif
---------------------------------------------------------

Then:
(In reply to comment #7)
> (In reply to comment #6)
> > For -8:
> > 
> > * Desktop file
> >   - Remove "Application" category. This category is marked as
> >     deprecated. ref:
> >    
> > http://www.redhat.com/archives/fedora-extras-list/2006-October/msg00723.html
> 
> Removed.
- You can use "--remove-category" option of "desktop-file-install"
  (as you are already using --add-category) instead of using sed.

-----------------------------------------------------
  This package (xine-ui) is APPROVED by mtasaka
-----------------------------------------------------

True. Thanks for the review!

New Package CVS Request
=======================
Package Name: xine-ui
Short Description: A skinned xlib-based gui for xine-lib
Owners: jussilehtola
Branches: F-9 F-10 F-11 EL-5
InitialCC:

cvs done.

xine-ui-0.99.5-10.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/xine-ui-0.99.5-10.fc9

xine-ui-0.99.5-10.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/xine-ui-0.99.5-10.fc11

xine-ui-0.99.5-10.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/xine-ui-0.99.5-10.fc10

(In reply to comment #2)
> (In reply to comment #1)
> > * Scriptlets
> >   - Update GTK icon cache update scriptlets:
> >     https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache
> 
> This was already partly done, but I rewrote the part to use the snippets 
> above.

The "touch --no-create ..." %post part appears to be missing.

Oops, I also missed it on my review, sorry...

Now closing.
(Please fix GTK icon cache updating scriptlets, sorry)

(In reply to comment #20)
> Now closing.
> (Please fix GTK icon cache updating scriptlets, sorry)  

Whoops, the %post scriplet was missing. Fixed in the -11 release.

xine-ui-0.99.5-11.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

xine-ui-0.99.5-11.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

xine-ui-0.99.5-11.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

xine-ui-0.99.5-15.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/xine-ui-0.99.5-15.el5

xine-ui-0.99.5-15.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.