Re: koffice/libs/flake

On Friday 16. October 2009 09.12.49 Thomas Zander wrote:

> On Friday 16. October 2009 07.24.59 Thorsten Zachmann wrote:
> > SVN commit 1035881 by zachmann:
> >
> > o Fix copy and paste to make sure the zIndex of the inserted shapes are
> >   unique lin the parent.
>
> why did you remove the
>   maxZIndex = qMax(s->zIndex(), maxZIndex);
> code ?
> That was there for shapes that don't have a parent.

oh, skip that question, I missed the
   d->canvas->shapeManager()->topLevelShapes()
So your solution looks complete.

But this means that on paste you now iterate over *all* shapes in the shape
manager. As you know, outside of KoPageApp this means the whole
document, so this is very very slow for long documents.

What is the advantage over the old method of only looking at the place
where we paste? There is no visual difference at all.

I don't see what you are trying to solve here, and its a terribly expensive
solution,
--
Thomas Zander


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

(restoring koffice-devel 'to', keep this archived please)

On Friday 16. October 2009 10.44.05 you wrote:
> On Fri October 16 2009, you wrote:
> > But this means that on paste you now iterate over *all* shapes in the
> > shape manager. As you know, outside of KoPageApp this means the whole
> > document, so this is very very slow for long documents.
>
> That is only true for kword. Al other apps use layers. However that is
> surely something that can be improved.

Any suggestions?
Its a speed regression introduced just before the RC, maybe a bad time ;)

> > What is the advantage over the old method of only looking at the place
> > where we paste? There is no visual difference at all.
> >
> > I don't see what you are trying to solve here, and its a terribly
> > expensive solution,
>
> It makes sure the z-index is unique which is a must as stated before to
> make painting work reliable (not paint the shape sometimes above or below a
> different shape with the same z-index). Not ones above and other wise below
> the shapes. If you have a not so expensive solution please let me know.

The painting order was always correct before; the code ensured that there was
no problems in painting order by having different z-indexes already.

Can you give a usecase of the problem you were trying to solve?
Maybe a document that shows the bug?

--
Thomas Zander
_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel

[Thorsten Zachmann]

On Fri October 16 2009, Thomas Zander wrote:

> (restoring koffice-devel 'to', keep this archived please)
>
> On Friday 16. October 2009 10.44.05 you wrote:
> > On Fri October 16 2009, you wrote:
> > > But this means that on paste you now iterate over *all* shapes in the
> > > shape manager. As you know, outside of KoPageApp this means the whole
> > > document, so this is very very slow for long documents.
> >
> > That is only true for kword. Al other apps use layers. However that is
> > surely something that can be improved.
>
> Any suggestions?
> Its a speed regression introduced just before the RC, maybe a bad time ;)

I think a fixed painting bug is much more important then this small slowdown
when a shape is copied.

> > > What is the advantage over the old method of only looking at the place
> > > where we paste? There is no visual difference at all.
> > >
> > > I don't see what you are trying to solve here, and its a terribly
> > > expensive solution,
> >
> > It makes sure the z-index is unique which is a must as stated before to
> > make painting work reliable (not paint the shape sometimes above or below
> > a different shape with the same z-index). Not ones above and other wise
> > below the shapes. If you have a not so expensive solution please let me
> > know.
>
> The painting order was always correct before; the code ensured that there
>  was no problems in painting order by having different z-indexes already.

no it was not. Before when you pasted shapes it can happen that the same z-
index is created if the pasted shapes are do not overlap. If you the move the
shapes around to the shape with the same z-index sometimes one shape is above
and sometimes it is below

>
> Can you give a usecase of the problem you were trying to solve?
> Maybe a document that shows the bug?
>

You can reproduce:

- add 2 shapes above each other
- copy the shapes
- move the existing shapes so that they are out of the way of the soon to be
pasted shapes
- paste the shapes
- move one of the pasted shapes over the ones you moved

select different overlapping shapes and you will see that the paint order
changes randomly.

Thorsten

See also:

http://lists.kde.org/?l=koffice-devel&m=125482842332755&w=2
_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel

On Friday 16. October 2009 11.24.38 Thorsten Zachmann wrote:
> On Fri October 16 2009, Thomas Zander wrote:

> > Any suggestions?
> > Its a speed regression introduced just before the RC, maybe a bad time ;)
>
> I think a fixed painting bug is much more important then this small
> slowdown when a shape is copied.

How do you figure its a small slowdown?
Jan just changed the zindex to be 16 bits because he has good reasons to
believe 14 bits is not enough.
Thats a *lot* of shapes to iterate over on each paste and not something I'd
call 'small'. Don't you agree?
In general I think its bad that we consciously add code that we know doesn't
scale when the document gets larger.

> > The painting order was always correct before; the code ensured that there
> >  was no problems in painting order by having different z-indexes already.
>
> no it was not. Before when you pasted shapes it can happen that the same z-
> index is created if the pasted shapes are do not overlap. If you the move
> the shapes around to the shape with the same z-index sometimes one shape
is
> above and sometimes it is below

That sounds rather far-fetched, to be honest.
If a user moves a shape manually and notices its not in the right stacking
order he always (before and after your patch) will make the shape be above or
below the shape he targets.
So if this is your usecase, this is the bug you fix, I object to the fix. It
doesn't scale and its solving a rarely-hit usecase that is introduced by a
user action that in normal cases as well as these corner cases have the same
solution; the user changes the z-order.

What do others think?
--
Thomas Zander


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

[Thorsten Zachmann]

On Fri October 16 2009, Thomas Zander wrote:

> On Friday 16. October 2009 11.24.38 Thorsten Zachmann wrote:
> > On Fri October 16 2009, Thomas Zander wrote:
> > > Any suggestions?
> > > Its a speed regression introduced just before the RC, maybe a bad time
> > > ;)
> >
> > I think a fixed painting bug is much more important then this small
> > slowdown when a shape is copied.
>
> How do you figure its a small slowdown?
> Jan just changed the zindex to be 16 bits because he has good reasons to
> believe 14 bits is not enough.
> Thats a *lot* of shapes to iterate over on each paste and not something I'd
> call 'small'. Don't you agree?
> In general I think its bad that we consciously add code that we know
>  doesn't scale when the document gets larger.

I changed the algorithm used to be o(n) instead of o(n log n). Also I tested
how expensive it is to do this for 1000000 shapes and it it takes less then
0.02 s on my system which I would think is fast enough.

Thorsten
_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel

[Thorsten Zachmann]

On Fri October 16 2009, Thorsten Zachmann wrote:

> On Fri October 16 2009, Thomas Zander wrote:
> > On Friday 16. October 2009 11.24.38 Thorsten Zachmann wrote:
> > > On Fri October 16 2009, Thomas Zander wrote:
> > > > Any suggestions?
> > > > Its a speed regression introduced just before the RC, maybe a bad
> > > > time ;)
> > >
> > > I think a fixed painting bug is much more important then this small
> > > slowdown when a shape is copied.
> >
> > How do you figure its a small slowdown?
> > Jan just changed the zindex to be 16 bits because he has good reasons to
> > believe 14 bits is not enough.
> > Thats a *lot* of shapes to iterate over on each paste and not something
> > I'd call 'small'. Don't you agree?
> > In general I think its bad that we consciously add code that we know
> >  doesn't scale when the document gets larger.
>
> I changed the algorithm used to be o(n) instead of o(n log n). Also I
>  tested how expensive it is to do this for 1000000 shapes and it it takes
>  less then 0.02 s on my system which I would think is fast enough.

If -O2  is used it even gets in the region of 0.002 seconds for doing so. So
it is fast enough.

Thorsten
_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel