Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix (Whiteboards): History issues #8788

Merged
merged 9 commits into from Mar 13, 2023
Merged

Conversation

sprocketc
Copy link
Collaborator

@sprocketc sprocketc commented Mar 8, 2023

Is some cases we had to persist a new state by replacing the last history entry, instead of pushing a new one.
The related block was removed here https://github.com/logseq/logseq/pull/7786/files#diff-f8d520c32e2ccdf12b14a130b819d3ecbc21284ceabab9ac1ed48d2cab65f272L68-L70

As a result, calling app.persist(true), disregards the boolean and always pushes a new entry. For example, if you drag and drop a block into the canvas and then undo the action, you will notice that there are intermediate history entries that shouldn't exist. There is a similar problem with tweet shapes and iframes.

I think we can achieve a similar result by setting :whiteboard/transact? to false and by using the existing :compute-new-refs? that seems to swap the data of the last undo entry.

Fixed another bug (d3cd17e) that didn't allow us to undo a block/page creation by using the portal tool or by double clicking. The issue was accidentally fixed by #7786, because replace stopped working. The problem was that the first persist was called with replace true (so not undoable). The next one was called with replace false, but the portal element was already part of the state.

Also fixed persisting the collapsed state of portals and added tests for the mentioned cases.

Some notes about history

  • I think we need to eliminate both replace and pause/resume on whiteboards. Since we have to explicitly call persist, we should be able to avoid those quirks by persisting only when needed.
  • Having a shared history stack for all pages under the same repo, can lead to perceived data loss. This can be overstated on Whiteboards, because the canvas feels like a completely different context. You might add something to a whiteboard, navigate to a different page, make some changes, spam the undo and loose the changes on the whiteboard without noticing. Ideally, we should have a dedicated history per page. Clearing the stack when we navigate could help, but it could also end up being annoying.
  • Persisting to edn on undo/redo seems to behave inconsistently.

@sprocketc sprocketc marked this pull request as draft March 8, 2023 13:03
@sprocketc sprocketc changed the title Fix (Whiteboards): History replace [wip] Fix (Whiteboards): History replace Mar 8, 2023
@sprocketc sprocketc changed the title [wip] Fix (Whiteboards): History replace [wip] Fix (Whiteboards): History issues Mar 8, 2023
await page.click('#tl-zoom-out')
await page.waitForTimeout(1500) // wait for the zoom animation to finish
await page.keyboard.press(`${modKey}+-`)
await page.waitForTimeout(1500) // wait for the zoom animation to finish
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this PR, but this test was sporadically failing.

@sprocketc sprocketc changed the title [wip] Fix (Whiteboards): History issues Fix (Whiteboards): History issues Mar 9, 2023
@sprocketc sprocketc marked this pull request as ready for review March 9, 2023 10:29
Copy link
Contributor

@tiensonqin tiensonqin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as described! 👍

@tiensonqin
Copy link
Contributor

I think we need to eliminate both replace and pause/resume on whiteboards. Since we have to explicitly call persist, we should be able to avoid those quirks by persisting only when needed.

It'll be great if we can remove both replace and pause/resume.

You might add something to a whiteboard, navigate to a different page, make some changes, spam the undo and loose the changes on the whiteboard without noticing.

Outliner pages share this problem too, we need a way to prevent the undo/redo if the history is not related to the current operating pages.

Persisting to edn on undo/redo seems to behave inconsistently.

Can you create an issue on it?

@sprocketc
Copy link
Collaborator Author

Outliner pages share this problem too, we need a way to prevent the undo/redo if the history is not related to the current operating pages.

@tiensonqin Working on a fix here #8816

Can you create an issue on it?

Will do, once I figure out how to reproduce this consistently.

@sprocketc sprocketc mentioned this pull request Mar 13, 2023
13 tasks
Copy link
Collaborator

@logseq-cldwalker logseq-cldwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sprocketc Thanks for the fixes with tests! 🐛 🔥 Confirmed tests behave as desired

@logseq-cldwalker logseq-cldwalker merged commit 537b715 into master Mar 13, 2023
@logseq-cldwalker logseq-cldwalker deleted the fix/history-replace branch March 13, 2023 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants