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

Automatic DateTime Updates are Triggered Twice When File Doesn't Have Trailing Newline #221

Closed
farmerau opened this issue Jan 5, 2022 · 10 comments
Projects
Milestone

Comments

@farmerau
Copy link
Contributor

farmerau commented Jan 5, 2022

Describe the bug
When frontMatter.content.autoUpdateDate is enabled and editing a document that doesn't have a trailing newline, the automatic date updates are triggered twice. This doesn't happen when the file does have a trailing new line. For example, after it updates twice (the newline is added for you), subsequent updates happen only once.

To Reproduce
Steps to reproduce the behavior:

  1. Ensure frontMatter.content.autoUpdateDate is enabled.
  2. Try to update a markdown (or MDX) file and wait for the debounce to trigger the autoupdate.
  3. Observe that the lastmod field is updated twice.

Expected behavior
I'd expect the lastmod field to be updated only once.

Screenshots
A gif which demonstrates the behavior:
example

VS Code Version

Version: 1.63.2 (Universal)
Commit: 899d46d82c4c95423fb7e10e68eba52050e30ba3
Date: 2021-12-15T09:37:28.172Z (2 wks ago)
Electron: 13.5.2
Chromium: 91.0.4472.164
Node.js: 14.16.0
V8: 9.1.269.39-electron.0
OS: Darwin arm64 21.2.0

Extension Version
v5.9.0

Additional context
This is likely due to the autoupdate logic defined here:

public static async autoUpdate(fileChanges: vscode.TextDocumentChangeEvent) {
const txtChanges = fileChanges.contentChanges.map(c => c.text);
const editor = vscode.window.activeTextEditor;
if (txtChanges.length > 0 && editor && ArticleHelper.isMarkdownFile()) {
const autoUpdate = Settings.get(SETTING_AUTO_UPDATE_DATE);
if (autoUpdate) {
const article = ArticleHelper.getFrontMatter(editor);
if (!article) {
return;
}
if (article.content === Article.prevContent) {
return;
}
Article.prevContent = article.content;
Article.setLastModifiedDate();
}
}
}

One way to solve this might be to adjust the change detection logic, but this may introduce new bugs.

Another possible solution would be to do away with change detection entirely-- moving away from vscode.workspace.onDidChangeTextDocument to onWillSaveTextDocument: https://code.visualstudio.com/api/references/vscode-api#3591 . This would move auto-updates to the pre-save lifecycle which would prevent the extension from updating the lastmod timestamp while the user is actively editing the document.

@estruyf
Copy link
Owner

estruyf commented Jan 5, 2022

The trailing line is something I think VS Code adds for you in this case, so it will indeed trigger an update when you added your content, and VS Code adds the new line, which triggers it again.

onWillSaveTextDocument means that the user has to save the file, but could be a solution. As I think the new line will be added by VS Code just before saving it.

Another way would be to disable the new line setting for markdown files.

@farmerau
Copy link
Contributor Author

farmerau commented Jan 6, 2022

Yeah, the newline being appended by VS Code makes total sense.

My concern, albeit minor, was that the newline being appended is being triggered by the extension, even if indirectly. Any change to the document triggers another onDidChangeTextDocument (including the frontMatter one), which I believe is causing the change detection logic to detect another change, triggering another update.

If this isn't a concern, since it's certainly an edge case, I'm happy to close the issue.

It's worth some thought, though, about whether or not onDidChangeTextDocument is a solid event for the intent behind the feature. I ask this (very not sarcastically 😁 ): Does updating the field any time prior to save provide any value?

@estruyf
Copy link
Owner

estruyf commented Jan 6, 2022

@farmerau we could certainly try to update it to the onWillSaveTextDocument, I'm very much open to changing anything that doesn't make sense or can be improved in any way.

I do not use this functionality myself, so never ran against this issue, but I can see what could go wrong if there are multiple extensions listening to changes. Although, this might be the same for the save event.

@farmerau
Copy link
Contributor Author

farmerau commented Jan 6, 2022

Another related consequence that I see: If I type a sentence out and save it immediately (a common behavior of mine) the lastmod property is not updated:
on-save

@farmerau
Copy link
Contributor Author

farmerau commented Jan 6, 2022

@farmerau we could certainly try to update it to the onWillSaveTextDocument, I'm very much open to changing anything that doesn't make sense or can be improved in any way.

I do not use this functionality myself, so never ran against this issue, but I can see what could go wrong if there are multiple extensions listening to changes. Although, this might be the same for the save event.

Sure, that makes sense! I don't think the onWillSaveTextDocument can cause a loop in the same way but I can think of ways that the behavior could conflict with other extensions like Prettier (which runs first on the event, for example)

@estruyf
Copy link
Owner

estruyf commented Jan 6, 2022

Let us give it a try!

@farmerau
Copy link
Contributor Author

farmerau commented Jan 6, 2022

Cheers! I'll get a PR out!

@farmerau
Copy link
Contributor Author

farmerau commented Jan 6, 2022

After doing some debugging, I learned that the newline is actually coming from gray-matter:

  • Calling document.GetText() returns the text without a newline character
  • Evaluating newMarkdown (value returned from this.stringifyFrontMatter) includes a newline character at the end.

This would mean that there are no controls from VS Code that could prevent the newline from being appended, meaning that there would be no way for a user to prevent the double updates in the current state.

I've opened a PR to run on save instead of on-change.

@estruyf
Copy link
Owner

estruyf commented Jan 7, 2022

I've merged your PR @farmerau, thanks again for the help on this one! 🙏

Included a check for the gray-matter new line as well. When the content does not contain a newline before the stringify function, it will remove it when included. Combined with your change, I think this will complete the issue.

@estruyf estruyf added this to the 5.10.0 milestone Jan 7, 2022
@estruyf estruyf added this to In progress in v5.10.0 Jan 7, 2022
@estruyf estruyf moved this from In progress to Done in v5.10.0 Jan 10, 2022
@estruyf estruyf mentioned this issue Jan 10, 2022
@estruyf
Copy link
Owner

estruyf commented Jan 10, 2022

Released in v5.10.0

@estruyf estruyf closed this as completed Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

2 participants