https://bugzilla.redhat.com/show_bug.cgi?id=1201925
Bug ID: 1201925 Summary: Review Request: <main package name here> - <short summary here> Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: ville.skytta@iki.fi QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
https://scop.fedorapeople.org/packages/gnome-shell-extension-touchpad-indica... https://scop.fedorapeople.org/packages/gnome-shell-extension-touchpad-indica...
Touchpad Indicator is a minimalistic Touchpad management extension for the Gnome Shell. allows you to switch your Touchpad On or Off, if a Trackpoint, a Fingertouch, a Touchscreen or a Pen exists it is also possible to switch it On or Off through the extension. There's further the possibility to automatically switch On or Off the touchpad and/or trackpoint if a mouse is detected at startup or if a mouse is plugged or the automatic switch off is disabled by default.
Fedora Account System Username: scop
Note to reviewer: I'm not sure if a dependency on xorg-x11-drv-synaptics should be added, i.e. if this package only works with it or if there are some other touchpad implementations that this stuff works with too. If you know, please advice and I can add the dependency. I do know that this works for my synaptics based touchpad even if I remove the /usr/bin/synclient executable so at least that executable is not a dependency.
https://bugzilla.redhat.com/show_bug.cgi?id=1201925
Ville Skyttä ville.skytta@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- Summary|Review Request: <main |Review Request: |package name here> - <short |gnome-shell-extension-touch |summary here> |pad-indicator - | |Minimalistic Touchpad | |management extension for | |the Gnome Shell
https://bugzilla.redhat.com/show_bug.cgi?id=1201925
Alexander Ploumistos alex.ploumistos@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |alex.ploumistos@gmail.com
--- Comment #1 from Alexander Ploumistos alex.ploumistos@gmail.com --- Hi,
This is an informal review and it is also my first, so in all likelihood, I have missed something.
Your spec file looks good. You don't have any of the obsolete commands and sections.
When I ran it through rpmlint there where only a few warnings concerning the spelling of certain words - most of them highly debatable in my opinion:
gnome-shell-extension-touchpad-indicator.src: W: spelling-error Summary(en_US) Minimalistic -> Minimalist, Minimalism, Animistic gnome-shell-extension-touchpad-indicator.src: W: spelling-error %description -l en_US minimalistic -> minimalist, minimalism, animistic gnome-shell-extension-touchpad-indicator.src: W: spelling-error %description -l en_US trackpoint -> track point, track-point, checkpoint gnome-shell-extension-touchpad-indicator.src: W: spelling-error %description -l en_US startup -> start up, start-up, upstart
Personally, I wouldn't bother.
All of the automatic checks ran by fedora-review were OK. Same for the things I had to check by myself, except for one thing: the spec file states that the license is GPLv2+, but licensecheck reported that convenience.js is under the BSD license with the "no advertising" clause.
https://fedoraproject.org/wiki/Licensing:BSD?rd=Licensing/BSD#3ClauseBSD
According to
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/Li...
you should change the license line in your spec file to:
License: GPLv2+ and BSD
and it should be preceded by an explanatory comment, like this one:
# The entire source code is GPLv2+ except convenience.js, which is BSD
See the link above for more, it also states that the %files section should contain a breakdown of the files grouped by license.
In the few tens of spec files I have read, not many people make use of "c=%{commit}" or "d=%{_datadir}/gnome-shell/extensions/%{uuid}", they just repeat things over and over again. Perhaps my sample is not statistically significant and I really have no idea if seasoned packagers would consider it elegant or lazy (I like it though).
Well done and I hope a proven packager will review your package as soon as possible.
https://bugzilla.redhat.com/show_bug.cgi?id=1201925
--- Comment #2 from Ville Skyttä ville.skytta@iki.fi --- Good job with the review, thanks. License tag fixed and comment added:
https://scop.fedorapeople.org/packages/gnome-shell-extension-touchpad-indica... https://scop.fedorapeople.org/packages/gnome-shell-extension-touchpad-indica...
The comment and %files section breakdown etc are in the "...actual implementation of this is left to the maintainer. Some suggested implementations include..." category, so I think the comment is enough (they're suggested, not mandatory, and not all of them need to be done anyway).
https://bugzilla.redhat.com/show_bug.cgi?id=1201925
--- Comment #3 from Alexander Ploumistos alex.ploumistos@gmail.com --- (In reply to Ville Skyttä from comment #2)
The comment and %files section breakdown etc are in the "...actual implementation of this is left to the maintainer. Some suggested implementations include..." category, so I think the comment is enough (they're suggested, not mandatory, and not all of them need to be done anyway).
Fine by me, no need for nitpicking.
Have you by any chance tested the extension with libinput?
https://fedoraproject.org/wiki/Changes/LibinputForXorg
https://bugzilla.redhat.com/show_bug.cgi?id=1201925
--- Comment #4 from Ville Skyttä ville.skytta@iki.fi --- (In reply to Alexander Ploumistos from comment #3)
Have you by any chance tested the extension with libinput?
No. My laptop is still at F-20, not sure if that stuff would be doable with it.
https://bugzilla.redhat.com/show_bug.cgi?id=1201925
--- Comment #5 from Ville Skyttä ville.skytta@iki.fi --- https://scop.fedorapeople.org/packages/gnome-shell-extension-touchpad-indica... https://scop.fedorapeople.org/packages/gnome-shell-extension-touchpad-indica...
- Update to current git for gnome-shell 3.16 support
I'm now running this with libinput on F-22 and it appears to work ok. But then again I also ran without this and did not encounter the accidental touchpad hits nearly as badly as with F-20 so I'm not sure if I'm going to need this package after all.
This upstream commit does not sound too promising either: https://github.com/orangeshirt/gnome-shell-extension-touchpad-indicator/comm... "... but I don't support and develop the extension by myself, perhaps there ist somebody which will help you."
https://bugzilla.redhat.com/show_bug.cgi?id=1201925
--- Comment #6 from Ville Skyttä ville.skytta@iki.fi --- Looks like I'm not going to be needing this package myself, therefore I'm not going to maintain it either.
I'll keep this review request open for a bit in case someone else will take over, will drop it and delete the files in a few weeks if not.
https://bugzilla.redhat.com/show_bug.cgi?id=1201925
--- Comment #7 from Upstream Release Monitoring upstream-release-monitoring@fedoraproject.org --- pbrobinson's scratch build of gnome-dvb-daemon?#451a118009234eac9e3f06f465b00cf93478a5d5 for git://pkgs.fedoraproject.org/gnome-dvb-daemon?#451a118009234eac9e3f06f465b00cf93478a5d5 and rawhide failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11560895
https://bugzilla.redhat.com/show_bug.cgi?id=1201925
Ville Skyttä ville.skytta@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |CLOSED Resolution|--- |WONTFIX Last Closed| |2015-11-03 14:05:50
--- Comment #8 from Ville Skyttä ville.skytta@iki.fi --- Dropped and deleted.
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla