Bug 441637 - Review Request: fedorawaves-kdm-theme
Review Request: fedorawaves-kdm-theme
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity low
: ---
: ---
Assigned To: Kevin Kofler
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-09 05:02 EDT by Ngo Than
Modified: 2008-04-10 12:12 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-04-10 12:12:28 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
Screenshot with different users in 1280x1024 (523.54 KB, image/png)
2008-04-09 06:03 EDT, Sebastian Vahl
no flags Details
Screenshot with ColorScheme=ObsidianCoast in 1280x1024 (524.93 KB, image/png)
2008-04-09 07:48 EDT, Sebastian Vahl
no flags Details

  None (edit)
Description Ngo Than 2008-04-09 05:02:41 EDT
Spec URL: http://than.fedorapeople.org/wave-kdm-theme/wave-kdm-theme.spec
SRPM URL: 
http://than.fedorapeople.org/wave-kdm-theme/wave-kdm-theme-1.0.fc9.src.rpm
Description: 
This package contains the Fedora Wave KDM theme that is the default
theme in Fedora 9.
Comment 2 Sebastian Vahl 2008-04-09 06:03:52 EDT
Created attachment 301768 [details]
Screenshot with different users in  1280x1024

1. Shouldn't it be "waves" instead of "wave"?
http://fedoraproject.org/wiki/Artwork/F9Themes/Waves

2. For infinity and flyinghigh we've used the prefix "fedora":
fedoraflyinghigh-kdm-theme and fedorainfinity-kdm-theme

So I would suggest "fedorawaves-kdm-theme" as name.

3. Please have a look at the screenshot. It'll need some work:
 - hostname is bigger than the box
 - font color needs some changes for not selected users
Comment 3 Kevin Kofler 2008-04-09 07:11:51 EDT
Sebastian, what color scheme are you using? The author says you should use 
ObsidianBlue.

For the "hostname is bigger than the box" problem, we should just make the box 
wider, there's plenty of room.

I'll do the review when I get back home.

The name should be waves-kdm-theme or fedorawaves-kdm-theme. I'm not sure 
about the "fedora" part because the theme doesn't actually contain a Fedora 
logo, but on the other hand it makes it clear where the theme is coming from.
Comment 4 Sebastian Vahl 2008-04-09 07:30:58 EDT
(In reply to comment #3)
> Sebastian, what color scheme are you using? The author says you should use 
> ObsidianBlue.

Oxygen (the default in current kdmrc). Where do I get ObsidianBlue? 
kdebase-workspace only contains ObsidianCoast ATM.

> The name should be waves-kdm-theme or fedorawaves-kdm-theme. I'm not sure 
> about the "fedora" part because the theme doesn't actually contain a Fedora 
> logo, but on the other hand it makes it clear where the theme is coming 
from.

AFAIR also fedorainfinity-kdm-theme and fedoraflyinghigh-kdm-theme didn't 
contain a Fedora logo. So I would prefer fedorawaves-kdm-theme as %name 
(and /usr/share/kde4/apps/kdm/themes/FedoraWaves as directory).

Comment 5 Ngo Than 2008-04-09 07:35:19 EDT
1. Shouldn't it be "waves" instead of "wave"?
>http://fedoraproject.org/wiki/Artwork/F9Themes/Waves

it's typo, it should be waves.

>2. For infinity and flyinghigh we've used the prefix "fedora":
>fedoraflyinghigh-kdm-theme and fedorainfinity-kdm-theme
i will rename it to fedorawaves-kdm-theme.

>3. Please have a look at the screenshot. It'll need some work:
> - hostname is bigger than the box

it's knowned issue. I already talked with Pavel, he will fix it properly today 
evening. Temporary i will make the box wider.

> - font color needs some changes for not selected users
ObsidianCoast works fine for me.
Comment 6 Kevin Kofler 2008-04-09 07:41:19 EDT
ObsidianCoast must be what I meant, that was from memory and I just didn't 
remember it correctly.
Comment 7 Kevin Kofler 2008-04-09 07:43:31 EDT
I'm changing the title for the fixed name, please rename the specfile.
Comment 8 Sebastian Vahl 2008-04-09 07:48:12 EDT
Created attachment 301783 [details]
Screenshot with ColorScheme=ObsidianCoast in 1280x1024

(In reply to comment #6)
> ObsidianCoast must be what I meant, that was from memory and I just didn't 
> remember it correctly.

Ok. :)
ObsidianCoast looks better then.
Comment 9 Sebastian Vahl 2008-04-09 08:05:35 EDT
BTW: Where does the wallpaper come from? AFAIR the final wallpaper for Waves 
isn't finished yet: 
https://www.redhat.com/archives/fedora-art-list/2008-April/msg00149.html

IMHO it should match the default wallpaper we'll use after a login.
Comment 10 Ngo Than 2008-04-09 09:07:53 EDT
it's fixed now. could you please review it again? thanks

it's not the final wallpaper. We will update package if there's final 
wallpaper done.
Comment 12 Kevin Kofler 2008-04-09 11:19:18 EDT
rpmlint has one complaint:
fedorawaves-kdm-theme.noarch: W: no-documentation
Shouldn't there be a COPYING file included?
Comment 13 Ngo Than 2008-04-09 11:22:57 EDT
we should ignore this, it could be done later.
Comment 14 Kevin Kofler 2008-04-09 11:46:13 EDT
MUST Items:
+ rpmlint output:
fedorawaves-kdm-theme.noarch: W: no-documentation
harmless, ignoring
+ named and versioned according to the Package Naming Guidelines
+ spec file name matches base package name
+ Packaging Guidelines:
  + License GPL+ OK, matches actual license
  + No known patent problems
  + No emulator, no firmware, no binary-only or prebuilt components
  + Complies with the FHS
  + proper changelog, tags, BuildRoot, BuildRequires (none needed), Summary, 
Description
  ! Requires: kde-settings-kdm should be Requires: kdebase-kdm
  + no non-UTF-8 characters
  + no documentation relevant to include
  + nothing to compile, so RPM_OPT_FLAGS are irrelevant
  + nothing to compile (noarch package), so no debuginfo package
  + no static libraries nor .la files
  + no duplicated system libraries
  + no binaries => no rpaths
  + no configuration files, so %config guideline doesn't apply
  + no init scripts, so init script guideline doesn't apply
  + no GUI executables, so no .desktop file needed
  + ... and thus no desktop-file-install needed either (KdmGreeterTheme.desktop 
is not really a .desktop file, it's a theme description)
  + no timestamp-clobbering file commands (cp -p is being used)
  + nothing to build, so _smp_mflags are irrelevant
  + scriptlets are valid
  + not a web application, so web application guideline doesn't apply
  + no conflicts
+ complies with all the legal guidelines
+ no license in the tarball to include as %doc
+ no upstream, skipping source matches upstream check
+ builds on at least one arch (F8 i386)
+ no known non-working arches, so no ExcludeArch needed
+ no BuildRequires needed
+ no translations, so translation/locale guidelines don't apply
+ no shared libraries, so no ldconfig call needed
+ package not relocatable
+ ownership correct (owns package-specific directories, doesn't own directories 
owned by another package)
+ no duplicate files in %files
+ permissions correct, defattr used correctly
+ %clean section present and correct
+ macros used where possible
+ no non-code content
+ no large documentation files, so no -doc package needed
+ no %doc files, so no %doc files required at runtime
+ no header files which would need to be in a -devel subpackage
+ no static libraries, so no -static package needed
+ no .pc files, so no Requires: pkgconfig needed
+ no .so symlinks vs. plugins
+ no -devel package, so "-devel should require %{name} = %{version}-%{release}" 
is irrelevant
+ no .la files
+ no GUI executables, so no .desktop file needed
+ buildroot is deleted at the beginning of %install
+ all filenames are valid UTF-8

SHOULD Items:
! no license file in the tarball
+ no translations for description and summary provided by upstream
* skipping mock test (nothing to build, no BRs needed)
* skipping all arch test (it's noarch anyway)
* skipping functionality test (already tested, see previous comments)
+ scriptlets are sane
+ no subpackages other than -devel, so "Usually, subpackages other than devel 
should require the base package using a fully versioned dependency." is 
irrelevant
+ no .pc files, so "placement of .pc files" is irrelevant
+ no file dependencies

APPROVED, but please change Requires: kde-settings-kdm to Requires: kdebase-kdm 
after import.
Comment 15 Ngo Than 2008-04-09 13:03:01 EDT
it's fixed, could you please review it again? thanks
Comment 16 Kevin Kofler 2008-04-09 13:07:18 EDT
OK, that resolves the only real issue I found. Just to confirm:
APPROVED
Comment 17 Kevin Kofler 2008-04-09 13:14:29 EDT
New Package CVS Request
=======================
Package Name: fedorawaves-kdm-theme
Short Description: Fedora Waves KDM Theme
Owners: than,rdieter,kkofler,ltinkl
Branches: 
InitialCC: 
Cvsextras Commits: yes

For the branches, we need devel only, F-9 can wait for the mass branching.
Comment 18 Kevin Fenzi 2008-04-09 22:51:38 EDT
cvs done.
Comment 19 Ngo Than 2008-04-10 12:12:28 EDT
built in rawhide

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