Resource (paint server) information is not included in KCanvas render tree dumps
for example:
KCanvasItem {rect} at (274,109) size 101x141 [stroke={server=UNIMPLEMENTED}] [fill=
{server=UNIMPLEMENTED}] [data="M275.00,110.00L375.00,110.00L375.00,250.00L275.00,250.00"]
This will require being able to do a reverse lookup of canvas resources from their pointer value, as well as
adding dumping of resources to the top of the dump file. The two changes are not necessarily tied.
Created attachment 3531[details]
Patch to dump KCanvasRegistry info on KRenderingPaintServers
This patch makes KCanvasTreeDebug dump more info about KRenderingPaintServer
instances found from KCanvasRegistry. KCanvasResources are not dumped because
there is not currently type() method for them, and base class doesn't have any
info to be dumped.
paint server pointer -> string id in registry mapping is not implemented,
rather registry that owns the paint server will set the id for the server. If
reverse mapping is really wanted instead of this, this needs to be changed.
If the current approach of saving QString id for each server/resource is ok,
then perhaps it'd be good to have KCanvasRegistryItem base class, which would
implement setIdInRegistry() and idInRegistry(). Each class that is to be stored
in registry would then implement that interface..
Looking for comments..
The patch looks fine.
Gradient stop lists should be trivial to implement, lets land that at the same time.
It also bothers me a little that the << operators for paint servers need to do so much static casting... I
guess there are advantages to having this all in one file, but part of me feels like it would have been easier
to have made the << operator virtual on KCanvasPaintServer and implement it for each server... doing the
same for resources would make it possible to print them w/o a type() call.
Overalll, great work!
Created attachment 3550[details]
Alternative way of dumping paint servers and canvas resources
This is alternative approach for dumping the canvas registry.
Every class that can somehow be navigated to from canvas registry has virtual
method externalRepresentation. By overloading it, you can dump the properties
of the objects in the tree.
In addition to that, every base class or type has related QTextStream
&operator<<(QTextStream &, const Class &) -operator, which is actually used in
dumping code.
Which is the preferred approach, this or the previous? This patch is not final,
as it lacks filter effect dumping and updated layout tests.
the result is something like following snipet. However, filter effect dumping
is not implemented fully, so effects are only partial.
--
KCanvasRegistry
KCanvasResource {id=MyFilter[type=FILTER] at (0,0) size 200x120
[filterBoundingBoxMode=0] [effectBoundingBoxMode=0] [effects=[[in=SourceAlpha]
[result=blur] [subRegion=at (0,0) size 0x0], [in=blur] [result=offsetBlur]
[subRegion=at (0,0) size 0x0], [in=SourceGraphic] [result=] [subRegion=at (0,0)
size 0x0]]]}
KCanvasContainer at (0,0) size 479x359
KCanvasContainer at (70,110) size 320x178 [stroke={server=0}]
[fill={server=}]
KCanvasContainer at (70,110) size 320x178
[transform={m=((1.500000,0.000000)(0.000000,1.500000))
t=(80.000000,110.000000)}]
KCanvasItem {rect} at (80,110) size 298x178 [stroke={server=}]
[fill={server=}] [data="M1.00,1.00L199.00,1.00L199.00,119.00L1.00,119.00"]
KCanvasContainer at (70,147) size 320x105 [stroke={server=0}]
[fill={server=}] [filter=MyFilter]
KCanvasContainer at (70,147) size 320x105 [stroke={server=0}]
[fill={server=}]
KCanvasItem {path} at (70,147) size 320x105 [stroke={server=, stroke
width=10.000000}] [fill={server=0}]
[data="M50.00,90.00C0.00,90.00,0.00,30.00,50.00,30.00L150.00,30.00C200.00,30.00,200.00,90.00,150.00,90.00"]
KCanvasItem {path} at (124,169) size 212x61 [stroke={server=0}]
[fill={server=}]
[data="M60.00,80.00C30.00,80.00,30.00,40.00,60.00,40.00L140.00,40.00C170.00,40.00,170.00,80.00,140.00,80.00"]
KCanvasContainer at (0,0) size 0x0 [stroke={server=}]
[fill={server=}]
KCanvasItem {rect} at (0,0) size 479x359 [stroke={server=}] [fill={server=0}]
[data="M1.00,1.00L479.00,1.00L479.00,359.00L1.00,359.00"]
The second patch looks OK. I'm not sure why you have both the externalRepresentation function *and*
the << operator... why not just a virtual << operator on every subclass? In the case of sub-classes where
you already have a type() operator, you could go so far as to make << non-virtual, and then do your own
static_cast... but that sounds a bit like over-kill.
(In reply to comment #5)
> The second patch looks OK. I'm not sure why you have both the
externalRepresentation function *and*
> the << operator... why not just a virtual << operator on every subclass? In
the case of sub-classes where
> you already have a type() operator, you could go so far as to make <<
non-virtual, and then do your own
> static_cast... but that sounds a bit like over-kill.
To my understanding stream operators in a class affect the class they're
defined in, not the stream. So you need to have your instance of the class in
the left hand side, ie "resource << stream", which doesn't make sense. "stream
<< resource" affects the stream, and the operator cannot be defined in the
class of resource -object.
In other words, "stream << resource" calls Stream::operator<<(const Resource&)
or operator<<(Stream&, const Resource&), but to my understanding you cannot
make it call Resource::operator<<(Stream&), because it means different thing.
Created attachment 3583[details]
Dump KRenderingPaintServers and KCanvasResources and their properties
The patch dump KRenderingPaintServers and KCanvasResources and their properties
when using externalRepresentation()
Created attachment 3584[details]
Layout-test diffs for the previous attachment
SVGSupport/layout-test diffs that are result of the previous patch (3583)
Looks good. We discussed a few more issues over IRC:
1. there were several "server=," that seems wrong. Just get rid of "server=" all together.
2. "server=0" should just be "none"
3. PaintServer identifiers should aways be in quotes, both when listed in the registry, and when
referenced . e.g. fill="grad"
4. While you're in there, can you add some setprecision(2) calls when printing things like width, etc?
5. It would be nice to special case some KCanvasMatricies, like Identity, and print as such. (the more
human readible the better).
6. Also, when the registry is empty, print "KCanvasRegistry: empty" or some other indicator.
Hum... furthermore, this doesn't look quite right:
+ KCanvasResource {id=arithmetic50[type=FILTER] at (0,0) size 100x100 [filterBoundingBoxMode=1]
[effectBoundingBoxMode=0] [effects=[[type=IMAGE] [in=] [result=blue] [subRegion=at (0,0) size 0x0],
[type=IMAGE] [in=] [result=red] [subRegion=at (0,0) size 0x0], [type=COMPOSITE] [in=red] [result=]
[subRegion=at (0,0) size 0x0] [in2=blue] [k1=0.000000 k2=0.000000 k3=0.000000 k4=0.000000]]]}
1. there should be a space between the id (in quotes!) and the type.
2. Why a location? the bounding box is valid, I think...
3. No need to print default values... decide which ones are default (bbox mode on? etc?) and don't print
those.
4. don't print empty values... like "in" or subregion...
5. result should be in quotes result="blue"
Each filter effect shoudl probably have it's own line. Print the filter as though it were a container, with
each effect indented and on the lines below.
Comment on attachment 3583[details]
Dump KRenderingPaintServers and KCanvasResources and their properties
This is a really great start, but we need a few more tweaks before we land.
See my copious comments in the bug.
Created attachment 3636[details]
Dump KRenderingPaintServers and KCanvasResources and their properties (2nd)
Patch which tries to address previous comments.
Added some checks to not dump default valued properties. However, I added those
checks only for the properties I knew the default value - it's hard to know it
because a) there's no initialization on construction b) there's no
documentation. Maybe somebody who knows better could add more..
Didn't add indentation, because it would need that the dumping context (ie. the
value of the indentation level) would need to be transferred to each dump
function. This would ofcourse be most naturally done with specific
DebugTreeStream, but I didn't want to make this overengineered / deviate from
webcore..
Adds QTextStream::precision() to KWQ/KWQTextStream.h/mm .. I don't know how
welcome this is..
Comment on attachment 3636[details]
Dump KRenderingPaintServers and KCanvasResources and their properties (2nd)
Still could use a couple more tweaks we discussed over IRC. Otherwise looks
good.
Created attachment 3638[details]
Dump KRenderingPaintServers and KCanvasResources and their properties (3rd)
Another try at the resource dumping. removed server=, simplified
QTextStream::precision()
(whitespace diffs are still there..)
I think the following two lines in KCanvasTreeDebug.cpp will need to change from
if (o.style()->isStroked() && DIFFERS_FROM_PARENT(style()->strokePainter()) && o.style()-
>strokePainter()) [line 178]
if (o.style()->isFilled() && DIFFERS_FROM_PARENT(style()->fillPainter()) && o.style()->fillPainter()) [line
180]
to
if (o.style()->isStroked() && DIFFERS_FROM_PARENT(style()->isStroked()) && DIFFERS_FROM_PARENT
(style()->strokePainter()) && o.style()->strokePainter())
if (o.style()->isFilled() && DIFFERS_FROM_PARENT(style()->isFilled()) && DIFFERS_FROM_PARENT(style()-
>fillPainter()) && o.style()->fillPainter())
This because, in the current implementation of KCanvasRenderingStyle, strokePainter() and fillPainter()
will create a new painter if none already exists. Calling o.parent->style()->strokePainter() (which is
what the macro will do) could therefor alter the state of the parent, an unwanted side effect.
(In reply to comment #18)
Come to think of it, my proposed change wont do. The logic will need to be something like:
if (o.style()->isStroked())
if (!o.parent->isStroked() || DIFFERS_FROM_PARENT(style()->strokePainter()))
....
Created attachment 3669[details]
DUMP KRenderingPaintServers and KCanvasResources and their properties (5th)
Added macro to test a predicate before using accessor member function which has
side-effects to parent.
Included marker dumping, they were accidentally printed as ptrs.
Comment on attachment 3669[details]
DUMP KRenderingPaintServers and KCanvasResources and their properties (5th)
We caught one error, on printing KCanvasPath, which I'd like to correct before
landing.
Created attachment 3695[details]
Dump KRenderingPaintServers and KCanvasResources and their properties (6th)
Fixed paths to print relevant attributes only (depending on path command).
Moved path related dumping operators to KPathData.cpp, which didn't exist. This
means that the patch touches WebCore/WebCore.xcodeproj/project.pbxproject also.
Also fixes one small output formating detail (clip path names were not inside
hyphens "")
Note, that in order to compile WebCore+SVG currently I needed to add
EventNames.cpp to be compiled in WebCore+SVG project. This change is also
included in the patch, as I didn't feel confident enough to just delete the +
line from the patch.
Comment on attachment 3695[details]
Dump KRenderingPaintServers and KCanvasResources and their properties (6th)
This looks fine, and all the tests pass on my machine. I would like to discuss
the license with you before landing, so I've posted an alternative patch. We
can discuss and land tonight.
2005-08-23 06:06 PDT, Kimmo Kinnunen
2005-08-24 07:47 PDT, Kimmo Kinnunen
2005-08-26 03:34 PDT, Kimmo Kinnunen
2005-08-26 03:38 PDT, Kimmo Kinnunen
2005-08-29 01:43 PDT, Kimmo Kinnunen
2005-08-29 01:44 PDT, Kimmo Kinnunen
2005-08-29 05:02 PDT, Kimmo Kinnunen
2005-08-29 05:30 PDT, Kimmo Kinnunen
2005-08-29 05:31 PDT, Kimmo Kinnunen
2005-08-30 00:42 PDT, Kimmo Kinnunen
2005-08-30 00:43 PDT, Kimmo Kinnunen
2005-08-31 08:52 PDT, Kimmo Kinnunen
2005-08-31 08:55 PDT, Kimmo Kinnunen
2005-08-31 15:50 PDT, Eric Seidel (no email)