Hi Matúš, On Tuesday, 2012-07-31 19:09:45 +0200, Matúš Kukan wrote:
To be able to call the same methods from the view regardless of whether we have direct or collaborative input we need ScDocFuncDirect and ScDocFuncSend, otherwise we'd have to distinguish between direct and collab in each method.Right, it would be about adding one 'if' into each method.
And then call another method anyway (or have all code in the if block) for collaboration.. to me a derived ScDocFuncSend looks cleaner.
For the receiver we need ScDocFuncRecv to process the command and send it as input to ScDocFuncDirect. The functionality of ScDocFuncRecv theoretically could be implemented anywhere, but deriving it from ScDocFunc gives access to ScDocShell (currently available as member in ScDocFuncRecv also, but see abstract base class below), and IMHO deriving it clarifies the relation between ScDocFunc* classes.But currently ScDocFuncRecv is not deriving from anything
Hum yes, sorry I mixed that up and got carried away..
and there is just ~one method, which is possible to move into ScDocFuncSend. See attached diff as illustration for what I have in mind.
Sure, would work. Just why do it so and call methods of the base class
instead of having the little ScDocFuncRecv helper class with
a ScDocFuncDirect pointer?
Btw, we should not end up with yet another
if(string==...){}else if(string==...){}else ...
chain for the commands that looks ugly and slows down things again, I'd
like more a hashmap resolving to IDs that can be switch-cased on.
Chaining ScDocFuncRecv into ScDocFuncSend in real collab mode is only needed for ScDocFuncSend::SetCollaboration() so could be passed there instead, but in dev/demo mode it is also needed in ScDocFuncSend::SendMessage() to emulate the broadcast, having it always as member IMHO is fine.I was ignoring demo mode for now, I believe it can be solved later.
I think it's good to keep an eye on it as it will come handy when working on the send/recv commands.
Also ScDocFuncDirect is just ScDocFunc with own costructor, not sure what for.Ideally we'll have ScDocFunc as an abstract base class later, do not construct the derived classes with an extra ScDocShell and have ScDocFuncDirect implement the then abstract methods for which implementation currently still lives at ScDocFunc. Just ignore that little indirection for now.Sorry if that's obvious but I don't understand why ScDocFunc should be abstract. I've never understood the comment in sendfunc.cxx: // FIXME: really ScDocFunc should be an abstract base, so // we don't need the rDocSh hack/pointer ScDocFuncSend::ScDocFuncSend( ScDocShell& rDocSh, ScDocFuncRecv *pDirect ) : ScDocFunc( rDocSh ), How does that depend on whether ScDocFunc is abstract ( that means it has at least one pure virtual method I guess ?
yes
) or not ?
IMHO whether we need to pass rDocSh or not is not the point, but the benefit of an abstract base class would be that a pure virtual method forces the implementation of a derived class to implement exactly that signature. Adding a parameter to a method of just one of the classes and forgetting the other is not possible, it would need to be added to ScDocFunc and both ScDocFuncDirect and ScDocFuncSend derivatives would have to implement it. It helps the developer.
I actually see no benefit in having one ScDocFunc, why do you think it would be nicer?Hmm, because there would be only one but that may be also (ideo)logically bad. Depends where you want to distinguish between collaboration modes, what ScDocFunc means ...
ScDocFunc so far was the glue between view and document or API and document, actually much of its functionality previously was in ScViewFunc before it was extracted to work also with API calls. To me having a derived ScDocFuncSend that implements virtual methods for the collaboration case sounds logical, I think that in fact the receiving end should act much like if it was called via API. Eike -- LibreOffice Calc developer. Number formatter stricken i18n transpositionizer. GnuPG key 0x293C05FD : 997A 4C60 CE41 0149 0DB3 9E96 2F1A D073 293C 05FD
Attachment:
pgptekOpcv7zk.pgp
Description: PGP signature