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.
http://than.fedorapeople.org/wave-kdm-theme/wave-kdm-theme-1.0-1.fc9.src.rpm
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
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.
(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).
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.
ObsidianCoast must be what I meant, that was from memory and I just didn't remember it correctly.
I'm changing the title for the fixed name, please rename the specfile.
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.
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.
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.
Spec URL: http://than.fedorapeople.org/wave-kdm-theme/fedorawaves-kdm-theme.spec SRPM URL: http://than.fedorapeople.org/wave-kdm-theme/fedorawaves-kdm-theme-1.0-1.f9.src.rpm
rpmlint has one complaint: fedorawaves-kdm-theme.noarch: W: no-documentation Shouldn't there be a COPYING file included?
we should ignore this, it could be done later.
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.
it's fixed, could you please review it again? thanks
OK, that resolves the only real issue I found. Just to confirm: APPROVED
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.
cvs done.
built in rawhide