[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-devel
Subject:    Re: Kicker patches (was Patches and enhancements for KDE 3.5)
From:       "Aaron J. Seigo" <aseigo () kde ! org>
Date:       2008-07-28 16:19:16
Message-ID: 200807281019.16318.aseigo () kde ! org
[Download RAW message or body]

[Attachment #2 (multipart/signed)]


On Thursday 24 July 2008, Benoit Minisini wrote:
> Hi,
>
> Here is the diff file of my patches.
>
> I finally fixed the transparency in the following places:
> - Panels, when they have scroll buttons.
> - Buttons.
> - Clock applet.

it looks like you did a lot more than just fix transparency. you also changed 
from using smoothScale to just scale, changed how the background colours are 
being set (i can't test so can't see if that breaks anything), changed all 
repaint calls to update calls (which again, i can't test, but i'd be surprised 
if this didn't introduce unwanted flicker in certain configurations).

i think the formatting of the code is highly unfortunate as well. you've gone 
in and taken what was consistently formatted code and made it highly 
inconsistent: tabs instead of spaces, no use of {}, etc. i put a lot of effort 
into making that code as readable as possible, it would be nice to see that 
maintained.

> - Weather applet.
> - System tray applet.
> - Taskbar applet.
> - Launcher applet.

in this patch, you removed the connections to settings changed to update the 
icon or cursor. this seems to be have been in the course of changing over to 
SimpleButton usage, however SimpleButton doesn't seem to have gained these 
features.

i fear that you are making changes that "seem to work" but you aren't actually 
testing them thoroughly (e.g. changing different panel locations, sizes, colour 
schemes, icon themes, etc).

there are also huge amounts of code that are just commented out. please clean 
up your patches.

> - Virtual desktop applet.

> These are the applets I use. If everything is correct with them, I will
> look at the other applets, those I don't use.
>
> I fixed too:
> - The applet handles.

yes, you removed the 2 pixels that were there to ensure the background lined 
up in all situations when transparent. =/

> - The "add applet" dialog.
> - The launcher applet layout routine.
> - The arrow buttons, that follow the KDE widget style now.
> - The launcher and weather applet buttons.
> - The way the virtual desktop applet draws the desktop names in
> transparency mode.

+        if (transparent)
+        {
+         ·      bp.setPen(on ? colorGroup().midlight() : 
colorGroup().buttonText());
+         ·      drawShadowText(bp, QRect(0, 0, w, h), AlignCenter, label, 
size());
+        }
+        else
         bp.drawText(0, 0, w, h, AlignCenter, label);

erg. so now you there's a dangling line after the else that isn't even 
indented, though at least the if follows the coding style, well except for the 
use of tabs. =/ anyways, shadowed text wasn't used there due, iirc, primarily 
speed issues. there may have been other issues, it's been so long since i 
touched that code and i don't remember exactly now; but there was shadowed 
text there for a while, and then it was removed.

> - Buttons and applets are freshed during their move too. It is nicer. :-)

you also added a filter feature to the browser button.

you also broke BackgroundHide mode for panels:

-                KWin::raiseWindow(winId());
+                //KWin::raiseWindow(winId());

i mean, you didn't even remove the if condition, you just commented out the 
one line of code inside of the if condition and in the process completely 
changed how these panels work. this is completely unacceptable, because not 
only is it just *wrong* it's also doing this in a supposdly stable branch. =(

these patches are more than just little fixes, they are huge changes. many of 
them probably unecessary, some are feature additions, and some are just plain 
buggy or outright wrong. i have no way to test them here without setting up a 
kde3 devel environment again, and i do not have time to devote to working on 
kde3. given the nature of the patches, i'm just going to have to step away 
from this whole experiment as i can't in good consciense pretend to sign off on 
these changes. i think that the patches, as they stand, are not in the best 
interests of those who rely and depend on 3.5.x being stable and reliable.

as you continue, i'd ask that you please, please, please:

* respect the coding style in code you change; at least don't make it even 
uglier than it already is

* don't change things so carelessly (esp things like panel hiding!)

* test a whole lot more than you evidently are

* realize that this is a *stable branch* you are working in, so be a lot more 
conservative in what you do

i'm happy to see that you are fixing some bugs along the way. that part is 
great.

i don't know .. perhaps you should be working in a branch outside of the 3.x 
mainline with some of these feature changes, etc. i just don't want to see 
kicker in 3.x go downhill. i'd rather see the minor annoyances in it remain 
than have new bugs and misfeatures introduced.

-- 
Aaron J. Seigo
humru othro a kohnu se
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43

KDE core developer sponsored by Trolltech


["signature.asc" (application/pgp-signature)]

>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<


[prev in list] [next in list] [prev in thread] [next in thread]