feat: Allow additional tasks states and filtering by states #675 #670
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| *.js text eol=lf | ||
| *.ts text eol=lf | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| [CmdletBinding()] | ||
|
There was a problem hiding this comment. Oh, based on the file name, I misunderstood the purpose of this file. I though it was actually testing Obsidian, but it is copying the built plugin in to an Obsidian vault. That's still very useful, but also a candidate for being in a separate PR please (alongside the associated change to There was a problem hiding this comment. I am using it to help me work on this PR. So there will be a chicken egg scenario. Ill make a separate PR and if it goes in first then this will disappear from this PR. |
||
| param ( | ||
| [Parameter(HelpMessage = 'The path to the plugins folder uner the .obsidian directory.')] | ||
| [String] | ||
| $ObsidianPluginRoot = $env:OBSIDIAN_PLUGIN_ROOT, | ||
| [Parameter(HelpMessage = 'The folder name of the plugin to copy the files to.')] | ||
| [String] | ||
| $PluginFolderName = 'obsidian-tasks-plugin' | ||
| ) | ||
|
|
||
| $repoRoot = (Resolve-Path -Path $(git rev-parse --show-toplevel)).Path | ||
|
|
||
| if (-not (Test-Path $ObsidianPluginRoot)) { | ||
| Write-Error "Obsidian plugin root not found: $ObsidianPluginRoot" | ||
| return | ||
| } else { | ||
| Write-Host "Obsidian plugin root found: $ObsidianPluginRoot" | ||
| } | ||
|
|
||
| Push-Location $repoRoot | ||
| Write-Host "Repo root: $repoRoot" | ||
|
|
||
| yarn run build:dev | ||
|
|
||
| if ($?) { | ||
| Write-Output 'Build successful' | ||
|
|
||
| $filesToLink = @('main.js', 'styles.css', 'manifest.json') | ||
|
|
||
| foreach ($file in $filesToLink ) { | ||
| if ((Get-Item "$ObsidianPluginRoot/$PluginFolderName/$file" ).LinkType -ne 'SymbolicLink') { | ||
| Write-Output "Removing $file from plugin folder and linking" | ||
| Remove-Item "$ObsidianPluginRoot/$PluginFolderName/$file" -Force | ||
| New-Item -ItemType SymbolicLink -Path "$ObsidianPluginRoot/$PluginFolderName/$file" -Target "$repoRoot/$file" | ||
| } else { | ||
| (Get-Item "$ObsidianPluginRoot/$PluginFolderName/$file" ).LinkType | ||
| } | ||
| } | ||
|
|
||
| $hasHotReload = Test-Path "$ObsidianPluginRoot/$PluginFolderName/.hotreload" | ||
|
|
||
| if (!$hasHotReload) { | ||
| Write-Output 'Creating hotreload file' | ||
| '' | Set-Content "$ObsidianPluginRoot/$PluginFolderName/.hotreload" | ||
| } | ||
|
|
||
| yarn run dev | ||
|
|
||
| } else { | ||
| Write-Error 'Build failed' | ||
| } | ||
|
|
||
| Pop-Location | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| // import { Editor, MarkdownView, View } from 'obsidian'; | ||
|
There was a problem hiding this comment. I see elsewhere that you've introduced a Feature Toggle, and I do like that idea. I agree that it allows working code to be committed to main, to enable testing before it's ready for prime-time. https://martinfowler.com/articles/feature-toggles.html Sorry if this comment is premature (I did check that the PR is ready for review) but the usual behaviour of feature toggles is to allow experimental code to be turned on or off by changing the toggle. As it's all commented out, I'm unsure how we can test it. Would it be possible for all the UI code to be uncommented out, and then for the code at run-time to only enable the new features if the feature toggle is turned on? There was a problem hiding this comment. Later: I see that the feature toggle is about whether to show a pop-up menu, and this code is about adding a command. I still would suggest the new code be activated, even if it's not called, so that if there are any future refactorings, like renaming symbols, IDEs would update the inactivated code too, preventing the commented-out code later needing to be manually updated for any changes made in between. There was a problem hiding this comment. Coming in a separate PR. I added the feature capability in late in the cycle so it is skipped for the status change as the refactor is to large. However the UI will be behind the switch as it may be more problematic. All the UI code is just txt files for the moments as I think it through. This means that this PR can go in and the next one will enable that behind the feature flag. When a feature is marked as stable we can have the settings automatically show it in the main body as well with a warning if the user has it disabled stating that it is now stable and available for use. |
||
|
|
||
| // import { Task } from '../Task'; | ||
|
|
||
| // import { promptForMark } from '../ui/TaskMarkModal'; | ||
| // export const selectStatus = (checking: boolean, editor: Editor, view: View) => { | ||
| // const mark = await promptForMark(this.app, this.plugin); | ||
| // if (mark) { | ||
| // this.markTaskOnLines(mark, this.getCurrentLinesFromEditor(editor)); | ||
| // } | ||
|
|
||
| // if (checking) { | ||
| // if (!(view instanceof MarkdownView)) { | ||
| // // If we are not in a markdown view, the command shouldn't be shown. | ||
| // return false; | ||
| // } | ||
|
|
||
| // // The command should always trigger in a markdown view: | ||
| // // - Convert lines to list items. | ||
| // // - Convert list items to tasks. | ||
| // // - Toggle tasks' status. | ||
| // return true; | ||
| // } | ||
|
|
||
| // if (!(view instanceof MarkdownView)) { | ||
| // // Should never happen due to check above. | ||
| // return; | ||
| // } | ||
|
|
||
| // // We are certain we are in the editor due to the check above. | ||
| // const path = view.file?.path; | ||
| // if (path === undefined) { | ||
| // return; | ||
| // } | ||
|
|
||
| // const cursorPosition = editor.getCursor(); | ||
| // const lineNumber = cursorPosition.line; | ||
| // const line = editor.getLine(lineNumber); | ||
|
|
||
| // const toggledLine = toggleLine({ line, path }); | ||
| // editor.setLine(lineNumber, toggledLine); | ||
|
|
||
| // // The cursor is moved to the end of the line by default. | ||
| // // If there is text on the line, put the cursor back where it was on the line. | ||
| // if (/[^ [\]*-]/.test(toggledLine)) { | ||
| // editor.setCursor({ | ||
| // line: cursorPosition.line, | ||
| // // Need to move the cursor by the distance we added to the beginning. | ||
| // ch: cursorPosition.ch + toggledLine.length - line.length, | ||
| // }); | ||
| // } | ||
| // }; | ||
|
|
||
| // const toggleLine = ({ line, path }: { line: string; path: string }): string => { | ||
| // let toggledLine: string = line; | ||
|
|
||
| // const task = Task.fromLine({ | ||
| // line, | ||
| // path, | ||
| // sectionStart: 0, // We don't need this to toggle it here in the editor. | ||
| // sectionIndex: 0, // We don't need this to toggle it here in the editor. | ||
| // precedingHeader: null, // We don't need this to toggle it here in the editor. | ||
| // }); | ||
| // if (task !== null) { | ||
| // toggledLine = toggleTask({ task }); | ||
| // } else { | ||
| // // If the task is null this means that we have one of: | ||
| // // 1. a regular checklist item | ||
| // // 2. a list item | ||
| // // 3. a simple text line | ||
|
|
||
| // // The task regex will match checklist items. | ||
| // const regexMatch = line.match(Task.taskRegex); | ||
| // if (regexMatch !== null) { | ||
| // toggledLine = toggleChecklistItem({ regexMatch }); | ||
| // } else { | ||
| // // This is not a checklist item. It is one of: | ||
| // // 1. a list item | ||
| // // 2. a simple text line | ||
|
|
||
| // const listItemRegex = /^([\s\t]*)([-*])/; | ||
| // if (listItemRegex.test(line)) { | ||
| // // Let's convert the list item to a checklist item. | ||
| // toggledLine = line.replace(listItemRegex, '$1$2 [ ]'); | ||
| // } else { | ||
| // // Let's convert the line to a list item. | ||
| // toggledLine = line.replace(/^([\s\t]*)/, '$1- '); | ||
| // } | ||
| // } | ||
| // } | ||
|
|
||
| // return toggledLine; | ||
| // }; | ||
|
|
||
| // const toggleTask = ({ task }: { task: Task }): string => { | ||
| // // Toggle a regular task. | ||
| // const toggledTasks = task.toggle(); | ||
| // const serialized = toggledTasks | ||
| // .map((task: Task) => task.toFileLineString()) | ||
| // .join('\n'); | ||
|
|
||
| // return serialized; | ||
| // }; | ||
|
|
||
| // const toggleChecklistItem = ({ | ||
| // regexMatch, | ||
| // }: { | ||
| // regexMatch: RegExpMatchArray; | ||
| // }): string => { | ||
| // // It's a checklist item, let's toggle it. | ||
| // const indentation = regexMatch[1]; | ||
| // const statusString = regexMatch[2].toLowerCase(); | ||
| // const body = regexMatch[3]; | ||
|
|
||
| // const toggledStatusString = statusString === ' ' ? 'x' : ' '; | ||
|
|
||
| // const toggledLine = `${indentation}- [${toggledStatusString}] ${body}`; | ||
|
|
||
| // return toggledLine; | ||
| // }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does changing line endings relate to "feat: Allow additional tasks states and filtering by states"?
Could this be a separate PR, with a statement about motivation?
What impact will this have on anyone who already has a clone of this repo on Windows? In the past, when these attributes have changed, I think I have struggled to get a clean repo, because it sees any local CRLF as differences. I seem to recall being confused, in any case.
From my own experience, I feel that mixing (seemingly) unrelated changes inside one PR generally makes it harder for reviews. The more focussed a PR is, the quicker it can be reviewed, and the quicker separate PRs for later features can be started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reduces noise in windows, if you have CRLF and then add them in the file will just go away as git is smart enough to know that they are the same on commit/push. However in a IDE they will be different as CRLF <> LF and you will get reg conflicts.
Adding this in just lets the IDE/git know they should always be LF on all operating systems and to ignore the git config settings for LF/CRLF for these files.
I can add this to a very small PR if needed, typically we would not make a separate PR for something like this and place it in a change going in as it is small and easily identified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made #682, so when it goes in an I merge with main this file will go away from this PR.