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 LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
