Hi,
as you might have noted I did some changes deep in the bowels of LibreOffice
Writer, namely the SwClient/SwModify classes that almost all Writer objects
derive from. So what do these do? They are a rather unfortunate attempt at
implementing the observer pattern. Namely, the original intend was to allow all
SwClients to watch a SwModify for events:
- You could call pModify->ModifyBroadcast(...) on a SwModify, which ...
- ... hands the arguments to all SwClients on this SwClient and calls the
event handler pClient->Modify(...) on them. The pClient deregistering from
the pModify in this event handler should be handled gracefully.
This sounds not too bad so far, however it has been tainted by some design
decisions, namely:
- The clients are kept in a intrusive double linked list, thus each client can
only observe at most _one_ SwModify (and creating likely unexpected behaviour
in "dreaded diamond" classes).
- The "message" is a rather unwieldy pair of SfxPoolItem pointers.
- pModify->ModifyBroadcast(...) was extended to allow sending the message to
only a specific "type" (as in tools/rtti.hxx type), thus breaking the
possible dependency inversion by the observer pattern
- There are some weird side-effects triggered by 'magic bools' in SwModify.
- If a SwModify is dying it re-registers all SwClients registered at it on the
object it itself is registered in as a client.
This has lead to some hacks around this design, making things even more
"interesting":
- Often, instead of passing the clumsy two-pointer message to
ModifyBroadcast(), instead the SwModify directly iterates over clients and
does downcasting to interesting classes and triggers code on those,
creating really tight coupling.
- Because of the "client can only listen to one modify" limitation, hacks are
needed for objects that need to observe more event sources. This is SwDepend.
- Because of the "client can only listen to one modify", many SwClients are
assumed only registered to a specific SwModify: They then use the
GetRegisteredIn() function to find that SwModify and downcast it to whatever
they expect it to be.
So, what did my recent changes do? In fact not, as much as should be done, but
its a start:
- add unittests for these
- allow inlining most of this code
- killed the untyped SwClientIter, the slightly better (because stronger typed)
SwIterator<> template class should be used instead
- added a template specialization for SwIterator<SwClient,...>, which should
make it as fast as the old SwClientIter for iterating over (~untyped) SwClients
- mba already added the option to send a more generic SfxHint& as a message to
clients, with the ultimate goal to unify this SwClient/SwModify mess with the
SfxBroadcaster/SfxListener implementation, to have at least one (weird)
observer pattern less in our codebase. These changes make SfxHints the
default message -- the old SfxPoolItem*-pairs are tunneled through these.
- made the SwIterator, SwDepend and LegacyModifyHint classes final
- killed some accessor functions, that were unused and should rather be kept
private
IMHO what needs to happen to further untangle this mess:
- Pass final classes derived from SfxHint as messages, instead of the clumsy
old SfxPoolItem*-pair where appropriate.
- While SwIterator<> gives some more typesafety than the old SwClientIter, it
still creates quite a mess dependency-wise. Also replace these with passing a
SfxHint& that is just handled properly by the receiving SwClients.
- Often something like SwIterator<Foo,...>(this).First() is used with
confidence that there is only exactly one registree of a certain type.
Instead of iterating through a possibly longer list of clients, consider having
a point reserved for this specific client at the modify.
- Ultimately: Use a SfxBroadcaster/SfxListener API only in Writer, instead of
yet-another-message-passing. And yes, SfxBroadcaster etc. might be
wrapped/merged down itself around something like boost::signals/whatever.
- Ultimately: Slowly soften the implied "everything in writer need to derive
from SwClient" situation.
- Ultimately: Once this SwIterator<> mess is gone, we also have one barrier
less for getting rid of the hackish tools/rtti.hxx stuff.
Given that:
grp SwIterator sw/|wc -l
shows 220 cases of SwIterator being used (58 of which seemingly only interested
in just using .First() on it once), this is still a bit of work though ...
FWIW, the changes done so far killed some ~150 LOC and were measured to have no
negative performance impact (actually 0.1% faster according to callgrind).
If there is passionate objection to any of the above, we might discuss it on
the next ESC call.
Best,
Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Locked
signature.asc (188 bytes)