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

[Alexander Wiedenbruch]

Am Freitag, 2. März 2007 schrieb 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 =)

Hmm, I should stay at app delevopment and keep my hands out of libs :/
Most of these 'problems' are results of the moving the class from SK, like the 
last two items. Thanks anyway ;)

>
> > > 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".
>
Thanks for pointing this out!

> > > >  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" :)

Well it's gone now and that is probably a good thing...