[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=495902





--- Comment #13 from Paul Fox <pgf at laptop.org>  2009-06-05 09:50:45 EDT ---


many thanks!

> 
> Issues:
> - MINOR: BuildRoot: should be
> %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
> see https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag
> (minor, but for a new package we should do this properly I think)

fixed.

> 
> - MAJOR: BuildArch: i386 is wrong, I think you want %{ix86} or no BuildArch at
> all.

fixed.

> 
> - MAJOR: Requires: hal is missing for proper function and dir ownership

fixed.

> 
> - MINOR: Description: line breaks at 80 characters

i'm not sure what you mean.  are you saying my line breaks shouldn't be there? 
won't that make the spec file unreadable?

> 
> - MAJOR: RPM_OPT_FLAGS not honored:
> cc -Wall -O2 -g -DVERSION=6 $(pkg-config --cflags hal) $(pkg-config --cflags
> glib-2.0) $(pkg-config --cflags dbus-glib-1)     olpc-kbdshim-hal.c 
> $(pkg-config --libs hal) $(pkg-config --libs glib-2.0) $(pkg-config --libs
> dbus-glib-1) -o olpc-kbdshim-hal
> cc -Wall -O2 -g -DVERSION=6    olpc-kbdshim.c   -o olpc-kbdshim

fixed.

> 
> - MAJOR: Preserve timestamps during install by adding "-p", see
> https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

fixed, (though i'm opposed on principle.  i think the usefulness of timestamps
is subverted almost entirely by making it appear as if files are older than
they actually are.  but i'll adapt, i guess.  :-)

> 
> 
> Questions:
> - Is the non-hal version still developed? If so, this package should be named
> olpc-kbdshim-hal, so that both version could be packaged

no, only the hal version is being maintained.  i've removed the commented lines
in the spec file, and added a note to the README noting the existence of the
non-HAL version, just in case someone wants it.

> 
> - Is there any way to tag or mark the checkouts, so we can later get a specific
> version and verify it's md5? For a git snapshot this package is not named
> properly, see
> http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Snapshot_packages

i'll research this.  i confess it's a little confusing as the maintainer to be
doing a release based on a sandbox, rather than on a tarball.  can you perhaps
point me at a (simple) git-based package that does this correctly?  a template
would help me here.

> 
> - How can I test the program's functionality? I'm especially interested in the
> brightness because this is something I need for LXDE and Xfce as well.  

there's no configuration:  after installation and a reboot, the following
things should work:
 - with any grab key pressed, both the touchpad and the arrow keys should
    cause scrolling.  on a USB keyboard, the "windows" keys will act as grab
    keys.
 - the rotate and brightness keys should "just work".  these keys cause the
    olpc-rotate and olpc-brightness scripts in /usr/bin to be invoked.
 - when the screen is rotated, the action of the touchpad will be rotated
    as well.  the action of the "d-pad" arrow keys on the screen bezel will
    also be rotated to match.  it's possible for the touchpad and arrow actions
    to get out of sync with the screen if X crashes, but the next use of the
    rotate button will fix this.

that's it.  you should feel free to test on the current package (URL above),
since none of the current review comments have affected its operation.  i'll do
a new package when i've resolved the "description" and tagging issues still
open above.  okay?

thanks again.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.