[Ktechlab-devel] kde4 port: CircuitModel instantiation from tests

[Julian Bäume]

hi,

On Tuesday 23 March 2010 12:15:30 P Zoltan wrote:
>   I've looked more closely to the source code of kde4-port, and observed a
> strange thing: CircuitModel needs an instance of Kdevelop::Core, and
> internally has a pointer to a Circuit plugin object. In my opinion this
> can cause problems for test case writing, because then running tests
> supposes a working copy of ktechlab, and maybe more.
> 
>   Is that really necessary? In my opinion a circuit model shouldn't know
> about a kdevplatform plugin. Maybe the code can be reworked in order to
> have a relatively easy to use CircuitModel class.
The KTLCircuitPlugin knows about the electronic components it is able to 
handle. The Model doesn't know about this, so it has to get information from 
the CircuitPlugin (the filename of the SVG file, in this case). KDevPlatform is 
needed, because more components can be provided by new Plugins. KDevPlatform 
does all the plugin-loading magic, so I guess we need an instance here, or 
provide this functionality by ourselves, which I would like to avoid. However 
for testing, the test can create a KDevelop::Core instance without a GUI. This 
will create something that is able to handle projects and documents and load 
Plugins (which don't require a GUI). To compile KTechLab, you will need 
KDevPlatform installed, anyway.

I don't see an easy way to change this, without bringing back the "one class 
for each component" problem, which IMHO doesn't scale.
 
>   In very long term I'd like to have a "light weight" version ktechlab,
> with a more simplistic GUI, which would depend only on QT. That could come
> useful for testing the core functionality, and it would be cross-platform.
> This is yet another reason why I'd prefer a layer of abstraction
> independent from KDE.
You would have to re-implement most parts of KDevPlatform in this case. This 
would be something like: ProjectManager, DocumentManager, PluginManager. These 
are IMHO all things, we don't want to care about, since we want to provide 
something to develop and simulate electronic circuits and program MCUs. But 
well, of course I'm willing to do all this with Qt-only libraries, for now I 
just don't see, how this can be done without rewriting the parts of 
KDevPlatform, we use.

>   Also there are some files where multiple classes defined in them, for
> example CircuitDocument and CircuitDocumentPrivate,  KTL*Factory. This
> shouldn't be a problem by itself, as those are internal classes, but the
> order they appear in the file might be confusing. Maybe make them inner
> classes?
There is a difference between CircuitDocument(Private) and these Factory 
classes. Providing private class implementation for a class withing a Plugin 
is not mandatory. In this case, the private classes are used to remove the 
implementation from the class (API) definition and implementation. This is 
needed for all classes in src/interfaces/ to provide binary compatibility. For 
Plugins (which are implementations of these interface classes), this is not 
needed. I learned about that after creating private class for CircuitDocument, 
so I didn't change it, yet ;) If these classes grow to large, they can be 
separated into their own files named like classname_p.(h|cpp).
Concerning the Factory classes, these are mostly generated by cpp macros 
provided by KDE and KDevPlatform. These classes are used to dynamically create 
instances of the Plugins classes which themselves implement some public 
interface. It's not possible to make them inner classes. In the case of the 
private class implementations, this would just bring the problem back, that it 
solves (resulting in binary incompatibility, if something changes withing the 
private class). In the case of the Factory classes, they are used by the 
KDevPlatform to create the Views to be displayed withing the Shell or create 
some extra entries for the context-menu. I don't know, if this will still 
work, when hiding them within some class.

Can you provide a patch (in a new branch) to demonstrate, what you would like 
to change? As I said, it should be okay to merge CircuitDocument and 
CircuitDocumentPrivate, but for the Factory classes, I think they should stay 
as they were. This is also the way these are used throughout KDE code. May be, 
I just miss your point here and you are right and we should change some 
detail.

>   Yet another note: the predefined GNU GPL headers are not completed
> everywhere in the source files; it would look better if that would be done.
I mentioned that, too :S Sometimes I just forgot about it. There should also 
be 2 different forms of these headers, because I used vim (and some macros 
doing this stuff for me) before KDevelop became usable, again.. I wanted to 
clean these things up, one day. For now, this has low priority for me...

>   What is your opinion?
Well, thanks for having a look into the code. May be, it's time to start with 
the review-based work-flow. Meaning, from now on I will only commit patches 
into the kde4-port branch, after they have been discussed on the ML. This 
should prevent such things like the Private class for an implementation, 
because I would have needed to explain why I did it, before it hits the kde4-
port branch.

bye then
julian