Some const-correctness in SaveHandler #5263
Conversation
* SaveHandler now only relies on a const Document*
* Remaining issue: the background images are still edited (to
add a clone index and set the filepath up in attached mode)
Some const-correctness elsewhere...
| /* | ||
| * Because BackgroundImage is basically a wrapped pointer, the following lines actually modify | ||
| * *(p->getBackgroundImage().content) and thus the Document. | ||
| * TODO Find a better way | ||
| */ | ||
| backgroundImages.back().setFilepath(filename); | ||
| backgroundImages.back().setCloneId(id); |
There was a problem hiding this comment.
This is still a bit problematic: the SaveHandler actually modifies a BackgroundImage to link it to a newly saved file. Because BackgroundImage is a simple pointer wrapper, this mean part of the data which constitutes the Document is modified, even though the Document instance is const.
This is fine in terms of c++, but will mess up any shared_mutex plan: we'd like the SaveHandler to only own a shared_lock (as opposed to a unique_lock), so that other read-only tasks (e.g. rendering) can still happen simultaniously.
Any suggestion would be welcome here.
There was a problem hiding this comment.
I fear to lock the whole document at once for a single change. This will make the hole application single threaded and will also block most of the UI at each operation. Instead, I would like to have many independent subgroups of the document, which can be locked and modified simultaneously.
Another alternative would be, to remove the locking completely and to replace any changeable subpart by an atomic<shared_ptr> to constant (immutable) Data. The data can then be replaced with a read-modify-write operation.
There was a problem hiding this comment.
I fear to lock the whole document at once for a single change. This will make the hole application single threaded and will also block most of the UI at each operation. Instead, I would like to have many independent subgroups of the document, which can be locked and modified simultaneously.
I agree with that. I'd say one mutex per page (or maybe per layer), and one for the document's structure (i.e. the pages vector) is a decent balance. Of course, if tiling comes into play at some point, this'll have to be rethought.
Another alternative would be, to remove the locking completely and to replace any changeable subpart by an atomic<shared_ptr> to constant (immutable) Data. The data can then be replaced with a read-modify-write operation.
This would be very expensive. I don't quite see how to clone the content of a layer in an atomic way either.
There was a problem hiding this comment.
Another alternative would be, to remove the locking completely and to replace any changeable subpart by an atomic<shared_ptr> to constant (immutable) Data. The data can then be replaced with a read-modify-write operation.
This would be very expensive. I don't quite see how to clone the content of a layer in an atomic way either.
It hardly depends on the data structure. Every change must be kept in the Undo Redo Handler, therefore it is copied anyway. When you now store all the document state in a tree of shared_ptr nodes, you would just reference the unit you want to change in the undo-redo handler and then replace the shared_ptr with a copy of it and its data. The replacement of the shared_ptr can be done atomically. We only need a solution for the ABA problem, but that's solvable by introducing a change index.
| template <typename container_type> | ||
| class PointerContainerView { |
There was a problem hiding this comment.
Should be namespaced, at least with xoj::.
Also, isn't this just like std::subrange?
And I would use the container's iterator when possible, instead of wrapping it. That will retain the properties of the original iterators.
In order to cleanup the race conditions and add a shared_mutex for the Document (like in this attempt #3586), it'll be important to know when the document is accessed as read-only, or as read-write. All in all, we need better const-correctness (const Document gives const Page gives const Layer gives const Element).
This PR handle the use of Document* in SaveHandler to make it rely on a
const Document*.