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

List:       koffice-devel
Subject:    Re: Page variable support for kopageapp
From:       Thorsten Zachmann <t.zachmann () zagge ! de>
Date:       2009-09-17 19:41:30
Message-ID: 200909172141.30989.t.zachmann () zagge ! de
[Download RAW message or body]

On Thu September 17 2009, Thomas Zander wrote:

> > > I think this is a mild annoyance, at best.
> >
> > It makes developing the app a painfull.
> 
> I think you are missing that we are talking about libraries that we ask 3rd
> party developers to develop on. Other KDE people, companies, friends.
> If you have to spent an extra hour on writing some little loops and that
> avoids API clutter than that makes it easier for a lot of people to write
> Flake plugins. Don't you think thats worth it?
> 
> > > > - The application needed to set the KoTextPage onto the
> > > > KoTextShapeData. In kword this is implemented on inserting of a
> > > > textshape. This is not the best way as you need special code to
> > > > handle inserts of textshapes into the application.
> > >
> > > The application having *the same* extra code doesn't make it so bad. It
> > > would be nicer if it could be avoided, granted.
> >
> > And that the patch avoids.
> 
> Sure, but at a big cost to everyone but kpresenter. Thats not really fair,
> is it?

there is only a very small payload for all the other apps.

> > > This is the core concept of flake, no shape can look different by only
> > >  adding it to different shapeManagers.
> > > I would go out on a limb and say thats a concept thats universal.
> > > It is wrong to say your suggestion is not possible. It is possible, and
> > > I showed you how. You use a 'proxy' shape.
> > > If you want example code, let me know.
> >
> > Who says that. I have a different point of view on this one. These are
> > all the same shapes and no proxy is needed. Makes live much easier. No
> > extra code at all.
> 
> Its a universal design concept. Model view shows this too.
> If I put a string in a model the exact same string will be shown in a tree
> or in a list view. Maybe the way its shown is slightly different (different
> font, only the first 20 characters) but the actual content is always the
> same.
> So if you have a different point of view, then I'm afraid you have the
>  burden of proof here as you disagree with the core concept of MVC. And one
>  thing you will have to prove is that its not expensive to do what you
>  want. You say "no extra code at all". Which is not true. Its only no extra
>  code for KPresenter. But instead you add extra code for each and every
>  shape that you want to have showing different. And the amount of plugins
>  is infinite.

Even the proxy shape you introduced for kword uses the same model and sets a 
different page for a short period so that is not different to what is done by 
this patch.

> > > * The 'update' method is made virtual. This is a severe performance
> > > impact for all shapes and future plugins. This alone is a show-stopper
> > > as shapes typically call update a lot with small rects (text layout
> > > calls it once for every line)
> >
> > I think you are exaggerating a bit here. That is the right place for it
> > it is what inheritance is for to do something different on some shapes.
> > And it is quite fat.
> 
> I suggest you google for "cost of virtual methods". It essentially turns a
> simple short call into a long call and 2 memory lookups from the vtable.
> It also avoids any inlining.
> You may ask a senior C++ dev why we don't just make each and every method
> virtual to get a nice answer about the cost.

Nobody wants to make every function virtual. It is one function and that is 
only a small overhead if you can measure it at all
> So, no, I'm not exaggerating.
> And I have to again ask why you want to make each and every shape that is
> ever going to be written to pay a penalty just because you don't want to
>  add some cheap loops to kpresenter? Honestly...
> 
> > > * the m_paintRegion is a dirty hack that I don't think should be in the
> > >  text shape.
> >
> > What is dirty in that?
> 
> Selecting what part to repaint in one method and obeying it in a completely
> separate other method. With no connection between them two? Yes, thats
> dirty.
> Much much more dirty than setting a variable on a text shape in a little
> loop in kopageapp on switching slides.

It is not selected what needs to be repainted. The only thing that is done is 
to not forward updates when layouting is done during the paint event and the 
updated part will already be repainted. So actually this does speed up thinks 
when a KoPageProvider is used.

> > > * re-layouting *all* text shapes, *every* paint event is wrong. If you
> > > use the scrollwheel you'd re-layout the text 40 times for a small area.
> >
> > That is not done at all. The relayout only happens if page number of the
> > text shape has actually changed.
> 
> Hmm, can you show where the code is that avoids a relayout when the page is
> the same?
> As I read the code the shape is made dirty and re-layouted at every paint
> event. Irregardless of the shape actually having a variable or not. So it
> also re-layouts shapes that have no need to be re-layouted.

Have a look at line 339. If the page number stays the same no relayout is 
done. So most of the time there is no relayout as the page is actually the 
same as the last time.

> > > And last;
> > > * It invents a different way, and introduces a lot of new API, of doing
> > > the thing that is already possible using prior existing public API.
> >
> > The current API works perfectly for kword but as I explained before ir
> > totally fails for how kpresenter works.
> 
> Have you tried? I did and it works. Maybe time to see if your view of flake
> may be wrong?
> 

I tried to explain why I don't like this way. The application needs special 
code for this kind of shape.

I have read your objections but don't agree on them. I will go on and commit 
the patch to fix the bug.

Have a nice day, 

Thorsten

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