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

List:       koffice-devel
Subject:    Re: QGraphicsWidget based canvases
From:       Boudewijn Rempt <boud () valdyas ! org>
Date:       2010-07-26 17:20:05
Message-ID: 201007261920.05985.boud () valdyas ! org
[Download RAW message or body]

On Monday 26 July 2010, Marijn Kruisselbrink wrote:
> On Monday 26 July 2010 18:06:52 Boudewijn Rempt wrote:
> > Hi,
> > 
> > It's a bit too big (340k) and complicated for reviewboard, so I'm
> > proposing this for review at koffice-devel. At
> > http://www.valdyas.org/~boud/graphicsview.tgz there is a set of git
> > patches (apply to svn with patch -p1) that Marijn and me have developed
> > last week
> 
> > that do the following thing:
> Here my comments on the patches after giving it a more careful look :)
> 
> 0002:
> > * There are currently zero or one QGraphicsItem-based canvases per
> > document; this might need to change in the future.
> 
> Actually I'm not sure what the usecase is of having this somewhat arbitrary
> limitation of one QGraphicsItem that is created by the KoDocument, but then
> not owned by it... Wouldn't it make more sense to just get rid of
> canvasItem() and make createCanvasItem() public?

Yes, that's possible. I originally did it this way to keep some paralellism to 
the pattern of creating KoView from KoDocument; I think I already mentioned 
that we might need more than one canvas item per document. The one thing that 
bothers me is that we really want to be sure there are no canvas items left 
once the document is deleted, and I'm not sure how to do that properly right 
now. (Originally, the document owned the canvas item and would delete it, but 
that clashed with the way QGraphicsView works).

> 
> 0003:
> It would seem that there is quite some code in KoCanvasControllerWidget
> that could have been in KoCanvasController itself just as easily. For
> example ensureVisible() doesn't use anything widget specific at all, and
> there probably are more methods where it would be quite usefull for future
> KoCanvasController implementations to not have to duplicate code that most
> likely won't be different anyway.
> -- okay, I see you did some of that in 0031 already, so maybe that is
> alright then.

Yes, right. I originally kept KoCanvasController as abstract as possible, only 
moving methods to the base class later when it was clear I could share them 
between different actual implementations. I'm sure there's more we can share, 
though.

> 0011:
> I might not understand completely what this does, but a non-virtual method
> that is hardcoded (and inline) to always return false seems rather useless.
> Shouldn't this be a virtual method with the old implementation in
> KoCanvasControllerWidget overriding it?

Yes, that's a mistake, it should be virtual. It's overriden in kis_canvas2, 
which is the only koffice canvas that can switch between qwidget and 
qglwidgtet.git I've amended the patch (but not uploaded yet).

> 0032/0033:
> I'm not quite sure I see why there is even a need for two seperate
> setDocumentSize methods? I probably overlooked something in one of the
> earlier patches, but why was this needed? It is at least very confusing to
> have two identical named methods that are rather different.

setDocumentSize(QSize) was added because the d-pointer of KoCanvasController 
stores the document size. How about renaming setDocumentSize(QSize, bool) to 
updateDocumentSize, since it clearly is more than a mere setter?

> 0038:
> Not sure why this is in this patchset, but why are you installing headers
> that are marked as internal? Namely KoFilterVertex.h and
> PriorityQueue_p.h.

The libs/main filter api is a mess... Basically, without these, any code that 
uses KoFilter outside KOffice doesn't compile. I didn't want to spend much 
time on it since Cyrille intends to rewrite the filter api anyway.

> 0040:
> again seems to not belong in this patchset, and installs a rather arbitrary
> set of headers it seems? Also having some order in the list would have been
> nice 

Maybe we can go through the set of installed headers tomorrow and see what we 
should install and what we should keep as patches for the koffice deb for 
meego/

> 0041:
> I think it would be better to have a virtual setCursor method in
> KoCanvasBase, rather than have getters for all possible implementations of
> KoCanvasBase. Now the canvasItem() and canvasWidget() method are more a
> sort of ugly way to implement your own rtti.

True, though canvasItem and canvasWidget are also used in other places. I 
don't like it either :-(.

> 
> 0047:
> I wrote this one, and as is clear, it isn't quite finished/clean yet. Now
> I've just added extra methods to KoToolProxy, but I think it should be no
> problem to remove the non-KoPointerEvent methods of the ones that now have
> a KoPointerEvent overload. The only thing the other methods do is that
> they wrap the two parameters they get in a KoPointerEvent, and then use
> that further.
> 
> 0048:
> No, the fixed by us both was 0047, not this one, this patch was fine :)
> 
> Patch 49 and 50 seem missing?

I think they only installed headers the mobile office uses.

________________________
> koffice-devel mailing list
> koffice-devel@kde.org
> https://mail.kde.org/mailman/listinfo/koffice-devel


-- 
Boudewijn Rempt | http://www.valdyas.org
_______________________________________________
koffice-devel mailing list
koffice-devel@kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel
[prev in list] [next in list] [prev in thread] [next in thread]