On Thu, Apr 2, 2009 at 3:49 PM, Julie S <[email protected]> wrote:
> Take 2: I believe you were not feeling well when I first posted this and you
> might have missed it.
Oh yes, that's right -- I had seen it, but you're right, I wasn't
feeling up to doing much at the time and had since forgotten.
> Could you lead me through this a bit? Let's look at NotationView.cpp
>
> line59: m_commandRegistry = new NotationCommandRegistry(this);
>
> This creates a new NotationCommandRegisty and stores in in m_commandRegistry
>
> We don't do anything with this pointer, am I correct?
Right. All of the work that it ever does is done inside the
constructor. The only reason we need to keep the pointer around is so
as to delete it on destruction -- something I notice I have forgotten
to do. (As the command registry is a QObject, it wouldn't be
necessary to delete it explicitly if it was parented to the notation
view some way or other... but it doesn't look as if it is, despite
taking the notation view as an argument.)
> now inside NotationCommandRegistry.
>
> line 58: AddFingeringMarkCommand::registerCommand(this);
>
> At this point I'm a little confused. Is this where the constructor
> AddFingerMarkCommand() is called or is it never called? Or is it somehow
> called implicitly elsewhere. My knowledge of C++ is failing me here.
It's not called here. registerCommand() is declared "static" in
AddFingeringMarkCommand, which means it does not need an object
instance to be called upon.
The registerCommand function calls back on
CommandRegistry::registerCommand, which associates a newly-constructed
"builder" object (some subclass of AbstractCommandBuilder) with the
action name, and calls addAction, implemented in ActionCommandRegistry
using our old friend ActionFileClient::createAction.
So effectively, calling this static function
(AddFingeringMarkCommand::registerCommand(this)) causes
this->createAction(actionName, slot) to be called, with the slot set
to the command registry's standard dispatch slot
(slotInvokeCommand()). This calls the previously registered builder
object's build() function, which constructs the actual command ("new
CommandType" in CommandRegistry.h, where CommandType is a template
argument that in this case happens to be AddFingeringMarkCommand).
> If the constructor is being called, I'm not certain how it is getting its
> arguments such as fingering, and selection passed to it.
Well, it is not called at that time, but the answer to the latter
question is that the builder's build() function calls
AddFingeringMarkCommand::getArguments() (another static function) to
extract these arguments from the action name.
It is all rather complicated, yes. The fact that this stuff is there
at all is testament to how keen I was to get all of that boilerplate
out of NotationView. Of course, had the action file idea come along
sooner, there would have been less boilerplate to remove, and I
wouldn't have minded so much.
> Other functions inside AddFingerMarkCommand:
> getGlobalName() appears just to be used by AddFingerMarkCommand() constructor
> but is called without arguments, so the getGlobalName() function code itself
> appears dead.
>
> Is this true?
Well, actually that's an interesting point you raise there. I hadn't
really looked closely at this bit since the action file stuff came
along.
The problem is that, when the command is executed and goes on the
undo/redo stack, it has to have a nice human-readable name (so you can
have "Undo Add Fingering 2" or whatever). This comes from calling
getName() on the command.
Originally, what a number of commands did was have a static function
called getGlobalName() which was static, which the view class could
use to look up the action label for use in the menu. This usage has
been superseded by the action file text.
However, many classes also call getGlobalName() to set their own name
(perhaps in the superclass constructor call for NamedCommand, which
provides an implementation of getName()). So in these classes it is
still called, at least in that context.
In the case of AddFingeringMarkCommand it looks like a simple bug that
getGlobalName() is called without arguments from the constructor --
surely fingering should be passed as an argument.
But the interesting part of course is that we really don't want to
have the action text coded up twice, once in the getName() function or
getGlobalName() or whatever of the command that implements the action
and once in the action file. When a command is executed from an
action that was set up using the action file, it should take its name
from the action that was used to execute it. How to do that? Hmm.
> 1. I would like to see the AddXXXCommand() functions re-factored. We can
> remove much of the extraneous internal function calls like getActionName(),
> getArgument(), and other function like getStandardFingering()
getArgument() is used, as mentioned above. getActionName() is
definitely obsolete. getStandardFingerings() could be rendered
obsolete using your rewrite of registerCommand, which I would have no
objection to.
getGlobalName() is not obsolete, but as I also said above, it should
be. We'll have to work out how to get rid of it.
Chris
------------------------------------------------------------------------------
_______________________________________________
Rosegarden-devel mailing list
[email protected] - use the link below to unsubscribe
https://lists.sourceforge.net/lists/listinfo/rosegarden-devel
