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

Feature: Auto save #10

Merged
merged 8 commits into from
Aug 21, 2023
Merged

Feature: Auto save #10

merged 8 commits into from
Aug 21, 2023

Conversation

Acylation
Copy link
Contributor

@Acylation Acylation commented Aug 11, 2023

Closes #2

Changes

  • Add icon and explicitly assign file name to the tab header, bdeb76f
  • Wrapped render KetcherReact in setViewData(), 6da45a3
  • Set and refresh interval for each file, and clear it in onClose(), 17ecb34

Boundary case

If we have multiple tabs holding the same file, on proposed, these tabs will each have an interval, and they will run together, calling save() more frequently then expected.

Notice that, the setViewData() function would only be called by save() if there's any change on file data, and the user may probably can't operate multiple editors at the same time, so here's what we'll get:

Tab A (changed) | Tab Bsave() by Tab A → setViewData() called → Tab A (changed) | Tab B (changed)
Tab A (changed) | Tab Bsave() by Tab B → setViewData() won't be called → Tab A (changed) | Tab B -> save() by Tab A ...

And that's a pseudo-sync maybe.

`setViewData(data, false)` will be called by `save()` when we have more than one split/tab holding the same file.

If we only have one split/tab of the file, then `setViewData()` won't be called.
@yuleicul yuleicul self-requested a review August 11, 2023 09:12
@Acylation
Copy link
Contributor Author

Acylation commented Aug 11, 2023

Sorry for the delay upon my declaration, the API is confusing. Here's the explanation of the second change.

This is also related to the boundary case, that multiple tabs sharing the same file. In this case, when one tab calls save(), the
_check flag in setViewData() of other tabs will be false. So through wrapping render KetcherReact, we can retain the editor, and just refresh the data.

With this specific case I found another problem. If we assign the view data through ketcher.setMolecule(), and the undo operation would just revert what the funcion did.

When we have two tabs, drawing on Tab A and updating Tab B automatically, the undo stack of Tab B behaves weird. For obsidian-ketcher v0.1.2, after refreshing the editor, undo will leave a blank canvas on Tab B, because we re-render the editor, and then assign the data value to the tab. For the proposed change, undo will revert save() operation, and we will get the previous state since we retained the editor and pervious data.

If there's a way to interact with the undo stack of ketcher editor, we can even keep it on the page (init in the constructor of KetView), and just update the data. (Update: code here https://github.com/Acylation/obsidian-ketcher/tree/single-editor-instance)

@Acylation
Copy link
Contributor Author

Acylation commented Aug 11, 2023

Update: code in #11

An alter would be, call this.requestSave() when editor change, could be achieved through epam/ketcher#645

requestSave() triggers a debounced save in 2 seconds from now
https://docs.obsidian.md/Reference/TypeScript+API/TextFileView/requestSave

image
https://github.com/marcusolsson/obsidian-plugin-docs/blob/2b61554e995560036b1789648f46379b76664e3b/docs/tutorials/text-based-file-formats.md#L237-L253

Copy link
Owner

@yuleicul yuleicul left a comment

Choose a reason for hiding this comment

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

Thank you @Acylation for your contribution!

Sorry for the late reply, thank you again, and I got something new from your code. Please feel free to discuss any questions you may have.

@Acylation
Copy link
Contributor Author

In latest updates, I migrated to requestSave() method, and unsubscribe onChange() listener when refreshing or removing the editor.

);
// If clear is set, then it means we're opening a completely different file.
if (_clear) {
this.ketcher?.editor.unsubscribe("change", this.subscriber);
Copy link
Contributor Author

@Acylation Acylation Aug 19, 2023

Choose a reason for hiding this comment

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

Avoid calling unsubscribe() before editor initialize. (Will happen only when we open a new ketcher view)

@yuleicul yuleicul self-requested a review August 21, 2023 06:40
@yuleicul yuleicul merged commit e5533c2 into yuleicul:master Aug 21, 2023
@Acylation Acylation deleted the improve-ketview branch August 21, 2023 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto save
2 participants