refactor history to be able to record after a change

[flxzt]

This PR is refactoring the history and record() calls to be made after a change on the document happens. This is more elegant and was a design flaw in the original implementation. Previously, there was the issue that possible changes had to be anticipated before they actually happened, which caused some redundant record calls in certain conditions.

This also improves the history when using the typewriter, state after typing characters that are non-whitespace is now appended to the latest history entry, and only text that contains whitespace results in new record calls.

Remaining issues that need to be fixed:

• the selected state of strokes when undoing is inconsistent. It might be better to simply unselect after every undo/redo, or even remove the selection components from the store entirely so that this state only lives in the selector pen and is not part of the recorded history state. fixed by removing selected state entirely from the history.

the work of this PR is needed for #672 so that in-progress strokes can be removed without them being recorded into the history

[Kneemund]

When opening files, the first history entry is not updated. This means that undoing until you reach the first entry clears the entire document.

Calling update_latest_history_entry when loading a snapshot fixes this, but I'm not sure if it covers everything/is the best way to handle this.

    pub fn load_snapshot(&mut self, snapshot: EngineSnapshot) -> WidgetFlags {
        self.document = snapshot.document;
        self.store.import_from_snapshot(&snapshot);

        let mut widget_flags = self.current_pen_update_state();
        widget_flags.merge(self.update_latest_history_entry(Instant::now()));

        widget_flags
    }

If this seems like a reasonable fix to you, I could push it (or you can try & potentially push it yourself). I haven't bisected the exact commit, but it seems likely that this bug was introduced in here :P

[flxzt]

Oh thanks, I missed handling this. The cleanest way would probably be adding a initial_state argument in store.clear_history() and explicitely calling this with initial state in store.import_from_snapshot() instead of it being called indirectly through clear()

But feel free to push, especially bug-fixes!

[Kneemund]

Alright. Is there a situation where we don't want the initial state to be the current state though? Conceptionally, I think it would make the most sense to always set it to store.create_history_entry() instead of the default entry. That also doesn't change the behavior of store.clear().

(This would of course still require another explicit call of clear_history() at the end of import_from_snapshot().)

[flxzt]

Yeah that's true. That would work as well, it's just a bit less explicit and one has to be a little bit more careful when using clear_history() - it is not immediately obvious for a caller that the order in which the history and the current state is cleared/overwritten matters.

[Kneemund]

Fair enough, I've implemented it like you suggested :)

[flxzt]

okay :) thanks for noticing and fixing this, I don't think I would have noticed until next release