Bug 696357 - Review Request: fedora-logo-gnome-shell-extension - adds a Fedora logo to Gnome shell's Activities button
Summary: Review Request: fedora-logo-gnome-shell-extension - adds a Fedora logo to Gno...
Keywords:
Status: CLOSED WONTFIX
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mohamed El Morabity
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-04-13 21:43 UTC by Timur Kristóf
Modified: 2012-12-24 11:53 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-12-24 11:53:05 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Timur Kristóf 2011-04-13 21:43:46 UTC
Spec URL: http://sources.venemo.net/fedora-logo-gnome-shell-extension.spec
SRPM URL: http://sources.venemo.net/fedora-logo-gnome-shell-extension-1.0.0-1.fc15.src.rpm
Description: fedora-logo-gnome-shell-extension is a simple Gnome shell extension
that adds the Fedora logo to the Activities button
of the Gnome shell.

This is a gnome-shell extension that adds a Fedora logo to the Activities button of the Shell. (On right-to-left locales it appears on the top right corner, otherwise on the top left corner.)

The extension is supposed to work with any Gnome shell theme.
(Tested with some popular ones and the default one.)

Link to the idea on the design team's mailing list:
http://lists.fedoraproject.org/pipermail/design-team/2011-April/004215.html

Comment 1 Mohamed El Morabity 2011-04-14 16:19:15 UTC
I will review this package.

Comment 2 Mohamed El Morabity 2011-04-15 02:54:17 UTC
A quite good package, simple but works well :). A few comments anyway:

* maybe your package should be renamed « gnome-shell-extension-fedora-logo » or « gnome-shell-fedora-logo », at least to respect the naming guidelines for addons packages (see http://fedoraproject.org/wiki/PackageNamingGuidelines#Addon_Packages_.28General.29).

* the fedora-logos package provides a « system-logos » capacity, as well as the generic-logos package (and probably the redhat-logos in RHEL also). Why not setting system-logos as Requires instead of fedora-logos, so that your package would be usable without any change in Fedora as well as in any Fedora-derivated distribution?

* about the URL tag: why not simply use http://sources.venemo.net/? The URL tag is intended to point to the project website, no matter how small it is. If it's not the solution you prefer, you could create a basic page in your fedorapeople.org space (or wherever you can) containing the description of the package and links to the sources.

Comment 3 Timur Kristóf 2011-04-15 05:59:17 UTC
Hello Mohamed, thank you for reviewing! :)

(In reply to comment #2)
> * maybe your package should be renamed « gnome-shell-extension-fedora-logo » or
> « gnome-shell-fedora-logo
Because if I named it like that, people would think that the package is provided by upstream (like gnome-shell-extensions), which is not the case. And if you look at any of such Fedora packages, all of them begin with either the name "fedora" or a release name. (examples: fedora-icon-theme, lovelock-backgrounds, laughlin-kde-theme, etc.)
If you still think that the name needs to change, I will rename it.
> * the fedora-logos package provides a « system-logos » capacity, as well as the
> generic-logos package (and probably the redhat-logos in RHEL also). Why not
> setting system-logos as Requires instead of fedora-logos, so that your package
> would be usable without any change in Fedora as well as in any Fedora-derivated
> distribution?
Very good point, I wasn't aware of such a possibility.
Currently the extension finds the logo by the icon name 'fedora-logo-icon'. If you tell me what icon name to use in order to utilize this system-logos capacity, I will gladly change it. :) 
> * about the URL tag: why not simply use http://sources.venemo.net/? The URL tag
> is intended to point to the project website, no matter how small it is. If it's
> not the solution you prefer, you could create a basic page in your
> fedorapeople.org space (or wherever you can) containing the description of the
> package and links to the sources.
I think making a fedorapeople.org page is a good idea, I'll make one.

Comment 4 Mohamed El Morabity 2011-04-27 00:58:38 UTC
Really really sorry for this late answer.

I may reconsider my views about system-logos, since there is in fact no generic equivalent of the fedora-logo file in the generic-logos package. So I tried to find an alternative.
The proper way to get the logo whatever the distribution may be to refer to start-here; this is by the way the icon grabbed by gnome-panel in GNOME 2.
The fedora-logos package contains start-here.* icon files, as symbolink links to fedora-icon.*, in /usr/share/icons/hicolor/, since any theme inherits from the hicolor theme. In Fedora<=14, the default theme used in GNOME was the "fedora" one, based on mist which doesn't provide any start-here icon, so start-here was grabbed in hicolor. generic-logos doesn't provide any start-here.* icon, so the "fallback" GNOME start-here.* icon was used instead (the foot logo).
In Fedora 15, it seems the default icon theme is "gnome", which provides its own start-here icon, that is to say the foot logo.
Some heuristics to grab a path to the distribution logo whatever the current icon theme may exist. It may be an evolution for this extension, one day ^^.

Anyway I'm OK to approve this package as it is currently if you rename it to gnome-shell-extension-fedora-logo, which seems to me finally the most appropriate name according to the guidelines.
Be *very* careful also with the GNOME Shell version set in metadata.json: you set the shell-version key to 3.0.0.2 whereas the latest version available of GS in the repos is 3.0.1. As a result, your extension won't work in GS > 3.0.0.2. A workaround would be to set shell-version to 3.0, which seems to work for any GS version 3.0.*.

Comment 5 Timur Kristóf 2011-05-03 20:59:08 UTC
Hello Mohamed,

I've made the modifications you requested.
I renamed the package and added an "Obsoletes: fedora-logo-gnome-shell-extension" line to the .spec so that the Shell doesn't crash for those people who have the package with the original name.

I also took your advice and put it onto fedorapeople.
The new .spec, .src.rpm, and tarball are here:
http://venemo.fedorapeople.org/sources/
The SCM is here:
git://fedorapeople.org/~venemo/gnome-shell-extension-fedora-logo.git

I hope it is okay now.

Comment 6 Mohamed El Morabity 2011-05-11 16:44:23 UTC
Sorry (again!) for this late answer.

The package is indeed OK. Just two minor points to fix:
- set the version-release in the first entry of the changelog according to the ones defined in the .spec (fixed string : « * Sun Apr 10 2011 Timur Kristóf <venemo> 1.0-2 »).
- the description is limited to 80 characters per line, not less. You have space enough to set it in two lines ^^.

Comment 7 Timur Kristóf 2011-05-16 19:47:08 UTC
Both of the "minor points" have been fixed.
The new files can be found at http://venemo.fedorapeople.org/sources/ (as usual)

Comment 8 Mohamed El Morabity 2011-05-17 06:39:58 UTC
Here is the review.

MUST: rpmlint must be run on every package.
->OK

MUST: The package must be named according to the Package Naming Guidelines.
->OK

MUST: The spec file name must match the base package %{name}, in the format
      %{name}.spec unless your package has an exemption.
->OK

MUST: The package must meet the Packaging Guidelines.
->OK

MUST: The package must be licensed with a Fedora approved license and meet the
      Licensing Guidelines.
->OK

MUST: The License field in the package spec file must match the actual license.
->OK

MUST: If (and only if) the source package includes the text of the license(s) in
      its own file, then that file, containing the text of the license(s) for
      the package must be included in %doc.
->OK

MUST: The spec file must be written in American English.
->OK

MUST: The spec file for the package MUST be legible.
->OK

MUST: The sources used to build the package must match the upstream source, as
      provided in the spec URL.
->OK

MUST: The package MUST successfully compile and build into binary rpms on at
      least one primary architecture.
->OK

MUST: If the package does not successfully compile, build or work on an
      architecture, then those architectures should be listed in the spec in
      ExcludeArch.
->N/A

MUST: All build dependencies must be listed in BuildRequires, except for any
      that are listed in the exceptions section of the Packaging Guidelines ;
      inclusion of those as BuildRequires is optional.
->OK

MUST: The spec file MUST handle locales properly. This is done by using the
      %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.
->N/A

MUST: Every binary RPM package (or subpackage) which stores shared library files
      (not just symlinks) in any of the dynamic linker's default paths, must
      call ldconfig in %post and %postun.  ->N/A

MUST: Packages must NOT bundle copies of system libraries.
->OK

MUST: If the package is designed to be relocatable, the packager must state this
      fact in the request for review, along with the rationalization for
      relocation of that specific package. Without this, use of Prefix: /usr is
      considered a blocker.
->N/A

MUST: A package must own all directories that it creates. If it does not create
      a directory that it uses, then it should require a package which does
      create that directory.
->NOK => /usr/share/gnome-shell/extensions/fedoralogo/ is not
         owned by the package

MUST: A Fedora package must not list a file more than once in the spec file's
      %files listings. 
->OK

MUST: Permissions on files must be set properly. Executables should be set with
      executable permissions, for example. Every %files section must include a
      %defattr(...) line.
->OK

MUST: Each package must consistently use macros.
->OK

MUST: The package must contain code, or permissable content.
->OK

MUST: Large documentation files must go in a -doc subpackage.
->N/A

MUST: If a package includes something as %doc, it must not affect the runtime of
      the application. To summarize: If it is in %doc, the program must run
      properly if it is not present.
->OK

MUST: Header files must be in a -devel package.
->N/A

MUST: Static libraries must be in a -static package.
->N/A

MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1),
      then library files that end in .so (without suffix) must go in a -devel
      package.
->N/A

MUST: In the vast majority of cases, devel packages must require the base
      package using a fully versioned dependency: Requires: %{name} =
      %{version}-%{release}.
->N/A

MUST: Packages must NOT contain any .la libtool archives, these must be removed
      in the spec if they are built.
->N/A

MUST: Packages containing GUI applications must include a %{name}.desktop file,
      and that file must be properly installed with desktop-file-install in the
      %install section.
->N/A

MUST: Packages must not own files or directories already owned by other
      packages.
->OK

MUST: All filenames in rpm packages must be valid UTF-8.
->OK

Just one issue, mentionned above:%{_datadir}/gnome-shell/extensions/fedoralogo/ is created by the packaged but not owned. To fix it, you can set your %files section as below:
    %files
    %defattr(-,root,root,-)
    %{_datadir}/gnome-shell/extensions/fedoralogo/
    %doc LICENSE
    %doc README

Comment 9 Mohamed El Morabity 2011-09-08 22:59:45 UTC
Ping?

Comment 10 Mohamed El Morabity 2011-10-26 13:27:40 UTC
There is unfortunately no comment on this review for more than a month. Without any action within a week, the review will be closed.
   http://fedoraproject.org/wiki/Package_maintainer_policy#Reviewer_not_responding

Comment 11 Timur Kristóf 2011-11-02 11:57:30 UTC
Sorry Mohamed, I haven't had the time to take care of the stuff yet. I promise to look into it this week.

Comment 12 Miroslav Suchý 2012-12-17 19:41:40 UTC
Ping? Any progress here? Or we can close this review?

Comment 13 Timur Kristóf 2012-12-24 11:53:05 UTC
I think there are other extensions by now which can fulfill the same purpose. So I'm closing this as WONTFIX.


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