Refactor setsquare to use MVC pattern

[rolandlo]

This is a continuation of #3962 and is based on #4137, #4158 and #4159 from @bhennion.
This PR refactors the setsquare. Currently it contains adding a compass, but that will later be moved into another PR.
Previously it contained a compass, but that was moved into PR #4443.
I tried to follow the MVC pattern (push variant) explained by @LittleHuba in #3962 (comment). Only the last two commits are relevant.
I will explain the most relevant commits:

1. MVC pattern
   In 6efc15a the MVC pattern is applied:

The class Setsquare is the model.
The class SetsquareView is the view.
The class SetsquareController is the controller.
The class SetsquareInputHandler is somewhere between a view and a controller.

• The model has a viewPool and a handlerPool for pushing updates to the SetsquareView(s) and SetsquareInputHandler(s).
  It stores the data for the setsquare and the stroke (near the longest side of the setsquare or from a point close to a shorter side of the setsquare to the midpoint of the longest side) and has getters and setters.

• The view only uses the model for querying the viewPool. It doesn't update the model and contains only drawing methods except of the methods to update itself when pushed from the model.

• The input handler just handles input and uses the controller to manipulate the model. It also updates itself when pushed from the model.

• The controller manipulates the model (moves, rotates and scales the setsquare, creates and updates strokes) and contains more methods, which didn't fit in one of the other classes.

For pushing updates to the view and input handler, I use the xoj::util::Listener class; for rendering the setsquare I use the OverlayBase and ToolView classes, all by @bhennion .

There are still two issues is still one issue with the rendering:
1) When activating the setsquare, it is not rendered immediately. It is only rendered after it has been moved/scaled or rotated. I'm not sure why that happens. I tried to notify the setsquare after constructing instances of all classes (in Control.cpp), but that crashes.

2) Since the rendering uses an overlay of one of the page views and the pages are drawn one after the other, when moving the setsquare towards the next page, the fraction that lands on the next page appears to be cut, as if the setsquare was gliding under the next page. It's not like that when moving the setsquare to the page before.

2. Derive Setsquare classes from GeometryTool classes

In order to prepare for other geometry tools (like a compass or a ruler) to be added, in 714e479 the Setsquare classes are derived from more generic GeometryTool classes. Those classes basically contain the functionality of the Setsquare that all geometry tools will most likely share. This makes it easier to implement new geometry tools.

• Setsquare is derived from GeometryTool
• SetsquareView is derived from GeometryToolView
• SetsquareController is derived from GeometryToolController
• SetsquareInputHandler is derived from GeometryToolHandler

This strategy is demonstrated in commit bb51258 PR #4443 where the Compass is added. Although that commit won't be part of this PR, it is included so that the strategy can be checked to work.

Any feedback is welcome. As I had no previous exposure to the MVC pattern, I find it quite tough to find the proper way for this refactoring.

Only the two last commits are relevant:
ff90582 is the combination of the commits in #3962 rebased on #4159. The setsquare is functionally equivalent to the version in master.
The real work is in adfc317

Edit: Updated description, described derivation from GeometryTool classes (Oct 15)
Edit: Updated description after moving the compass into another PR.

[bhennion]

Thanks for the great work!
I haven't had time to check the code in details yet, but I do have input on this point:

2. Since the rendering uses an overlay of one of the page views and the pages are drawn one after the other, when moving the setsquare towards the next page, the fraction that lands on the next page appears to be cut, as if the setsquare was gliding under the next page. It's not like that when moving the setsquare to the page before.

To avoid issues like this, some overlays and tools will have to be owned, not by the PageController, but by the XournalController (currently a mix of XournalView and widget/XournalWidget). The SetSquare is one of those, as are selections for instance.
So in the end, a decent DocumentView class should have its own vector of OverlayView's.

As far as the SetSquare is concerned, I think none of its related classes should be owned by a PageSomething class, and the SetSquare classes should not refer to any specific page (there are bugs on the current master branch if you try to drag the setsquare to a different page and use it there).

[rolandlo]

Thanks for the input.

As far as the SetSquare is concerned, I think none of its related classes should be owned by a PageSomething class, and the SetSquare classes should not refer to any specific page (there are bugs on the current master branch if you try to drag the setsquare to a different page and use it there).

I think the setsquare must always refer to a specific page. What should happen otherwise, if you change the page layout (e.g. change from a 2-row setup to a 3-row setup), if the position of the setsquare is not relative to a specific page? Currently when you activate the setsquare, it is activated on a specific page and should only be used there. If you need it on a different page, you should deactivate the setsquare and activate it on the other page.

[rolandlo]

I managed to fix the first rendering bug (where the setsquare was invisible until it was manipulated). So everything works again, just the setsquare is overpainted on the next page. Thinking about that issue a little more, it seems to be only a question about rendering priority, not about ownership. Maybe overlays could get a rendering priority assigned, so that overlays of lower priority would only get painted after all overlays (of all pages) of higher priority were painted.

[bhennion]

Hey @rolandlo. Could you rebase this on latest master? I don't think this conflicts with #4158 or #4159, so it could be directly based on master. This would help the reviewing.
A couple of changes happened in #4137 before it got merged, so the rebasing on master may not be trivial.

[rolandlo]

Hey @rolandlo. Could you rebase this on latest master? I don't think this conflicts with #4158 or #4159, so it could be directly based on master. This would help the reviewing. A couple of changes happened in #4137 before it got merged, so the rebasing on master may not be trivial.

Sure, I will rebase on master. I have progressed more and already implemented the compass. Now I know better what the setsquare code and its integration in the code base should look like. It will take some time to split everything into sensible commits and PR's.

[rolandlo]

Hey @rolandlo. Could you rebase this on latest master? I don't think this conflicts with #4158 or #4159, so it could be directly based on master. This would help the reviewing. A couple of changes happened in #4137 before it got merged, so the rebasing on master may not be trivial.

The rebasing is finished. If you could take a look that would be great. I have updated the description on the top to help through the most important commits.

[rolandlo]

Currently there are regular pointers

    GeometryTool* geometryTool = nullptr;
    GeometryToolController* geometryToolController = nullptr;
    GeometryToolHandler* geometryToolHandler = nullptr;

in XournalView.h. These should probably be unique_ptr. I tried to create unique_ptrs for the derived classes and std::move them to get unique_ptrs for the base classes (GeometryTool classes), but got a crash when executing. Maybe I forgot to change something elsewhere.

[bhennion]

Below are a couple of details I noticed while skimming through the code. I have not look at every line of code.

I do have a more important structural remark: I feel like the very existence of gui/inputdevices/GeometryToolHandler (and derived classes) is problematic.

The content of gui/inputdevices/ should be an abstraction layer and no tool implementation should ever end up in this directory.
Of course, you created those classes because you need them: the input system is inadequate and does not handle multitouch as we'd need to avoid tool-specific code.
I'm not familiar enough with the input system to come up with a better structured solution at this point. I need to think.

(Also, we have to keep in mind that GTK4 sees a lot of changes in the input mechnisms, so porting to GTK4 will someday require extensive work on the input system.)

[rolandlo]

I do have a more important structural remark: I feel like the very existence of gui/inputdevices/GeometryToolHandler (and derived classes) is problematic.

The content of gui/inputdevices/ should be an abstraction layer and no tool implementation should ever end up in this directory. Of course, you created those classes because you need them: the input system is inadequate and does not handle multitouch as we'd need to avoid tool-specific code. I'm not familiar enough with the input system to come up with a better structured solution at this point. I need to think.

I agree that those files look misplaced. Maybe @LittleHuba has an opinion about how to deal with it.

(Also, we have to keep in mind that GTK4 sees a lot of changes in the input mechnisms, so porting to GTK4 will someday require extensive work on the input system.)

We should probably get acquainted with the input mechanism in GTK4 as soon as possible and try to figure out what changes will be required when porting.

[LittleHuba]

Historically the input system was only a fix to get rid of the old c-style input handling. In general I think it is simply impossible to get rid of all tool specific code in the input handling. Thus, for me the goal would be to at least separate the unspecific handling very strictly from the tool specific handling to get something like a layered system. How that would be possible, I honestly don't know yet.
As far as I know, GTK4 goes more in line with other frameworks where each widget does it's own parsing of input and you have a bubbling down behavior. But I never looked deeper into that.

[rolandlo]

Rebased and fixed merge conflict.

[rolandlo]

/azp run create-installers

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[rolandlo]

Cleaned up the commit history a bit (with no functional changes) and rebased on current master.
Merging in 48 hours if no objections are raised.