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

List:       koffice-devel
Subject:    Re: pagestyles branch
From:       Pierre <pinaraf () pinaraf ! info>
Date:       2008-08-15 21:31:19
Message-ID: 200808152331.20510.pinaraf () pinaraf ! info
[Download RAW message or body]

On Friday 15 August 2008 10:16:54 Thomas Zander wrote:
> Looking at the diff that was integrated; some things that slipped my mind
> in the first set;
>
> * KWDocument::frameSetAdded / Removed signals have been removed, why?
> I think they should stay.
Sebastian removed them because they were unused.
I won't answer for him, but I think removing unused things can be good (even 
if I don't really like doing this in the middle of other changes)
> * In the KWDLoader most of the changes are commenting out of code without
> any comments on how to fix this.  Will the new method of loading headers be
> restored in that loader, please?
This will be done.
> * the KWPage::OrientationHint seems to have been commented out, why?
My bad, I forgot to remove it after commenting it and fixing the code, sorry.
(The layout isn't stored in KWPage anymore)

> * KWPageSettings (aka the master page data object)
> is very clearly a data object. It holds no real controller code and has
> lots of data fields.
Agree
> yet it doesn't have a copy constructor and its even a QObject.
> I think it would be very helpful to make this class a QSharedData
> inheriting object and get rid of the signal by just making the view force
> the relayout. This would solve the problem if it being a QObject and it
> would also solve the now rather odd passing of the document pointer to the
> pageManager. This doc pointer to the pagemanger I think is something we
> should try to avoid. The pattern I set up is that KWDocument is the front
> end for common things and that it defers to the page manager for less-common
> API. You can see this by the fact that the document has an (non-pointer)
> instance of the pageManager.
>
> As an added bonus, this makes unit testing the pagemanager a LOT easier.
Can't agree more, I'll look at this.

> * KWPageManager::addPageSettings seems to leak an instance of
> KWPageSettings if the name has been added before.
It should never happen, and I don't really see a way to properly deal with 
this.
For instance, if one call addPageSettings with the following code :
KWPageSettings *settings = new KWPageSettings("MyStyle");
pageManager->addPageSettings(settings);
pageManager->appendPage(settings);

I think your point is fully valid, but the only "reliable" fix I see would be 
to have the KWPageSettings constructor being private and its creation managed 
by the KWPageManager.

> * Can we rename KWPageSettings to something that looks more like
> MasterPageData ?
Sure, that's what I told you monday, renaming it after the merge.
But I'd rather go for KWPageStyle, to mimic KoParagraphStyle, KoListStyle...
So, should we do it the democratic way ?
I vote for "KWPageStyle" :)


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