Correct history management

[barklund]

Feature description

The history currently remembers too many states compared to the UX. We don't want to remember every change in selection nor every page change, but when undoing, the selection and page change associated with the currently "undone" state should be recreated.

Please see UX for details.

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• Selecting an element or changing page should not count as an undo state in itself in the history.
• Selection and current page does however need to be part of every state in the undo history.

Implementation brief

• It can probably be done simply by remembering current page and selection in refs, but only actually updating the history state on changes to the pages array.

Something like this might work (compare with existing):

function useHistoryEntry( {
    currentPageIndex,
    pages,
    selectedElementIds,
} ) {
    const { actions: { appendToHistory } } = useHistory();
    const currentPageIndexRef = useRef();
    const selectedElementIdsRef = useRef();
    useEffect( () => {
        currentPageIndexRef.current = currentPageIndex;
        selectedElementIdsRef.current = selectedElementIds;
    }, [ currentPageIndex, selectedElementIds ] );
    useEffect( () => {
        appendToHistory( {
            currentPageIndex,
            pages,
            selectedElementIds,
        } );
    }, [ appendToHistory, currentPageIndex, pages, selectedElementIds ] );
    useEffect( () => {
        appendToHistory( {
            currentPageIndex: currentPageIndexRef.current,
            selectedElementIds: selectedElementIdsRef.current,
            pages,
        } );
    }, [ appendToHistory, pages ] );
}

Then only changes to pages will actually create a new entry in the history, but any changes to current page or selection will be remembered and persisted along with those major page changes.

QA testing instructions

Demo

Changelog entry

[barklund]

IB Review please, @dvoytenko, @pbakaus, @miina, @swissspidy, @spacedmonkey.

[pbakaus]

This SGTM.

[miina]

If necessary later, we could easily add other general document settings that wouldn't influence the pages to the history, right? Such as general Page advancement (automatic/manual), Story status, visibility, etc. Would it be good to consider those/add an example in the IB right now as well, or is it OK to worry about that later?

[spacedmonkey]

Should a user history persist between sessions? It would be really cool, to be able to undo something a change I made last time I edited a story. This is of course as nice to have. But if the history states are just stored as a JSON object, should be simple enough to save them as post meta when a user pleases the save / update / publish button.

[pbakaus]

Should a user history persist between sessions? It would be really cool, to be able to undo something a change I made last time I edited a story. This is of course as nice to have. But if the history states are just stored as a JSON object, should be simple enough to save them as post meta when a user pleases the save / update / publish button.

That depends on how much the history JSON stack implicitly relies on the current client state. If it's trivial to not depend on the client state, and restore everything with just the JSON object, then I'm all for it. If not, then I'd say it's a P2 feature.

[dvoytenko]

@barklund a bit risky maybe: but what if we rolled both data-model and history into a single composed reducer. And we'd hoist selection provider above it. My initial feeling is that it could simply things a bit, but I'm not sure I'm considering all nuances.

The composed reducer could look something like this:

// getSelection is provided by some context and hoisted above.

function reduceHistory(state, action) {
  switch (action) {
    case 'undo':
      return undo(...);
    case 'redo':
      return redo(...);
    default:
      const { currentStoryState } = state;
      const updatedStoryState = reduceStory(currentStoryState, action);
      if (!Object.is(currentStoryState, updatedStoryState)) {
        pushToHistory(updatedStoryState, getSelection(), ...);
      }
  }
}

Then if we route all property updates, add/remove page/elements, etc via this dispatcher, we'd possibly have a tighter control over model+history.

Is there anything that doesn't fit here very well?

[spacedmonkey]

@dvoytenko We are not using reducers where as we not using redux. We are using context and hooks. Hooks will take care of tracking the changes in variables and write them to state.

I agree that presisting history seems unwise, as it opens a number of issues, with multiple users editing a story. But would about persisting in the browser? What if a user's browser crashes or refreshes?

[barklund]

@dvoytenko We are not using reducers where as we not using redux. We are using context and hooks. Hooks will take care of tracking the changes in variables and write them to state.

Hooks can still use reducers in the useReducer hook. The current history implementation does that.

[dvoytenko]

@dvoytenko We are not using reducers where as we not using redux. We are using context and hooks. Hooks will take care of tracking the changes in variables and write them to state.

Hooks can still use reducers in the useReducer hook. The current history implementation does that.

The useReducer hook is exactly the one I proposed to adopt here. I noted on ampproject/amp-wp#3871 as well - I think we could benefit by combining history and data model within the same useReducer(). Maybe we should discuss this soon?

[swissspidy]

@barklund Can this one be closed?

[barklund]

No, there's still some work to be done regarding not creating history entries for changes to selection and current page. But it's a rather small thing at this point, as we decided to not do history entry compression for now.