fix: Trim trailing white-space from headings #1004
Conversation
fix: Use header cache instead of section cache to find headings for shorter loop and greater code readability fix: Only read and parse the file for tasks if there are listItems in Obsidian cache refactor: Match call signatures of getSection and getPrecedingHeader for greater code readability
|
Thanks @AnnaKornfeldSimpson ! The code is definitely more readable. I'll do the review later. First I wanted to test the behaviour of the new code. It's labelled as a fix, which would be a change in behaviour, but the code change looks to me like it's intending to be pure refactor - so no change in behaviour. So I though I would see if the behaviour has changed, in my approx 3,600 file vault. How I testedI re-instated the old code, then ran old and new, and if they gave a difference in behaviour, I logged the difference. diff --git a/src/Cache.ts b/src/Cache.ts
index bf492b9..7fe93e0 100644
--- a/src/Cache.ts
+++ b/src/Cache.ts
@@ -288,15 +288,30 @@ export class Cache {
}
const line = fileLines[listItem.position.start.line];
+
+ const precedingHeaderOld = Cache.getPrecedingHeaderOld({
+ lineNumberTask: listItem.position.start.line,
+ sections: fileCache.sections,
+ fileLines,
+ });
+
+ const precedingHeaderNew = Cache.getPrecedingHeader(
+ listItem.position.start.line,
+ fileCache.headings,
+ );
+
+ if (precedingHeaderNew !== precedingHeaderOld) {
+ console.log(
+ `Header mismatch:\nOld: "${precedingHeaderOld}"\nNew: "${precedingHeaderNew}"`,
+ );
+ }
+
const task = Task.fromLine({
line,
path: file.path,
sectionStart: currentSection.position.start.line,
sectionIndex,
- precedingHeader: Cache.getPrecedingHeader(
- listItem.position.start.line,
- fileCache.headings,
- ),
+ precedingHeader: precedingHeaderNew,
});
if (task !== null) {
@@ -347,4 +362,45 @@ export class Cache {
}
return precedingHeader;
}
+
+ private static getPrecedingHeaderOld({
+ lineNumberTask,
+ sections,
+ fileLines,
+ }: {
+ lineNumberTask: number;
+ sections: SectionCache[] | undefined;
+ fileLines: string[];
+ }): string | null {
+ if (sections === undefined) {
+ return null;
+ }
+
+ let precedingHeaderSection: SectionCache | undefined;
+ for (const section of sections) {
+ if (section.type === 'heading') {
+ if (section.position.start.line > lineNumberTask) {
+ // Break out of the loop as the last header was the preceding one.
+ break;
+ }
+ precedingHeaderSection = section;
+ }
+ }
+ if (precedingHeaderSection === undefined) {
+ return null;
+ }
+
+ const lineNumberPrecedingHeader =
+ precedingHeaderSection.position.start.line;
+
+ const linePrecedingHeader = fileLines[lineNumberPrecedingHeader];
+
+ const headerRegex = /^#+ +(.*)/u;
+ const headerMatch = linePrecedingHeader.match(headerRegex);
+ if (headerMatch === null) {
+ return null;
+ } else {
+ return headerMatch[1];
+ }
+ }
}ResultsThese are the 4 differences that were logged:
Questions
|
|
Fascinating! I will take a closer look. We already know that Obsidian manipulates headers to remove punctuation when making the backlinkable version, so perhaps they normalize away trailing spaces as well. I will investigate. |
Thank you. If it does throw up any other issues, let's explore how to divide them up in to separate issues/PRs, to keep the scope of this one to just the changes it already makes... |
|
Summary: Everything still works fine with backlinks either way. This does change the result of "group by heading" if the only difference between two headings was trailing space(s). Small tests. # @Computer
- [ ] task1 no-header-spaceFile2: # @Computer
- [ ] task2 header-spaceFile3: ```tasks
description includes header-space
group by heading
```Results:
|
|
Am slightly struggling to visualise the change in behaviour. No hurry, but would there be any chance of screenshots of the before-and-after behaviour with that example please? |
|
Maybe this will be easier to visualize? Same files and query as above. Before:
|
|
OK Thanks. I'm sorry I don't have time to test this right now, but I wanted to capture a thought while I remember... Will test when I have time.
I'll be interested in whether the stripping out of any punctuation might lead to unexpected missing results in For example, if a heading has a question mark in, and the user includes that |
|
The heading that is saved by Tasks has the punctuation. The punctuation removal is only for making links. (Check your test cases above: lots of punctuation preserved!) |
Now I am really confused!! I thought I/we were talking about how the new implementation of 'get me previous heading, via Obsidian API' in this PR differs from the previous behaviour? And the tests don't call that code, because I don't know how to call the Obsidian API in tests? Anyway, not to worry, I will do some empirical testing some time soon... |
|
Sorry, I should not write comments when not entirely awake. It also has implications for #909 where we are trying to understand Obsidian's behavior with heading / heading-link normalization and what the correct construction of backlinks is. Backlinks do change in this PR to remove trailing spaces - this makes them closer to what Obsidian would auto-generate - both versions are parsed correctly by Obsidian so there is no behavior change there. |
There was a problem hiding this comment.
Thank you very much indeed for these changes. I learned stuff reading them!
The thing that is slightly holding me off merging now is I am struggling to word the 1-line summary of the PR and the merge commit.
As far as I understand it, the changes are:
- perf: slightly speed up indexing tasks
- behaviour change: remove trailing spaces from headings
I can't call the latter one a refactor, even though it simplifies code, as we found it does change behaviour. Refactoring is very strictly about not changing the visible behaviour of the code.
I can't work out to fit both of those things in to one line, for the release notes.
On reflection, I think the behaviour change is the most relevant change for users, so I will reword the PR name, and describe that as a fix.
claremacrae
changed the title
…oup#1004) fix: Use header cache instead of section cache to find headings for shorter loop and greater code readability. (This was mainly a refactoring, which massively simplified the code. However, during testing, we found that the new code strips any whitespace from the end of headings, which if there are duplicate headings differing only in trailing whitespace, would merge the seemingly identical headings in 'group by heading'.) refactor: Only read and parse the file for tasks if there are listItems in Obsidian cache refactor: Match call signatures of getSection and getPrecedingHeader for greater code readability
Description
fix: Use header cache instead of section cache to find headings for shorter loop and greater code readability
fix: Only read and parse the file for tasks if there are listItems in Obsidian cache
refactor: Match arguments of getSection and getPrecedingHeader for greater code readability
Motivation and Context
This PR contains the re-written
getPrecedingHeaderthat I removed from my callout and blockquote PR. It uses the Obsidian-provided heading cache to both shorten the loop (perf) and simplify the extraction of the header text (readability, maintenance).Also contains a couple other small fixes:
Fixes #959
Readability improvement to have
getSectionargs matchgetPrecedingHeaderargs. Note: If there is some reason that I do not know of to keep using the lengthier "pass all args as JS object then add type annotations separately" style, I can swap to using that style for both instead, but I find this style with fewer lines much easier to read.How has this been tested?
There are no automated tests for this file.
Instead, smoke tested on Windows in up-to-date Obsidian.
Types of changes
Changes visible to users:
fix- non-breaking change which fixes an issue) - note: should not really be visible to users except as perhaps a tiny perf improvement in some large filesfeat- non-breaking change which adds functionality)feat!!orfix!!- fix or feature that would cause existing functionality to not work as expected)docs- improvements to any documentation content)vault- improvements to the Tasks-Demo sample vault)Internal changes:
refactor- non-breaking change which only improves the design or structure of existing code, and making no changes to its external behaviour)test- additions and improvements to unit tests and the smoke tests)chore- examples include GitHub Actions, issue templates)Checklist
yarn run lint.Terms