|Summary:||[PATCH] RFE: Enable userlist by default in KDM|
|Product:||[Fedora] Fedora||Reporter:||Kevin Kofler <kevin>|
|Component:||kde-settings||Assignee:||Rex Dieter <rdieter>|
|Status:||CLOSED RAWHIDE||QA Contact:||Fedora Extras Quality Assurance <extras-qa>|
|Version:||rawhide||CC:||fedora, rdieter, than|
|Target Milestone:||---||Keywords:||FutureFeature, Patch|
|Fixed In Version:||Doc Type:||Enhancement|
|Doc Text:||Story Points:||---|
|Last Closed:||2007-05-15 19:21:45 UTC||Type:||---|
|oVirt Team:||---||RHEL 7.3 requirements from Atomic Host:|
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)  > 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 Ngo Than 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 Ngo Than 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 firstname.lastname@example.org 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 Ngo Than 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 Ngo Than 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 Ngo Than 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.