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

List:       koffice-devel
Subject:    Re: ODF text (was Release schedule after the alpha's)
From:       Sebastian Sauer <mail () dipe ! org>
Date:       2007-12-20 1:00:14
Message-ID: 200712200200.14857.mail () dipe ! org
[Download RAW message or body]

Thomas Zander wrote:

> On Wednesday 19. December 2007 02:14:20 Sebastian Sauer wrote:
>> > I have also, as you can see, formulated a set of criteria I see as
>> > showstoppers for a final release. I would also like to consider the
>> > milestones I've placed for each of the three betas showstoppers.
>>
>> I am strickly against marketing KWord as "beta" atm since I strikly deny
>> the imgo right logical; "can be saved and loaded without loss of
>> information" for KWord.
> 
> I have to admit; I tried getting into the odf loading a couple of times,
> but failed.
> There are just too many classes and too many cross dependencies and no
> overview doc I could find.

yeah, I just have to agree there. The code is really not that easy to 
understand :-/ I'll try to fix it within next days by adding more 
documentation. Probably we could also sit down 1-2 hours and try to refactor 
it a bit to decrease the confusion aka level of time needed to get into that 
code.

> I think having unit tests for loading is an absolute must, but every time
> I try I'm lost in the amount of classes that have to be instantiated to be
> able to actually call something like KoTextLoader::loadList

y, I also agree there. Well, an excuse may that we did follow the 
XP-development but did left out the chapter that deals with unittests ;)
 
> I'd like to suggest two things
> 
> 1) refactor the context objects to have no arguments to their constructors
> unless they are required for the context objects constructor to run.
> Add setFoo() methods on those classes.

sounds for me more like a cosmetic change which could be for sure done rather 
fast but may not provide a the best solution :-/
 
> 2) explain or refactor this confusing construct;
> Constructor of KoTextLoadingContext;
>    KoTextLoadingContext (KoTextLoader *loader, KoDocument *doc,
>    KoOdfStylesReader &stylesReader, KoStore *store)
> notice how you pass a loader there.

Without looking at the code I've to clue there by myself (/me ducks :)

All I am able to say is that those things that did got passed to the ctor are 
needed to process loading. So, you may also note that most of the code is 
within one huge class. That#s for 2 reasons;
1. I had much trouble by myself to get into the 1.6.x codebase cause 
loading-code was splitted all over at different places. So, rather then 
uintroducing some fancy e.g. operator-pattern or something like this, I just 
followed the basic rule of try to prevent any code that is not direct needed 
+ try to keep all the code related to loading as close together as possible.
2. I still guess it's wise to go that way and try to don't hurt the codebase 
even more with a to complex design.
3. parts of the code (like the context) got just copied from 1.6 and then 
adapted. The reason for this was to try to keep as close to the prev codebase 
as possible to at least be able to reuse parts of the code+ideas we have 
there somehow - though I agree that this may not be valid anymore.

> And then almost all methods on KoTextLoader you get have to pass in an
> instance of KoTextLoadingContext. The one you created *with* the loader as
> a reference.

Nah, and that's where I did look at the code :) and...
seems it's not used at all. Probably it was used at a time before and later 
got changed. Anyway, another clean sign that I should probably try to get 
that codebase in a better state asap.

> That just doesn't make sense to me...
> Or, more accurately, I have no idea how to use this code by just looking
> at the API and docs.

ok, within next days I'll try to fix that points, thx. 
 
> Bottom line; I want to have a way to test this.  In my test i need a way
> to instantiate the correct classes.
> Define an XML that I then let KoXml load, as my testing code.
> Then I want to call specific methods in the KoTextLoader and check if the
> QTextDocument looks correct afterwards.
> 
> Who can help me with that? I already added an empty test framework to be
> filled ;)

please just commit it, so I can discover probably other points that are not 
logical / missing :)
_______________________________________________
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]