Bug 239701

Summary: [PATCH] RFE: Enable userlist by default in KDM
Product: [Fedora] Fedora Reporter: Kevin Kofler <kevin>
Component: kde-settingsAssignee: Rex Dieter <rdieter>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora, rdieter, than
Target Milestone: ---Keywords: FutureFeature, Patch
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Enhancement
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-05-15 19:21:45 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
Patch to enable the userlist in KDM
none
Screenshot of the result
none
Shutdown dialog with WhiteOnBlack
none
Color scheme for FedoraFlyingHigh KDM theme
none
FedoraFlyingHigh.kcsrc screenshot
none
slightly modified WhiteOnBlack.kcsrc
none
KDM patch to honor alternateBackground setting from color scheme
none
KDM with FedoraFlyingHigh.kcsrc and patched kdebase
none
Color scheme for FedoraFlyingHigh KDM theme - variant with 0A0A0A buttons and scrollbars
none
KDM patch to honor alternateBackground setting from color scheme (rev2)
none
Patch to enable the userlist in KDM (FedoraFlyingHigh color scheme)
none
KDM with FedoraFlyingHigh2.kcsrc and patched kdebase (second patch)
none
FedoraFlyingHigh2.kcsrc with fedora colors and patched kdebase (second patch)
none
Color scheme for FedoraFlyingHigh KDM theme (rev3) none

Description Kevin Kofler 2007-05-10 16:02:49 UTC
Description of problem:
The KDM userlist (face browser) feature is now supported by the 
FedoraFlyingHigh theme in the latest redhat-artwork-kde in f7-final. Can this 
please be enabled by default? I have attached a patch doing this.

Version-Release number of selected component (if applicable):
kdebase-3.5.6-6.fc7

Additional info:
* Once the kde-settings package is created, that will be the proper place for 
this change.
* My patch also sets the KDM widget color scheme because a white userlist in 
FedoraFlyingHigh's black frame doesn't look great.

Comment 1 Kevin Kofler 2007-05-10 16:02:49 UTC
Created attachment 154480 [details]
Patch to enable the userlist in KDM

Comment 2 Kevin Kofler 2007-05-10 16:06:10 UTC
Created attachment 154481 [details]
Screenshot of the result

This is a screenshot of the result taken by Sebastian Vahl.

(Note that the overlap between the labels and textboxes is due to the low
resolution, this is NOT caused by my patches.)

Comment 3 Sebastian Vahl 2007-05-10 16:15:16 UTC
(In reply to comment #2)
> Created an attachment (id=154481) [edit]
> Screenshot of the result
> 
> This is a screenshot of the result taken by Sebastian Vahl.
> 
> (Note that the overlap between the labels and textboxes is due to the low
> resolution, this is NOT caused by my patches.)

FYI: Screenshot is taken from a vmware. Resolution there is 640x480.



Comment 4 Kevin Kofler 2007-05-10 16:16:40 UTC
Kuickshow says it's actually 800×600.

Comment 5 Sebastian Vahl 2007-05-10 21:44:31 UTC
Created attachment 154501 [details]
Shutdown dialog with WhiteOnBlack

Uhm. The test was too quick. We really should modify WhiteOnBlack.kcsrc.
Otherwise the menus in kdm also too dark.

Comment 6 Kevin Kofler 2007-05-10 21:51:18 UTC
And I should clarify: with "modify WhiteOnBlack.kcsrc", we actually mean: make 
a copy of WhiteOnBlack.kcsrc called e.g. FedoraFlyingHigh.kcsrc, and then 
change ColorScheme=WhiteOnBlack to ColorScheme=FedoraFlyingHigh. The original 
WhiteOnBlack scheme should not be modified, obviously.

What I wonder, however, is how much better we can get than this, because 
several widgets share the same palette colors. We definitely need the userlist 
background to be black (or, even better, #0a0a0a) for the userlist not to look 
like crap.

Comment 7 Kevin Kofler 2007-05-10 22:17:33 UTC
Created attachment 154503 [details]
Color scheme for FedoraFlyingHigh KDM theme

So how's this one? I have:
1. started from the Plastik color scheme
2. did the minimum color changes to get lists displayed with the proper colors
(note that windowBackground is actually the "Standard background", not the
"Window background", the bad name must have stuck for backwards compatibility):

activeForeground=255,255,255
alternateBackground=10,10,10
windowBackground=10,10,10
3. replaced the 4 Plastik colors which were close to, but not quite the same as
the Fedora Blue with the real Fedora Blue (#3C6EB4, dark: #294172):
activeBackground=41,65,114
activeBlend=60,110,180
linkColor=41,65,114
selectBackground=60,110,180

This should go into /usr/share/apps/kdisplay/color-schemes/, and
ColorScheme=WhiteOnBlack changed to ColorScheme=FedoraFlyingHigh in kdmrc as
alrady discussed. I hope this looks acceptable now, it's the best I can do.

Comment 8 Sebastian Vahl 2007-05-10 22:43:28 UTC
Created attachment 154504 [details]
FedoraFlyingHigh.kcsrc screenshot

This is your color-scheme. It looks ok. But another problem is, that the half
of the usernames are not readadble. I've created 6 users to see this. This also
happens with WhiteOnBlack.

Comment 9 Sebastian Vahl 2007-05-10 22:45:35 UTC
Created attachment 154505 [details]
slightly modified WhiteOnBlack.kcsrc

This one is a slightly modified WhiteOnBlack. I've only changed
background=66,101,171. This is a color I've randomly token from the wallpaper.
The problem with the unreadable userlist is also present here.

Comment 10 Kevin Kofler 2007-05-10 22:47:25 UTC
Grrr, that looks like KDM is hardcoding the alternate list color instead of 
using the one from the color scheme. Patching that out should be a one-line fix 
to the code. I'm on it.

Comment 11 Kevin Kofler 2007-05-10 23:36:13 UTC
Created attachment 154506 [details]
KDM patch to honor alternateBackground setting from color scheme

This is the promised code patch. It's actually 4 code lines + 3 #include
<qcolor.h> lines, not the one-liner I expected, because I have to work around a
design limitation of KApplication::createApplicationPalette: that function is
used for special apps like KDM to create a QPalette out of a KDE color scheme,
however QPalette doesn't contain KDE-specific settings, and alternateBackground
is KDE-specific. Thus, that information is lost. So, instead, I have to read it
directly from the color scheme and save it separately. (If it's missing, it
will default to QColor() which means no alternate background at all, which is a
safe default.)

This is freshly-written, so I haven't run any sorts of tests (not even checked
if it compiles) on it yet.

Comment 12 Sebastian Vahl 2007-05-11 11:03:55 UTC
Created attachment 154528 [details]
KDM with FedoraFlyingHigh.kcsrc and patched kdebase

Yeah, this is much better. But has this patch any maybe negative effects to
normal color schemes?

Comment 13 Kevin Kofler 2007-05-11 13:12:15 UTC
On other explicitly-set color schemes, no, it is the right thing to do for 
those too!

It does change the behavior on the system-wide default color scheme (i.e. if 
you don't set ColorScheme= at all), it disables the alternateBackground instead 
of just using the system-wide default in that case, I'm about to fix that.

Comment 14 Kevin Kofler 2007-05-11 13:24:20 UTC
Created attachment 154539 [details]
Color scheme for FedoraFlyingHigh KDM theme - variant with 0A0A0A buttons and scrollbars

Hmmm, in that screenshot, the scrollbar could use a recoloring too... But if I
do that, that also recolors at least the buttons.

Here's another variant of FedoraFlyingHigh.kcsrc with buttonBackground and
buttonForeground changed. I don't know if this turns out better or worse than
the other variant.

Comment 15 Kevin Kofler 2007-05-11 13:32:31 UTC
Created attachment 154540 [details]
KDM patch to honor alternateBackground setting from color scheme (rev2)

And here's the fixed version of the code patch. Now if ColorScheme is not set,
the alternateBackground is just left alone as it's supposed to. This one might
even be good enough for upstream.

Comment 16 Kevin Kofler 2007-05-11 13:37:54 UTC
Created attachment 154541 [details]
Patch to enable the userlist in KDM (FedoraFlyingHigh color scheme)

And here's an updated version of the patch which started this thread, where I
changed ColorScheme (to FedoraFlyingHigh) and the associated comment.

Comment 17 Kevin Kofler 2007-05-11 13:42:30 UTC
An additional color scheme change worth investigating would be to change 
background to 60,110,180 (the Fedora Blue). Your blue background looked nice. 
If 60,110,180 doesn't look good, then we can try your 66,101,171, but I'd 
rather match the official Fedora colors in a FedoraFlyingHigh color scheme. ;-)

Comment 18 Sebastian Vahl 2007-05-11 15:32:50 UTC
Created attachment 154549 [details]
KDM with FedoraFlyingHigh2.kcsrc and patched kdebase (second patch)

Mhh. This one is FedoraFlyingHigh2.kcsrc with the new patched kdebase. It looks
like the old patch (haven't uploaded a screenshot for this). But I'm not sure
if I haven't mixed up something.
Sadly I'm not at home until sunday. So I cannot do any further tests.

Comment 19 Sebastian Vahl 2007-05-11 15:42:05 UTC
Created attachment 154550 [details]
FedoraFlyingHigh2.kcsrc with fedora colors and patched kdebase (second patch)

And this one is with background=60,110,180 but the same issues like in comment
#18

Comment 20 Kevin Kofler 2007-05-11 16:19:30 UTC
> It looks like the old patch

The new code patch isn't any different from the old one if you set a 
ColorScheme, it only fixes the case where no ColorScheme is set (that case now 
works the same as without the patch, which is how it should be, the patch is 
only supposed to apply to the case where a ColorScheme is set explicitly).

The difference between FedoraFlyingHigh.kcsrc and FedoraFlyingHigh2.kcsrc is 
that the scrollbar is no longer out of place (gray on black).

I really like that screenshot (attachment #154550 [details]) from comment #19 (I think 
that's the way it should look), I'm going to update FedoraFlyingHigh.kcsrc 
accordingly.

Comment 21 Kevin Kofler 2007-05-11 16:26:51 UTC
Created attachment 154554 [details]
Color scheme for FedoraFlyingHigh KDM theme (rev3)

So this one should be the good one. This is the version screenshotted in
comment #19: same as FedoraFlyingHigh2.kcsrc, but with background=60,110,180.

Comment 22 Sebastian Vahl 2007-05-11 19:05:51 UTC
(In reply to comment #20)
> > It looks like the old patch
> 
> The new code patch isn't any different from the old one if you set a 
> ColorScheme, it only fixes the case where no ColorScheme is setxplicitly).

Ah. Understood.

(In reply to comment #21)
> So this one should be the good one. This is the version screenshotted in
> comment #19: same as FedoraFlyingHigh2.kcsrc, but with background=6

I agree with that. But maybe we should show this one to some people with some 
knowledge and experience in usuability things. Just to be sure we aren't 
missing something.

Comment 23 Kevin Kofler 2007-05-11 19:45:03 UTC
I think we should just push it (everything: the code patch, the config patch 
and the color scheme, plus a default face as discussed in bug #239694) to 
Rawhide ASAP. If people don't like it, they'll complain anyway. And don't 
forget that:
* Unless it gets pushed back again, the deep freeze is 6 days from now. I 
estimate that it will take at least one day to update the specfile, build it, 
wait for the build to complete, then get it pushed to f7-final. We can wait a 
few hours, but not days or weeks.
* I can't think of anything left to improve in the color scheme (at least not 
without making the colors mismatch somewhere else), so I think it's no use 
waiting.
* It's only used during login, so I don't see usability as a big concern. 
People used to be happy with "login:" as a login prompt. ;-)

Comment 24 Than Ngo 2007-05-12 09:05:57 UTC
Kevin,Sebastian, it now looks great. I will build it this weekend so it should 
be included in FC7 release. Guys, it's great work. Thanks

Comment 25 Kevin Kofler 2007-05-14 22:34:08 UTC
Ping? The weekend is over, there's still nothing in CVS, and we only have 2 
days left!

Comment 26 Than Ngo 2007-05-15 12:53:06 UTC
Kevin, the FC7 tree was locked, so i cannot build it. I have asked the rel-eng 
to allow building it in FC7 tree, but it's still not happen now. It's still 
locked.

Comment 27 Rex Dieter 2007-05-15 13:24:08 UTC
Than, building isn't locked (I've been building stuff all the past week), just
tagging items for f7-final needs rel-eng approval.

Comment 28 Kevin Kofler 2007-05-15 13:25:55 UTC
Uh, what tree is locked where?

Core and Extras have merged now, so you should commit your work to 
kdebase/devel in the external CVS (the one which used to be "extras", it will 
work if you use the cvs.fedoraproject.org:/cvs/extras directory, though the 
canonical name is now cvs.fedoraproject.org:/cvs/pkgs), not in the internal Red 
Hat CVS. The external CVS is not locked for commits as far as I know, other 
packagers have been able to commit there.

Once you committed the work (which hasn't happened as far as I can see from the 
CVS web interface), you should then run "make build" in the kdebase/devel 
directory, this will have the package built in Koji (which is the replacement 
for Brew). The package will then be tagged "dist-fc7".

And then you should e-mail rel-eng and ask them to tag the 
package with "f7-final", that way it will go into the release. But you should 
get this done today or tomorrow, as the freeze happens some time on Thursday.

Please see the discussions at the fedora-maintainers mailing lists if you're 
confused by the new processes:
https://www.redhat.com/archives/fedora-maintainers/2007-May/thread.html

Comment 29 Rex Dieter 2007-05-15 14:08:01 UTC
hrm, the FedoraFlyingHigh color scheme really should be in redhat-artwork(-kde).

Comment 30 Rex Dieter 2007-05-15 14:09:38 UTC
re-assigning -> kde-settings

Comment 31 Kevin Kofler 2007-05-15 14:17:28 UTC
> hrm, the FedoraFlyingHigh color scheme really should be in
> redhat-artwork(-kde).

If you can get the folks who are maintaining redhat-artwork (and now -kde too) 
to include it, sure. ;-)

Comment 32 Kevin Kofler 2007-05-15 14:52:59 UTC
What's still missing in current CVS:
* the color scheme 
(/usr/share/apps/kdisplay/color-schemes/FedoraFlyingHigh.kcsrc). If this should 
be in redhat-artwork-kde as Rex says, we need to bug the redhat-artwork 
maintainers to include it, and quickly (time is running out).
* a default face: please symlink:
/usr/share/apps/kdm/pics/users/default1.png
(or default2 or default3) into:
/usr/share/apps/kdm/faces/.default.face.icon
as discussed in bug #239694. That one should definitely be done in kdebase.

Comment 33 Than Ngo 2007-05-15 15:26:19 UTC
yes, FedoraFlyingHigh color scheme should be included in redhat-artwork-kde.
i have already built redhat-artwork-kde-7.0.0-8.fc7 which includes the missing 
FedoraFlyingHigh.kcsrc.

the kdebase-3.5.6-7.fc7 is still building.

Comment 34 Kevin Kofler 2007-05-15 15:29:47 UTC
I forgot/missed that you are actually in the redhat-artwork ACL, that sure made 
things easier. :-)

What's now in CVS should be complete, thanks. Now all that's needed is waiting 
for the builds to complete and then e-mailing the f7-final tagging requests for 
the redhat-artwork and kdebase updates to rel-eng.

Comment 35 Than Ngo 2007-05-15 15:35:20 UTC
Keven, Sebastian. thanks for good work. when the both packages are done, i 
will send request to rel-eng.

Rex. why have you reassigned it to kde-settings? it's a problem in kdebase.

Comment 36 Kevin Kofler 2007-05-15 15:38:08 UTC
Rex, the kde-settings.spec you just imported still defaults to FedoraDNA. 
You'll have to change this to FedoraFlyingHigh, and then merge the 
configuration part of the changes from this thread into kde-settings.

Please don't rush kde-settings in without checking that no configuration change 
from the latest kde-redhat-config-3.5-fc7-1.tar.bz2 got lost.

Comment 37 Rex Dieter 2007-05-15 15:40:18 UTC
> Rex. why have you reassigned it to kde-settings? it's a problem in kdebase.

Than, the plan (all along) was to split all configs out of kdelibs/kdebase and
put them into kde-settings. (almost done now).

Kevin, yes, I'm working to integrate that all now, but pass one certainly won't
be perfect, and will require tweaking.

Comment 38 Kevin Kofler 2007-05-15 15:45:41 UTC
You _do_ know that the deep freeze, with only fixes for blocker bugs allowed 
in, is coming really soon now (2 days from now is the latest date I know), 
right? ;-)

Comment 39 Than Ngo 2007-05-15 15:50:27 UTC
Rex, it's too late to split all configs out of kdelibs/kdebase und put them 
into kde-settings. Please do it for F8 but not for F7.

Comment 40 Rex Dieter 2007-05-15 16:00:13 UTC
Bah, let's give it a shot.  It'll be easy enough to back out if this turns out
to be problematic.

Just commited kde-settings-3.5-21, 

* Tue May 15 2007 Rex Dieter <rdieter[AT]fedoraproject.org> 3.5-21
- kiosk-style configs (finally)
- kdm use UserLists, FedoraFlyingHigh color scheme (#239701)

issued build 
http://koji.fedoraproject.org/koji/taskinfo?taskID=8447

Comment 41 Kevin Kofler 2007-05-15 17:03:00 UTC
redhat-artwork-7.0.0-8.fc7 and kdebase-3.5.6-7.fc7 built, now please send the 
rel-eng request if you didn't already.

Comment 42 Sebastian Vahl 2007-05-15 19:24:38 UTC
FYI: I've grabbed redhat-artwork-7.0.0-8.fc7, redhat-artwork-kde-7.0.0-8.fc7 and
kdebase-3.5.6-7.fc7 from koji and spinned a new livecd with them. All is working
fine here.