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(editor): Fix blocks losing references and disappearing from references panel #9389

Merged
merged 2 commits into from May 16, 2023

Conversation

logseq-cldwalker
Copy link
Collaborator

@logseq-cldwalker logseq-cldwalker commented May 13, 2023

This PR fixes #9365 which was introduced in #9254. This brings back the previous behavior where the current block always has it's :block/path-refs transacted as noticed in #9254 (comment) . It seems this is needed. This PR fixes this by no longer having :block/path-refs as a retracted attribute. To QA, you can reproduce with the first minute of the video in #9365. You can also QA with:

  • Create a block foo #bar
  • Run show block data command on the block and note its :block/path-refs
  • Change block to fooz #bar
  • Rerun show block data command on the block and its :block/path-refs should not change

I'll see if I can do a test for this on Monday

@logseq-cldwalker logseq-cldwalker changed the title Fix: Fix child blocks with disappearing path refs Fix: Current block losing path-refs if refs don't change May 13, 2023
Copy link
Collaborator

@cnrpman cnrpman left a comment

Choose a reason for hiding this comment

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

It looks removing a "false optimization"?
LGTM

@logseq-cldwalker
Copy link
Collaborator Author

logseq-cldwalker commented May 15, 2023

It looks removing a "false optimization"?

@cnrpman More like a false fix. refs-changed? was a binding that was always evaluating to true. In #9254, we tried to fix it. In fixing it we introduced the current bug. This PR removes the binding since it seems the current block was dependent on it always being true

@logseq-cldwalker
Copy link
Collaborator Author

@tiensonqin I found the reason we end up needing to always transact :block/path-refs is because outliner-core/save-block! is always retracting the block's :block/path-refs attribute. I could make the fix there but I'm not confident that it wouldn't break something else

@tiensonqin
Copy link
Contributor

@logseq-cldwalker Yes, we can remove :block/path-refs from the retracted block attributes.
Because new-refs [here](https://github.com/logseq/logseq/pull/9389/files#diff-016f9b839a0f3c4d8b9e3089f9cb90c9424e21a793d6b395343f7f2697001263L78, to handle the case that a reference was deleted from a block) is always not empty, with at least the block's page.

outliner-core/save-block and save-block-inner! were retracting
block/path-refs aggressively, especially for cases when no path-refs
had changed
Also introduce helper fn for common db fixture setup
@logseq-cldwalker
Copy link
Collaborator Author

@tiensonqin this is ready for another look

@@ -213,3 +218,24 @@
"No page search within backticks"))
;; Reset state
(state/set-editor-action! nil))

Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

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.

QAed and works as described!

@tiensonqin tiensonqin merged commit decbc12 into master May 16, 2023
12 checks passed
@tiensonqin tiensonqin deleted the fix/disappearing-path-refs branch May 16, 2023 03:35
@logseq-cldwalker logseq-cldwalker changed the title Fix: Current block losing path-refs if refs don't change Fix(editor): Fix blocks losing references and disappearing from references panel May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants