Implementing actions 3

[szeli1]

This PR is in the series of PRs in the goal of achieving #7734

This is a single PR, unlike #7895, there is no roadmap.

This PR implements Commands, a layer between the GUI and the core. Most user interaction will be routed through Commands. Commands will provide a better way for journalling and overall clarity in code. The end goal is replacing the current journalling system. Commands do not store information in strings compared to JO, this results in more optimized code. Commands are journalled when invoked, this will help remove addJournalCheckPoint();. Commands are designed to be invoked from anywhere, this helps the UI (see #7734).

This PR introduces Command.h that contains many command classes with different use cases. In code, objects store command objects that are accessible to command invokers. Command invokers can use commands which results in the command being pushed to a command stack, that replaces the journalling stack.
This command system supports lambdas and function pointers, it supports actions without argument and actions with an argument.

Example: In AutomatableModel a command object exists for setValue() and FloatModelEditorBase (invoker) uses it to set a value.

This PR also added a test for commands where all of the added command types are used and the command stack is tested.

How to test:

1. Open lmms
2. drag a knob to adjust it's value. This action uses the command system.
3. test entering a value too.
4. Build lmms and run the tests by make tests and tests/tests or tests/CommandTest

[allejok96]

I have only glossed over the code, but I must say on a meta level this PR is really well made, the description is clear, the changes are concise, you created a new API and implemented it in just one place as an example, you even made tests for it. Perfect execution ⭐

[sakertooth]

I don't see why LambdaCommand couldn't be used for everything, I don't think there's a need to make separate classes that say for e.g. use this command when there's parameters.

[messmerd]

I think the command functions in Command.h could be simplified and made generic using partial specializations. I'm experimenting with it locally to see what could be done.

[szeli1]

I don't see why LambdaCommand couldn't be used for everything, I don't think there's a need to make separate classes that say for e.g. use this command when there's parameters.

To do functions with arguments, you need to pass in data. To undo these functions, you need to get the original data (90% of the cases). If you want to use "LambdaCommand" (meaning a lambda without arguments), you will have to capture the do and the undo data. To capture do and undo data, you will have to declare a lambda case by case where a command is used. I believe your suggestion would lead to implementing a lambda every time a programmer wants to use a command.

To use "ParamCommandLambda", I believe you can't declare it as a member (I did tests and declaring in the header didn't lead to type deduction), and you do not know the lambda's type beforehand (because the identifier is determined at compile time), this leads you to declare a member parent class pointer and constructing a new "ParamCommandLambda" object in the c++ file. Using it in the invoker is also worse, because you can't use operator () on a pointer I believe.

Dealing with these options is not convenient. That is why the function pointer types exist, because 1. their type is known, and they do not have to be deducted, and they can be declared as a regular member object (not a pointer) so they are automatically deleted so no chance for memory leaks. Basically the regular function pointer types are way more convenient.

If we would like to do the (first) "LambdaCommand" example I described, then It will fail in other aspects: code quality and complexity. I realized commands need to be in core, because gui calls core's functions all over the place so making commands for core placed in the gui would lead to silent rules (gui could only call gui's commands on core expect if it is their own core), and it would lead to repeating code (declaration of setValue command for AutomatableModels in almost every gui widget)

[szeli1]

I think the command functions in Command.h could be simplified and made generic using partial specializations. I'm experimenting with it locally to see what could be done.

The command functions only use 1 template argument, because if the functions use arguments, they will have to provide undo arguments to the undo function. "n" command arguments would lead to complexity for feeding back the undo data for the undo function. I don't know what you mean by "partial specializations", but if you would like to optimize templates, keep the design in mind.

[szeli1]

Thanks for you feedback @allejok96 @sakertooth @messmerd !

[messmerd]

Quick review

[messmerd]

This getFnPtr vs paramPtr thing is very strange and makes it more difficult to understand and use this class template. And I don't understand what the benefit of it is.

[szeli1]

This class is easier to construct compared to lambda commands.

[messmerd]

These class templates are very confusing to me.

My questions:

• How do I know which class template to use? The names are not very helpful to me.
• Some use "do"/"undo" terminology, while others use "getter"/"setter". It seems like it isn't just a difference between using a lambda, function pointer, or member function pointer - they seem to have different semantics.
• It's unclear what the function signatures of the do/undo or getter/setter functions are supposed to be.
• What is the command data parameter, and what does it mean for it to be present vs not present?

Could you add comments explaining some of these confusions?

I'm a user who is not an expert in how any of this works, so I'd like an interface whose usage is as clear and obvious as possible just from the names of the class templates and methods. And what isn't clear from that should be explained in comments.

And as much as possible, it should be type-safe so that it's difficult to misuse without it being a compile-time error.

[szeli1]

How do I know which class template to use? The names are not very helpful to me.

I gave better descriptions to them.

Some use "do"/"undo" terminology, while others use "getter"/"setter". It seems like it isn't just a difference between using a lambda, function pointer, or member function pointer - they seem to have different semantics.

This is true. One of the goals for this PR is to not have to implement / declare more functions in classes. The command objects try to use already existing functions or lambdas to do their job.

What is the command data parameter, and what does it mean for it to be present vs not present?

Some functions like setValue(float value) have input parameters. Usually to "do" / execute these functions, we need to provide the value. It is the same with undo value, when setValue() is undone, the old function parameter should be provided to it. Command data parameter stores this value, and an undo value (this can be optimized, but for future compatibility, I left it with 2 values). Some functions do not require input parameters to run (like inverting sample clips).

Could you add comments explaining some of these confusions?

Hopefully I managed to add better comments

And as much as possible, it should be type-safe so that it's difficult to misuse without it being a compile-time error.

The design ensures compile time errors for almost all commands. The lambda command with parameters (inputs) is the easiest to miss use.

[messmerd]

This should not be public, and maybe it should be renamed m_setValueCommand.

[szeli1]

How will an invoker call it.. By having a get function to it. Defeats the point of having these objects if we end up using functions.

There is a lot of conflict about how to achieve this functionality, but I believe this current PR is the simplest to use both for invokers and the objects that have commands.

Luckily I've already made a worse PR that uses functions instead of command objects. 7895

[messmerd]

If this class template or any of the others are an implementation detail that users of these commands should not have to know about, they should be placed in a lmms::detail namespace.

[szeli1]

Users need to know about these. Only CommandBase and CommandDataBase is "hidden".

[messmerd]

Please use std::unique_ptr and std::make_unique rather than doing manual memory management.

And Derived* (or std::unique_ptr<Derived>) implicitly converts to Base* (or std::unique_ptr<Base>), so there's no need for a static cast.

[messmerd]

Suggested change

	QVERIFY(obj.m_value == 0.0f); // these should be the default values
	QCOMPARE(obj.m_value, 0.0f); // these should be the default values

QCOMPARE is better than QVERIFY since if it fails, it will print out what the actual value of obj.m_value was and what it expected it to be, but QVERIFY will only tell you that it failed.

[messmerd]

If you wanted to have an easy way to convert an existing getter/setter pair into a command, my dream interface would look something like this:

class Foo
{
public:
    int getValue() { ... }
    void setValue(int) { ... }

    auto valueCommand()
    {
        return GetterSetterCommand<&Foo::getValue, &Foo::setValue>{this};
    }
};

// somewhere else
void myGuiMethod()
{
    auto command = getFoo().valueCommand();
    // use command here
}

GetterSetterCommand would be a simple Command adapter provided for ease of use rather than some special case of the action system's core design.

[allejok96]

Instead of writing a review at this stage I'll dump my experiments.

I agree with @szeli1 that the call to create a command should be as easy and short as possible. If that means having many overloads or types, it's fine by me.

In this experiment I implemented the lambda command, do/undo function pointer command, and function pointer with value command, just to see what's possible. But there is a possibility to just use a LambdaCommand for everything and have multiple overloads to create that from whatever data you pass it (function pointer, with or without argument)...

I let the CommandStack handle all the creation of Commands, because that's is the interface the user will most likely use anyway, and it makes the code less verbose for the user.

The command stack does not detect when objects are deleted, thus it can still try to call them when undoing. This will need fixing. Can use Qt's signal slot mechanisms or is this strictly a non-Qt solution? That is for later discussion.

---

Command.h

class LMMS_EXPORT Command
{
public:
	virtual ~Command() = default;
	virtual void execute() = 0;
	virtual void undo() = 0;
};



// Usage: LambdaCommand([] { doThings(); }, [] { undoThings(); })
template<typename DoLambda, typename UndoLambda>
class LMMS_EXPORT LambdaCommand : public Command
{
public:
	LambdaCommand(DoLambda doLambda, UndoLambda undoLambda)  :
		m_doLambda(doLambda),
		m_undoLambda(undoLambda)
	{}
	void execute() override { m_doLambda(); }
	void undo() override { m_undoLambda(); }

protected:
	DoLambda m_doLambda;
	UndoLambda m_undoLambda;
};

// Fixes warning: 'LambdaCommand' may not intend to support class template argument deduction
template<typename DoLambda, typename UndoLambda>
LambdaCommand(DoLambda, UndoLambda) -> LambdaCommand<DoLambda, UndoLambda>;




// Usage: SetterFunctionCommand(&object, &Object::setValue, newValue, oldValue)
// TODO this doesn't need to be a separate type, CommandStack::add could just have a overload
// that creates a LambdaCommand from a function pointer and two values
template<typename Obj, typename Setter, typename T>
class LMMS_EXPORT SetterCommand : public Command
{
public:
	SetterCommand(Obj obj, Setter setter, T newValue, T oldValue)  :
		m_object(obj),
		m_setter(setter),
		m_newValue(newValue),
		m_oldValue(oldValue)
	{}
	void execute() override { (m_object->*m_setter)(m_newValue); }
	void undo() override { (m_object->*m_setter)(m_oldValue); }

protected:
	Obj m_object;
	Setter m_setter;
	T m_newValue;
	T m_oldValue;
};


// Fixes warning: 'SetterFunctionCommand' may not intend to support class template argument deduction
template<typename Obj, typename Setter, typename T>
SetterCommand(Obj, Setter, T, T) -> SetterCommand<Obj, Setter, T>;




// Usage: FunctionPairCommand(&object, &Object::doAction, &Object::undoAction)
// TODO is this even that useful? Where would this be used?
template<typename Obj, typename DoFunction, typename UndoFunction>
class LMMS_EXPORT FunctionPairCommand : public Command
{
public:
	FunctionPairCommand(Obj obj, DoFunction doFunction, UndoFunction undoFunction)  :
		m_object(obj),
		m_doFunction(doFunction),
		m_undoFunction(undoFunction)
	{}
	void execute() override { (m_object->*m_doFunction)(); }
	void undo() override { (m_object->*m_undoFunction)(); }

protected:
	Obj m_object;
	DoFunction m_doFunction;
	UndoFunction m_undoFunction;
};

// Fixes warning: 'FunctionPairCommand' may not intend to support class template argument deduction
template<typename Obj, typename DoFunction, typename UndoFunction>
FunctionPairCommand(Obj, DoFunction, UndoFunction) -> FunctionPairCommand<Obj, DoFunction, UndoFunction>;

---

CommandStack.h

class LMMS_EXPORT CommandStack
{
public:
	CommandStack() = default;
	~CommandStack() = default;

	// Add and execute a command
	void add(std::unique_ptr<Command>&& command)
	{
		m_stack.resize(m_stack.size() - m_undoSize);
		m_stack.push_back(std::move(command));
		command->execute();
	}

	// These are just overloads to create different types of Commands...

	template<typename DoLambda, typename UndoLambda>
	void add(DoLambda doLambda, UndoLambda undoLambda)
	{
		// C++20 can omit the <decltype ...> due to template argument deduction
		add(std::make_unique<LambdaCommand<decltype(doLambda), decltype(undoLambda)>>(doLambda, undoLambda));
	}

	template<typename Obj, typename DoFunction, typename UndoFunction>
	void pushBack(Obj* obj, DoFunction doFn, UndoFunction undoFn)
	{
		add(std::make_unique<FunctionPairCommand<decltype(obj), decltype(doFn), decltype(undoFn)>>(obj, doFn, undoFn));
	}

	template<typename Obj, typename Setter, typename T>
	void add(Obj* obj, Setter setter, T newValue, T oldValue)
	{
		add(std::make_unique<SetterCommand<decltype(obj), decltype(setter), decltype(newValue)>>(obj, setter, newValue, oldValue));
	}

	void undo()
	{
		if (m_undoSize <= 0) { return; }
		--m_undoSize;
		m_stack[m_undoSize]->undo();
	}

	void redo()
	{
		if (m_undoSize >= m_stack.size()) { return; }
		m_stack[m_undoSize]->execute();
		++m_undoSize;
	}

private:
	size_t m_undoSize = 0;
	std::vector<std::unique_ptr<Command>> m_stack;
};

---

Example using lambdas. Pros: can call private methods. Cons: very verbose, must make sure to capture by value.

void AutomatableModel::setValue(const float value)
{
	auto oldValue = m_value;
	Engine::commandStack()->add(
		// must capture both the new and old value "by value" not by reference
		[this, value]{ setValueWithoutJournal(value); },
		[this, oldValue]{ setValueWithoutJournal(oldValue); }
	);
}

Example using function pointers. Pros: easy to use. Cons: the setter method must be public.

void AutomatableModel::setValue(const float value)
{
	Engine::commandStack()->add(this, &AutomatableModel::setValueWithoutJournal, value, m_value);
}

[szeli1]

The command stack does not detect when objects are deleted, thus it can still try to call them when undoing. This will need fixing. Can use Qt's signal slot mechanisms or is this strictly a non-Qt solution? That is for later discussion.

Commands delete themself when they are destructed so this issue is solved. Edit: sorry you meant it about your code

My problem is "what happens when we delete something". For example somebody deletes a clip. If this clip is destructed, then we can't recover it with this system. Saker had an idea to place these "deleted" objects in an inactive state instead of actually deleting them, so we can easily undo the delete operation. Implementing this seems difficult, we essentially need to "forget" an object temporarily, but I don't have any better idea.