[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:       Benoit Minisini <gambas () users ! sourceforge ! net>
Date:       2008-07-28 17:32:39
Message-ID: 200807281932.39812.gambas () users ! sourceforge ! net
[Download RAW message or body]

On lundi 28 juillet 2008, Aaron J. Seigo wrote:
> 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.

Sorry, all this patch was not cleaned up. It was just to show what I have 
changed. Of course, I won't commit anything that is not just a bug fix and 
that you won't agree as you wrote above.

>
> it looks like you did a lot more than just fix transparency. you also
> changed from using smoothScale to just scale, 

Because it was faster and visually changed almost nothing in the drawing of 
the antialiased clock. 

> changed how the background  
> colours are being set (i can't test so can't see if that breaks anything),

Because of the transparency fixes: if a widget is "transparent", it unsets its 
palette to get the background from its parent, then set its palette 
foreground color, not the contrary. If a widget is opaque, it will set both 
its background and foreground color. I just centralized the background 
settings in a "setBackground()" method.

> 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).

As one should never draw outside of a paint event, I replaced the immediate 
repaint() by update(), apparently with no problem. 

Note I have not finished working on the clock applet. I will check both 
repaint() and update() to see if it creates any flicker or not.

>
> 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.

Sorry for that, I didn't sent a clean patch. Of course, if I commit something, 
I will follow the same indentation mode and code convention used by the file 
I modified.

>
> > - 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.

These features are already in the SimpleButton class code. Here is the 
constructor of SimpleButton in the current 3.5 branch:

////
SimpleButton::SimpleButton(QWidget *parent, const char *name)
    : QButton(parent, name),
      m_highlight(false),
      m_orientation(Qt::Horizontal)
{
    setBackgroundOrigin( AncestorOrigin );

    connect( kapp, SIGNAL( settingsChanged( int ) ),
       SLOT( slotSettingsChanged( int ) ) );
    connect( kapp, SIGNAL( iconChanged( int ) ),
       SLOT( slotIconChanged( int ) ) );

    kapp->addKipcEventMask( KIPC::SettingsChanged );
    kapp->addKipcEventMask( KIPC::IconChanged );

    slotSettingsChanged( KApplication::SETTINGS_MOUSE );
}
////

>
> 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).

No, I try to test as many things as possible: different panel locations, 
sizes, I change colour schemes, I switch between transparency, plain 
background, themed background... I didn't change the icon theme yet.

Note that by doing all these tests, I found other bugs: I have fixed the 
behaviour of drag & drop when a panel has scroll buttons for example.

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

Of course I will.

> > - 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. =/ 

Sorry again for the indentation. It will be fixed in the commits.

> 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.

I don't think there would be a speed issue, because there is less shadowed 
text drawn inside the minipager than inside the taskbar. 

>
> > - Buttons and applets are freshed during their move too. It is nicer. :-)
>
> you also added a filter feature to the browser button.
>

I won't commit it, as it is a feature change.

> 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.
> =(
>

Yes, you already talked about that in a previous mail. It was just there 
because I didn't clean up the patch, and of course I won't commit it too.

> 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.

I didn't want to scare you :-). I sent a patch that was not cleaned up because 
I just wanted to show my changes quickly.

I fully understand that this is a end-of-life stable version. I don't think 
the situation is as horrific as you described, and I agree with all your 
points, except:

* The shadowed text in the minipager, that makes the desktop names readable on 
a transparent background.

* The remove of the unneeded two pixels area in the applet handle: these two 
pixels were just there for drawing a toolbar primitive element. I think there 
is a bug report about that in the bugs database, but I don't remember the 
number now.

I will try to commit fixes the more incrementally as possible.

Here are the other fixes and controversial changes I worked on since this 
patch:

- If you select a background image for the panel, and if you ask this 
background image to be colorized, then the one pixel border of the panel is 
drawn with the same color, not with the panel background color. This is 
logical, visually cleaner, and should close some request in the bug databse.

- Panels with scroll buttons now should work correctly. 
ContainerArea::contentsX() and ContainerArea::contentsY() were not taken into 
account during drag & drop for example.

- I took the antialiased code from the clock applet, and made the eyes applet 
antialiased the same way. This applet is not horrible anymore. :-)

Note that I will maintain all the code I will commit - I think I am a 
responsible guy about that. You can ask the gambas users. :-)

Regards,

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

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