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

List:       koffice-devel
Subject:    Re: QGraphicsWidget based canvases
From:       Marijn Kruisselbrink <m.kruisselbrink () student ! tue ! nl>
Date:       2010-07-26 16:54:38
Message-ID: 201007261854.38621.m.kruisselbrink () student ! tue ! nl
[Download RAW message or body]

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?

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.

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?

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.

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.

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

0041:
I think it would be betterto 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.

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?

Marijn
_______________________________________________
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]