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

List:       kopete-devel
Subject:    Re: [Kopete-devel] [Long] next release
From:       Martijn Klingens <klingens () kde ! org>
Date:       2002-04-29 19:48:03
[Download RAW message or body]

On Monday 29 April 2002 20:55, Andres Krapf wrote:
> no, whether to use a QMap internally or not is a plugin implementation
> detail, not a design decision. if by good OO you mean being able to
> m_contact[name], the same semantics could be achieved with another design.
> the real design decision is whether we want to make a service available to
> all plugins to do the mapping or let each plugin do it's own.
>
> i think each plugin doing it's own is more bug prone.. and i usually think
> it's better to let the framework do work if possible...

1. I consider the framework the Qt data management classes in this case. If 
you use them there really is so little work left that I don't think it's very 
error-prone. In fact I think adding a API for that might result in an API 
that is harder to use and work with than the Qt template classes.

2. If the plugin manages the data it can optimize the data structure for its 
own use, i.e. use the structure that fits the design best. Originally MSN 
used a QList of KMSNContacts, simple structs with the most important data of 
an MSN id. I could have made that a QList of MSNContact, the new 
KopeteContact based class. Instead I made it a QMap because for *this* plugin 
it fit much better. Other plugins might prefer QList though. If it's enforced 
in the API you might be using a sub-optimal data structure, which would make 
the code more complex and error prone. Adding the basic data manipulation 
methods in the KMM stuffs code in a class where it technically doesn't belong 
and in the end either gives you zillions of helper methods or requires you to 
make the data structure public anyway for more complex operations. Either of 
which again is error prone and complicates code.

3. UniqueId is clearly a hack to work around the problem of id's that might 
not be strings. Putting this all inside the protocol not only avoids the 
hack, but even the problem that made the hack sort of necessary in the first 
place.

> > If the design of the code is already clean and object oriented that would
> > be easy. Otherwise it will be a true pain.
>
> i don't know enough about the plugins to go through that... i'll have to
> leave this to the plugin authors.

Well, I'm not the MSN author either. Olaf Lueg wrote the original KMerlin code 
and Duncan made the Kopete MSN plugin of that. But my refactoring and 
restructuring of the MSN code made me learn quite a lot about the code. That 
is, once the code used decent data structures, because instantly the code 
became actually readable! Until then I was just mechanically refactoring the 
complex parts, just looking through the code if I saw certain patterns 
replacing them with more elegant solutions. What you see now is far from the 
final solution, but the code has come a long way and should be more or less 
understandable to someone who sees the code for the first time. Something 
which definitely wasn't the case for the old code. The aim, of course, is to 
have all plugins use a clean and simple design, by modelling small 
independent pieces that don't depend on eachother.

Bottom line: start refactoring a plugin and you learn as you go.

Second bottom line: that takes more time.

Third bottom line: it's extremely well worth it.

> my problem is not how i access the list mapping, it's how the list gets
> created/updated. that's what the pain in the ass would be for aim/icq... at
> least for me, since i don't know the plugins well.

Well, I obviously prefer to fix the way the list is managed, but that might 
not be very easy to do. The other solution, as I pointed out, is to indeed 
use the UniqueId hack, but *only* inside the protocol and not expose it 
outside. Shouldn't be too hard to do.

> > > i hadn't thought of that, but using UniqueId would allow you to do
> > > something like KopeteContact->getUniqueId(), dynamic cast this and then
> > > ->getProtocolId()
> >
> > MSNProtocol::protocol(), anyone? ;-)
>
> nope, that's not it... i meant KopeteContact->getUniqueId(), dynamic cast
> this and then ->getProtocolSpecificIdAssociatedWithTheContact().
> reverse mapping, that is.

Again, s/get// ;-)

But besides the nitpicking, that's just the same as casting a KopeteContact to 
an MSNContact and calling msnId() on it, right? libkopete can't do the 
dynamic cast anyway (it knows nothing of the protocol, and if it did this 
would clearly be a job for q_cast...). I don't think you meant that either. 
And I don't see the difference between KopeteContact->MSNContact casting or 
KopeteUniqueId->MSNUniqueId casting, besides extra logic, code, complexity 
and more.

Martijn

_______________________________________________
Kopete-devel mailing list
Kopete-devel@mail.kde.org
http://mail.kde.org/mailman/listinfo/kopete-devel
[prev in list] [next in list] [prev in thread] [next in thread]