Rework Tool properties architecture

[MrStevns]

Currently all our tool properties are exposed to BaseTool which means that whenever BaseTool is accessed from somewhere else all its properties are also "leaked". Right now we don't have a lot of properties so it's not that big of a deal but it doesn't scale that well... Also, it's been bothering me so much lately that I decided to put my other Floating Selection PR on hold again until I found a solution for it.

My proposal for solving this is called ToolSettings. The ToolSettings struct controls the settings of all our tool properties and allows extensibility through inheritance.

ToolSettings

ToolSettings controls the following:

• loading properties
• Saving properties
• Resetting properties
• Setting BaseValue
• Retrieve min, max, default and base value through a PropertyInfo structure.

When a tool is being loaded, two things needs to be called.
ToolSettings::setDefauts. This ensures that our tool setting properties can be manipulated, loaded and saved later.
ToolSettings::load. This is where we load the most recent stored settings.

When calling load an identifier needs to be specified. The identifier should be set to the name of the tool but this could easily become layer type + tool name, as long as it's consistent and static.

To declare the default properties of a tool, you must specify the min, max and default properties like so:

Here's an example what that could look like:

info[BucketSettings::FILLTHICKNESS_VALUE] = { 1.0, 100.0, 4.0 }; // min, max, default
info[BucketSettings::TOLERANCE_VALUE] = { 1, 100, 32 };
info[BucketSettings::TOLERANCE_ON] = false; // default

Internally the values are stored in a union, meaning that it only stores memory for the given type it's created with.

You might be wondering... what about the base value? we used to fetch that from QSettings, sometimes from the widget, other times from the tools load method. This is now handled by the ToolSettings::load method which Internally carries the responsibility of either fetching the value we have stored via QSettings or using the values we provide through the QHash above.

ToolSettings::Type Key

Right now each ToolSettings must have its own Type enum with a start and end value. I've picked an arbitrary 100 numbers range which should be sufficient for never having to change the bounds but i'm open to other suggestions.

I also thought about having a big enum like we had with ToolPropertyType, the naming would be something like:

class enum ToolSettingsType
{
	STROKETOOL_WIDTH_VALUE,
	STROKETOOL_FEATHER_VALUE,
	STROKETOOL_STABLIZATION_VALUE,
	STROKETOOL_STABLIZATION_ON
	BUCKETTOOL_.....
	POLYLINETOOL_......
}

The biggest reason I can think of for going in that direction would be to be able to use the enum as a key rather than an integer.

If people like that better, i'm open to change it to that instead. For now though there are assertions in various places to make it easy to spot when you try to access, load or save a value on the wrong model and that's has been sufficient so far.

Setting baseValue

Updating a ToolSettings base value is done through ToolSettings::setBaseValue which takes in a integer as key and a PropertyInfo as value.

mSettings->setBaseValue(StrokeSettings::WIDTH_VALUE, 1.0);
As opposed to
mSettings->setBaseValue(100, PropertyInfo(1.0));

I thought about creating a different ToolSetting model for all our different Stroke tools but decided against it for sake of the size of the PR. I did want to see whether my model could actually deliver on potential extensibility needs and as such created PolylineSettings to prove that specifically. So if we were to decouple say EraserTool from StrokeSettings, then following the setup of PolylineSettings would be the way to go.

Tool property name alignment

Prior to this PR, there was no naming convention for tool settings, meaning that whatever came to mind could be used a setting and there was no correlation between the setting and the tool except the name it shared eg "brush".

I've tried to align this by creating the ToolSettings::identifier. Granted, someone still has to write it but it's now tied to the specific ToolSettings struct and not some arbitrary combination of names. You won't be able to reuse the same setting for a different tool like we currently have in some places.

When the setting is saved now it will be stored in a group based on an identifier, meaning you should be able to find a toolsetting like so: brushTool/Width where brushTool is the ToolSettings identifier and /Width is the name of the specific setting identifier.

GUI Communication and Responsibility

Rather than going through ToolManager to update tool properties and requiring other listeners to know about ToolPropertyType, I decided to make a tighter coupling between a tool and it's respective tool widget. You can see the result of that in for example BucketOptionsWidget.

The only responsibility of the widget now is to add GUI elements, their labeling and things that's generally UI bound. The rest now comes from ToolSettings. The widget no longer has to nor should be allowed to update its stored/QSettings settings, nor should it set base, min or max values itself. This data now comes from the ToolSettings model and is setup prior to calling the widget and updating any property requires going through the model.

Updating a GUI elements in relation to a UI change builds on a UI -> Model -> UI update cycle. similar to MVVM but we don't have ViewModels so ignoring that part it's just:

UI -> Model

connect(ui->expandSlider, &SpinSlider::valueChanged, [=](int value) {
        mBucketTool->setFillExpand(value);
    });

Model -> UI

connect(mBucketTool, &BucketTool::fillExpandChanged, this, [=](int value) {
       setFillExpand(value);
    });

UI visibility

Another thing the tool options widget no longer needs to be concerned with, is the relation between what the tool can do for the given layer type. Rather than having a fixed map of which tool properties are enabled in general, each tool now declare what properties are usable as part of BaseTool::loadSettings like so:

mPropertyUsed[BucketSettings::FILLTHICKNESS_VALUE] = { Layer::VECTOR };
mPropertyUsed[BucketSettings::TOLERANCE_VALUE] = { Layer::BITMAP };
mPropertyUsed[BucketSettings::TOLERANCE_ON] = { Layer::BITMAP };
mPropertyUsed[BucketSettings::FILLEXPAND_VALUE] = { Layer::BITMAP };
mPropertyUsed[BucketSettings::FILLEXPAND_ON] = { Layer::BITMAP };
mPropertyUsed[BucketSettings::FILLLAYERREFERENCEMODE_VALUE] = { Layer::BITMAP };
mPropertyUsed[BucketSettings::FILLMODE_VALUE] = { Layer::BITMAP };

For the widget this means that the visibility logic becomes as simple as this:

void BucketOptionsWidget::updatePropertyVisibility()
{
    ui->strokeThicknessSlider->setVisible(mBucketTool->isPropertyEnabled(BucketSettings::FILLTHICKNESS_VALUE));
    ui->strokeThicknessSpinBox->setVisible(mBucketTool->isPropertyEnabled(BucketSettings::FILLTHICKNESS_VALUE));
    ui->colorToleranceCheckbox->setVisible(mBucketTool->isPropertyEnabled(BucketSettings::TOLERANCE_ON));
    ui->colorToleranceSlider->setVisible(mBucketTool->isPropertyEnabled(BucketSettings::TOLERANCE_VALUE));
....
}

I haven't quite decided who will be responsible for notifying about layer change and tool change updates to the UI. For now it's the main ToolOptions widget but that's still open for debate.

What's missing or still in the air:

• Settings name migration. A tool to migrate from the old QSettings names to the new ones.
• Create a TransformOptionsWidget to contain both properties of Select and move tool
• Boolean naming convention.. eg. ON vs Enabled, Use... we use a bit of both however I like ON for the Enum... just not everywhere else... writing brushWidthON is worse than brushWidthEnabled.
• Use ToolSettings min/max in the rest of the Tool option Widgets
• Cleanup and some final testing...

[MrStevns]

I believe this PR is ready to be reviewed now 🚀

[MrStevns]

Fixed a small bug and made sure that we're up to date with the latest changes from master

[chchwy]

I will be looking at this PR soon. It may take some time for such a big change.

[MrStevns]

I look forward to hearing your thoughts Matt. I'm also open to making changes if you think that'll be necessary.

I'd recommend reading my description above first to get an idea of the implementation and jump into the code after that