View refactor

[bhennion]

This ticket is for discussing View refactor related questions. See also #1795 and #1813 for a previous attempt.

The previous PR's #3503 #3985 #4001 #3969 and #3990 created view classes for each element, layer and background. The next step is the refactoring of the PageView class (for now, the class XojPageView in gui/PageView) and split it into a view/PageView and a control/PageControl.

The goal is to follow the general MVC scheme as layed down by @LittleHuba in
#3962 (comment).

This raises questions:

1. To be able to redistribute the functions properly, I need something clarified: in the event that we someday implement the possiblity to have several displays of the same document, should there be one PageControl instance per display, (say 2 PageView's and 2 PageControl), or a single PageControl?

2. How should the current tools be handled: for now, the function XojPageView::paintPageSync does two things:

   1. It paints the page (easy: goes to PageView)
   2. It paints whatever the current tool is doing, via code blocks like
      

      xournalpp/src/core/gui/PageView.cpp

      Lines 893 to 897 in 498e6c1

      	if (this->inputHandler) {
      	cairo_save(cr);
      	this->inputHandler->draw(cr);
      	cairo_restore(cr);
      	}

   I'm not sure how the second part should be handled. The tool handlers are definitely on the control side.
   This probably means that the tool handlers themselves need to be split into a model-view-controller pattern, right?

[Technius]

should there be one PageControl instance per display, (say 2 PageView's and 2 PageControl), or a single PageControl?

I think there should be one PageControl for the whole document, as it's basically a state machine that updates the document state upon receiving user input (by the input handlers making method calls on the PageControl). However, we should be able to support multiple views of a document.

I'm not sure how the second part should be handled. The tool handlers are definitely on the control side.
This probably means that the tool handlers themselves need to be split into a model-view-controller pattern, right?

I agree with this proposal. The way I see it:

• We will want a "PageControl" that is used for updating page state, which should only consist of "valid" elements.
• When using a tool, the user is manipulating a "partial/incomplete" element, with the hopes of creating/editing/removing a valid element.
  • This is inconsistent with how the page model works, since a page should only contain "valid" elements at all times. Therefore, we will want to separate the tool handler control logic from the page control logic.
  • A tool handler is like a state machine (e.g., model + controller). For example, the spline tool needs to keep track of the set of points in the spline and the parameters of the curves between the points. The user can update the tool handler state via user input.
  • We also want to render (e.g., view) any partial/incomplete elements as well as visual aids (e.g., as in spline or setsquare).
  • We have some notion of "initializing" and "finalizing" a tool handler. When a tool handler is finalized, it can interact with PageControl to transfer/replace/create a valid element.
• By separating the "valid" elements (page view) and partial/incomplete/invalid elements (tool handler & others), this enables us to decouple the rendering logic from the GUI and add features like picture-in-picture page previews.

[bhennion]

• We will want a "PageControl" that is used for updating page state, which should only consist of "valid" elements.

If we proceed as you suggest (if I understand correctly: a ToolHandler - owned by Control or the current gui/XournalView - handles the events and drawing of the elements under edition, the rest of the rendering goes into view/PageView), it is not clear to me what control/PageControl is used for then. It seems it'll end up being a 1-1 inerface to the model. Why not directly interact with the model then?

The key point is possibly: who dispatches modification notifications (to the views)? I assume that in @Technius' plan, that's what control/PageControl is for. Right? This would be like the current control/layers/LayerController.

On the other hand, @LittleHuba suggests in #3962 (comment) that the model itself would emit the notifications (like model/XojPage in the current base code).

I'm okay with both versions, but we need to pick one and stick to it.

Another thing bothers me: if and when we allow for a second view of the document, this second view should include the current edition (e.g. for a lecture, sharing the second view on a screen or online). This seems to imply that view/PageView should point to existing Toolhandler(s). I don't like that.

[LittleHuba]

The thought of having the model emit the notifications is that you can subscribe multiple views that will all show the same state.

In this case the model would need to also reflect elements that are currently edited. So the tool handler does not draw anything itself but just updates the model.

[bhennion]

I've looked at this a bit more. It seems that before being actually able to split XojPageView into a view and a controller, a big amount of work on the tool handlers is required. For now, I failed to come up with a satisfactory scheme that handles the various behaviours listed below.
(To understand properly, one needs to know: in our code base repaint means the page buffer is blitted and the tools are rendered on top of that, while rerender means the page buffer is updated, then blitted, and the tools are rendered on top of that).

1. Creative tool handlers (basically, the classes derived from InputHandler -- StrokeHandler and the like):
   • No direct modification of the document until the stroke is finished --> only need PageView::repaintRange
2. EraserHandler: (the only tool handle without a draw/paint function :-)
   • No direct modification of the document (except the ErasableStroke being referenced in the tree), but the page buffer is not good (because it contains the stroke before erasing) -> local rerendering
3. VerticalToolHandler:
   • Adopts elements upon creation (or upon key modifier being pressed) = modify the document directly -> rerendering
   • The rest of the time, only moves up and down a buffer -> repaint (for now rerendering, but that's a waste of flops)
4. TextEditor:
   • Creates a new Text element: no modification of the document until the text is finished (the user clicks elsewhere) -> repaint (the current version rerenders, which is a waste of flops)
   • Modify a preexisting Text element: first adopt the element (rerender - currently the entire page. It should be just a small area), then edit it as before (repaint).
5. Search/PdfElemSelection
   • Purely visual -> repaint
6. Selection (the tools to select something (shape or rectangle), not the selection edition tool (resize, rotate and so on))
   • Purely visual up until the end -> repaint.
7. EditSelection: This is a mess... This class depends the most on XojPageView being a mixed view/controller class.
   • First, this is not owned by a XojPageView, but by the Xournal++ widget itself (because the selection can be moved from one page to the other).
   • Upon creation, it adopts elements, thus triggering a page rerendering
   • The rest of the time, it calls for repaints

Ideal solution: Every tool handler is split into a model/view/controller. The model would every time be protected by a mutex, to fix #3579.

Realistic short term solution: Add a fireRepaint to PageHandler, to tell the difference between rerenderings (triggered e.g. by calls to PageHandler::firePageChanged)

The case of EditSelection is the most complicated, because it is not bound to a specific page. Short term solution: make it rely on the XournalView (rather than the XojPageView). This allows to split PageView, but postpones the issue.