feat: Add "Move task to file/section" feature #3735
Conversation
Create a new menu component that displays files and sections to move tasks to. The menu shows: - Files sorted by last modified time (top 10) - Sections within each file sorted by appearance - Current file and section marked - Option to move to end of file Signed-off-by: Ben Peter <[email protected]>
Add a new function to File.ts that handles moving tasks between different files and sections. Features: - Finds the correct insertion point in target section - Handles moving within same file - Cleans up the original task after insertion - Uses existing file cache for section detection Signed-off-by: Ben Peter <[email protected]>
- Add addMoveButton() method to HtmlQueryResultsRenderer - Add hideMoveButton option to QueryLayoutOptions - Add CSS styling for tasks-move button class - Place move button next to edit button in task extras Signed-off-by: Ben Peter <[email protected]>
- Add 'move' suggestion type to SuggestInfo - Add move suggestion to makeDefaultSuggestionBuilder - Handle move suggestion in EditorSuggestorPopup.selectSuggestion - Opens MoveTaskMenu when move suggestion is selected Signed-off-by: Ben Peter <[email protected]>
Add placeholder strings for the move task feature to all locale files. Includes button title, suggestion text, and various menu labels. Signed-off-by: Ben Peter <[email protected]>
- Add MoveTask.test.ts with tests for findInsertionPoint - Add MoveTaskTestHelpers.ts for test utilities - Update QueryLayoutOptions.test.ts for new hideMoveButton option - Update renderer approval files to include move button Signed-off-by: Ben Peter <[email protected]>
Changes: - Replace Menu with SuggestModal for fuzzy filtering - Show disambiguated file paths (minimal unique path) - Add moveTaskExcludedPaths setting to exclude folders - Add settings UI for excluded paths - Add SuggestModal mock for tests - Update jest config to use obsidian mock Signed-off-by: Ben Peter <[email protected]>
Changes: - Move task now includes all children (indented lines below task) - Use cursor line from editor for reliable deletion - Filter destinations by global filter (only show matching tasks) - Fix path disambiguation to show unique paths - Multi-strategy task finding: cursor line > task line number > content search Signed-off-by: Ben Peter <[email protected]>
- Try originalMarkdown first - Fall back to toFileLineString() if different - Try flexible search by description - Last resort: trust cursor line if it's a task - Add detailed debug logging to console Signed-off-by: Ben Peter <[email protected]>
UX improvements: - Show full folder path (dimmer, smaller font) for unambiguous identification - Filename displayed prominently (bold) - Section displayed below, indented with arrow, in italics - Task count shown in parentheses - "← current" indicator for current location - Better spacing and visual grouping This makes it immediately clear: 1. Which folder the file is in (full path, no ambiguity) 2. Which file within that folder 3. Which section within the file Signed-off-by: Ben Peter <[email protected]>
|
Hi, many thanks for doing this. I am behind on writing documentation on new features that have been submitted without documentation - and this is blocking me from doing a new release. So, to enable me to test and review it in a reasonable amount of time, please could you have a go at writing the first draft of the user documentation? I've written a guide on the mechanics of writing docs for this plugin: Don't agonise about the wording - just a first draft of how to use this, and any pitfalls or limitations that people might need to be aware of, would be really helpful. Many thanks in advance. |
- New page: docs/Editing/Moving Tasks.md - Screenshot: docs/images/move-task-modal.png - Updated About Editing.md with link to new page - Updated Layout.md with move button query element - Updated Settings.md with link to Move Task settings Signed-off-by: Ben Peter <[email protected]>
|
@claremacrae - I've just pushed the documentation :) |
|
this is the vault I used for testing ObsidianTasksMoveVault.tgz |
- Fix regex ReDoS vulnerability in MoveTaskModal.ts by using separate replace() calls instead of alternation pattern - Add void prefix to Notice() calls to satisfy unused object warning - Reduce cognitive complexity in multiple functions by extracting helpers: - MoveTaskModal.buildDestinations(): 22 -> ~10 - File.moveTaskToSection(): 22 -> ~8 - File.getTaskWithChildren(): 27 -> ~8 - File.findInsertionPoint(): 19 -> ~8 - EditorSuggestorPopup.selectSuggestion(): 16 -> ~6 - Use TypeError instead of Error for type check failures - Use RegExp.exec() instead of String.match() for consistency
Use a simple while loop to trim leading/trailing slashes instead of regex patterns that Sonar flags as potentially vulnerable to super-linear runtime due to backtracking.
|
Hi @benpeter, I'm a bit confused, when I run How can this be, as the pre-commit hooks run What have I misunderstood? |
Run yarn lint to apply Prettier formatting that should have been caught by pre-commit hooks. Signed-off-by: Ben Peter <[email protected]>
|
@claremacrae I had checked out the repo below my own repo (in which I keep other obsidian related things), and it seems that's how I unintentionally passed by the Lefthook setup, apologies - I should have properly set it up as a submodule. I've now run |
|
|
Hi @benpeter, I do think that this will be a really useful PR, but in the interests of time, sorry but I am going to be blunt, for the avoidance of doubt! The AI has generated tests which are both:
Because of the value of the feature, I am very much willing to work with you, to guide you towards generating effective tests. But before I spend quite a chunk of time trying to write up the sort of tests and coverage that would be needed, can I just please check that are willing to stick with it for a little while, to make the improvements. I'm asking because from experience:
|
claremacrae
added
scope: managing tasks
|
Hi @claremacrae - thanks for the feedback. I indeed didn't closely look into the tests and coverage. |
That's very kind, thank you. Before you do that work, can you try and express to me what you would like the tests to look like? |
claremacrae
left a comment
claremacrae
left a comment
There was a problem hiding this comment.
Hi, many thanks for offering this PR.
Some of these comments duplicate things I said earlier about the testing, but I'd typed these comments earlier, and it's helpful for there to be comments next to the relevant code.
|
|
||
| > [!released] | ||
| > | ||
| > - Move was introduced in Tasks X.X.X. |
There was a problem hiding this comment.
Please use the X.Y.Z pattern shown here for version number. Thank you.
https://publish.obsidian.md/tasks-contributing/Documentation/Version+numbers+in+documentation
| > | ||
| > - `urgency` was introduced in Tasks 1.14.0. | ||
| > - `tree` was introduced in Tasks 7.12.0. | ||
| > - `move button` was introduced in Tasks X.X.X. |
| "move": { | ||
| "buttonTitle": "Move task to another file or section", | ||
| "suggestionText": "Move task to another file/section", | ||
| "noFilesFound": "No other files with tasks found", | ||
| "currentFile": "current", | ||
| "currentSection": "current", | ||
| "noHeading": "(No heading)", | ||
| "endOfFile": "End of file", | ||
| "taskCount": "{{count}} tasks", | ||
| "success": "Task moved to {{filename}}", | ||
| "error": "Failed to move task: {{message}}" | ||
| }, |
There was a problem hiding this comment.
Please change all the string values (like "Move task to another file or section") in the non-English files to make them empty strings - like "".
The translation mechanism falls back to the English strings if the translation is not present or is empty for the current language.
Benefits of removing them include:
- It is easier to spot missing translations when the string is empty
- Prior to text being translated, we do not need to synchronise edits of English phrases across multiple different json files.
| // Map obsidian module to our mock | ||
| moduleNameMapper: { | ||
| '^obsidian$': '<rootDir>/tests/__mocks__/obsidian.ts', | ||
| }, | ||
|
|
There was a problem hiding this comment.
When I comment out these lines:
- The tests all still pass
yarn lintstill passes.
This suggests that the lines serve no purpose currently and can be removed.
| * @jest-environment jsdom | ||
| */ | ||
|
|
||
| import { findInsertionPointForTesting } from './MoveTaskTestHelpers'; |
There was a problem hiding this comment.
Did an AI write these two test files by any chance? 😄
Seriously...
- This entire test file only imports from
MoveTaskTestHelpers. - And
MoveTaskTestHelpersdoes not import fromsrc.
So whilst at first sight there is the appearance of a useful amount of test coverage, there is in fact none.
So File.ts has around 450 lines of added code with no actual test coverage.
For me, this is a show-stopper, it means I would have to manually review and test-by-hand all the changes, both now, and whenever the code is ever changed in future.
I see that some of the functions in MoveTaskTestHelpers are duplicates of non-exported functions in File.ts.
So the tests pass using a duplicate of production code - and in future, someone may modify the production code, but the tests keep passing, and we fail to spot breakages...
| "move": { | ||
| "buttonTitle": "Move task to another file or section", | ||
| "suggestionText": "Move task to another file/section", | ||
| "noFilesFound": "No other files with tasks found", | ||
| "currentFile": "current", | ||
| "currentSection": "current", | ||
| "noHeading": "(No heading)", | ||
| "endOfFile": "End of file", | ||
| "taskCount": "{{count}} tasks", | ||
| "success": "Task moved to {{filename}}", | ||
| "error": "Failed to move task: {{message}}" | ||
| }, |
There was a problem hiding this comment.
The .json files have not been consistently updated.
Some have two new blocks, and some, like this one, have only one.
| /* Move task modal */ | ||
| .move-task-suggestion { | ||
| padding: 6px 12px; | ||
| } | ||
|
|
||
| .move-task-suggestion.is-current { | ||
| opacity: 0.6; | ||
| } |
There was a problem hiding this comment.
For maintainability, we have divided up the SCSS in to separate files, according to their purpose.
So this Renderer.scss is specifically for styling the results of rendering task results.
It looks like a lot of the additions to this file are to do with the new modal, so they should be in their own file, next to the code for the modal.
So likely MoveTaskModal.scss.
Then the new file should be included in src/styles.scss.
| /** | ||
| * Finds the last task line within a range. | ||
| */ | ||
| function findLastTaskLineInRange(listItems: MockListItem[], startLine: number, endLine: number): number { |
There was a problem hiding this comment.
This is a duplicate of production code.
| /** | ||
| * Finds the insertion point for tasks with no heading. | ||
| */ | ||
| function findInsertionPointNoHeading(fileLines: string[], headings: MockHeading[], listItems: MockListItem[]): number { |
There was a problem hiding this comment.
This is a duplicate of production code.
| * @param appendToEnd - If true, always append to end of file | ||
| * @returns The line number to insert at | ||
| */ | ||
| export function findInsertionPointForTesting( |
There was a problem hiding this comment.
I am unclear the extent to which this is duplicating the logic in production code...
|
@benpeter, here you (or your AI) go.... Ask me if you have any questions, once you have tried this out. Things to tell an AI, to add testsThis is an overview of standard testing techniques, as they are used in this project. Separation of layers
Testing code which uses
|
|
Hi Ben, Just one note about how I use the review comments - please don't click 'Resolve Conversation' - I use this to keep track of things that I need to review later on. So I will click 'Resolve Conversation' once I've made sure that I understand any comments or improvements in the code... Thanks. |
| /** | ||
| * A mock implementation of the Obsidian SuggestModal class. | ||
| * Used for testing components that extend SuggestModal like MoveTaskModal. | ||
| */ | ||
| export class SuggestModal<T> extends Modal { | ||
| public setPlaceholder(_placeholder: string): void { | ||
| // Mocked interface, no-op | ||
| } | ||
| public setInstructions(_instructions: { command: string; purpose: string }[]): void { | ||
| // Mocked interface, no-op | ||
| } | ||
| public getSuggestions(_query: string): T[] { | ||
| return []; | ||
| } | ||
| public renderSuggestion(_item: T, _el: HTMLElement): void { | ||
| // Mocked interface, no-op | ||
| } | ||
| public onChooseSuggestion(_item: T, _evt: MouseEvent | KeyboardEvent): void { | ||
| // Mocked interface, no-op | ||
| } | ||
| } |
There was a problem hiding this comment.
Commenting this out does not result in an lint or jest failures - so it is unused code.
In general, I much prefer code to only be written when it is actually needed - this avoids later doing maintenance on code which is unused.
Let's leave it here for now, in the hope that there will be tests eventually for the new modal. By which time, these functions will no longer be empty, but will actually be participating in the tests.
3 participants
Types of changes
Changes visible to users:
feat- non-breaking change which adds functionality)Description
This PR adds a new "Move task" feature that allows users to move tasks (and their subtasks) from one file/section to another directly from the Tasks UI.
Features Added
hide move buttonquery instruction for controlling visibilityVisual Design
The destination modal uses clear visual hierarchy:
Motivation and Context
Currently, moving a task requires manual copy/paste between files and manual deletion - a tedious 5-step process. This feature reduces it to 2 clicks.
Closes #3734
Related issues:
How has this been tested?
findInsertionPointlogicScreenshots (if appropriate)
Checklist
yarn run lint.Terms