[Panel-devel] KDE/kdebase/workspace/plasma/lib

[Zack Rusin]

On Friday 02 March 2007 15:09, Alexander Wiedenbruch wrote:
> Am Freitag, 2. März 2007 schrieb Aaron J. Seigo:
> > On March 1, 2007, Alexander Wiedenbruch wrote:
> > > SVN commit 638393 by wirr:
> > >
> > > Move the InputBox from SuperKaramba to Plasma.
> > >
> > >
> > >  M  +6 -0      CMakeLists.txt
> > >  M  +1 -1      datavisualization.h
> > >  A             widgets (directory)
> > >  A             widgets/lineedit.cpp   [License: GPL (v2+)]
> > >  A             widgets/lineedit.h   [License: GPL (v2+)]

First of all some general comments:
- please try to pass arguments as const references,
- constructors, besides the argument passing, are rather ugly :) note that you 
did reverse of what we do in Qt - meaning the parent argument isn't optional 
and comes before the actual properties of the class. Also if you provide a 
QPointF/QSizeF combo, QRectF would be way more obvious. Altough I question I 
question presence of those arguments in the constructor (more about that 
later).
- you don't seem to follow, well any, indention style :) and switch between 4 
and 2 space indention.
- the LineEdit::blinkCursor is either a bad name for a method or just a bad 
method in general, depending how you look at it (it's either 
toggleBlinkCursor or setBlinkCursor(bool enable)). I'd also suggest making a 
slot on the Private object which completely hides from the public interface.
- you shouldn't be making event handling methods 
(keyPressEvent/mousePressEvent) public :)
- setValue/getStringValue are inconsistent names for a pair of setter/getter 
combination. and given what those methods setText would be better =)

> > this won't work; libplasma is lgpl. so either this needs to move into
> > plasma itself, or we could just use QGraphicsTextItem, no?
>
> I can put it under LGPL as I am the original author.

Well, you copied parts of it from Qt so that will be difficult :) 

> In SK I had some problems with using different colors for the frame,
> selected text, ... in QGraphicsTextItem so I implemented it myself.
> I am not sure this necessary in Plasma anyway.

To be honest, you'd be just so much better off by using QGraphicsTextItem than 
anything else. All that I guess you'd want is inherit QGraphicsTextItem and 
overwrite the paint method in it and do something like:
void Plasma::LineEdit::paint(...)
{
    QStyleOptionFrameV2 panel;
    panel.initFrom(widget);
    panel.rect = boundingRect();
    style->drawPrimitive(QStyle::PE_PanelLineEdit, &panel, painter, widget);
    style->drawPrimitive(QStyle::PE_FrameLineEdit, &panel, painter, widget);
    QGraphicsTextITem::paint(p, option, widget);
}
to get the current style based qlineedit's frames around this "widget".

> > >  A             widgets/widget.cpp   [License: LGPL (v2)]
> > >  A             widgets/widget.h   [License: LGPL (v2)]
> >
> > i see this implements the (pure virtual) boundingRect(). i suppose the
> > question is whether or not makes any sense to set the size in the
> > constructor?
>
> boundingRect should actually be virtual in Widget, so it can be
> reimplemted. My thought was that any widget needs a size, so this is
> necessary for every widget so I put it in the ctor.

Ah, and here be dragons :) This is getting into "what is size" on a canvas 
debate, which all setSize methods will lose. Lets say we have this code:
QGraphicsScene *scene;
//.... setting up the scene and among others doing:
scene->setSceneRect(-1, -1, 2, 2);
Plasma::SomeWidget *w;
//setting up the widget
w->setSize(200, 30);
w->scale(0.01, 0.01);
What is the size of w on the above set scene? (2, 0.3) or (200, 30)? Do you 
want to rotate it now to have some more fun? Size is really only meaningful 
on singular items, where by singular items I mean those that can be 
decomposed into one rendering calls (e.g. drawRect). Composition tend to get 
very messy especially if text is involved. Or you can place arbitrary 
restrictions on "size", e.g. "The size method defines the size of the object 
in screen coordinates on identity transformation matrix. Any operations which 
diverge the screen coordinates from the graphicsview coordinates will cause 
this method to return values of the size multiplied by the combined 
transformation matrix of its parents". 
And don't get me wrong, I don't necessarily think that having a method setting 
a size on a Plasma::Widget (if existing) is a bad thing, just a lot harder 
than shown here. In any way, the default way of having size set on 
Plasma::Widget's (just like any other widgets) is having the widget do it 
itself based on its contents :)


> > finally, everything in libplasma should be in the Plasma namespace. i've
> > fixing Runner right now, actually.
> >
> > what else are you planning on implementing in Widget, if anything?
>
> Actually it was just an idea to have a fixed interface for all widgets.
> Perhaps some necessary functions that need to implemented in each widget,
> like setSize()?

As mentioned above, that's not the best idea :) And if anything following Qt's 
naming conventions it should be "resize" :)

z