-
-
Notifications
You must be signed in to change notification settings - Fork 286
feat: Allow additional tasks states and filtering by states #675 #670
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
Changes from all commits
53d2d8e
bea862f
f2104c9
1bb8f9f
d8a3168
75eba71
d8a95b0
547f776
9716007
a3964f7
9c64527
f90abc4
4b88b4a
f02f2d2
67e97c6
b2ace95
3848523
788e25e
de6cb0f
010f439
bd31de1
b08bdc8
f58186a
9d27b5d
149eb34
d5976ba
292f684
69b8517
2b92499
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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; | ||
// }; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
import type { App, Editor, Plugin, View } from 'obsidian'; | ||
|
||
import { createOrEdit } from './CreateOrEdit'; | ||
//import { selectStatus } from './SelectStatus'; | ||
|
||
import { toggleDone } from './ToggleDone'; | ||
|
||
|
@@ -32,5 +34,12 @@ export class Commands { | |
icon: 'check-in-circle', | ||
editorCheckCallback: toggleDone, | ||
}); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this check be activated if the feature toggle is on? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, the feature toggle is specific to the pop-up menu. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, I removed it out to get a clean build and this PR in and will either add to this PR or will be in one to follow and under the flag. I have another draft PR in place using the feature flags based on this PR. |
||
// plugin.addCommand({ | ||
// id: 'select-status-modal', | ||
// name: 'Select Status', | ||
// icon: 'check-in-circle', | ||
// editorCheckCallback: selectStatus, | ||
// }); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
export type FeatureFlag = { | ||
[internalName: string]: boolean; | ||
}; | ||
|
||
/** | ||
* @todo documentation | ||
* | ||
* @since {date} | ||
*/ | ||
export class Feature { | ||
static readonly TASK_STATUS_MENU = new Feature( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aaaahhh, this is more granular than I first thought... It's very specifically to the status menu. I had wrongly assumed that the Feature would be 'Enable new task states UI'... So that's why a bunch of other code, about settings and commands, is commented out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I added flags very late in the cycle so it is for work post the status change all up, the UI will be covered by it and the append global filter capability will behind it as well which is in a separate PR. |
||
'TASK_STATUS_MENU', | ||
0, | ||
'Enables a right click menu for each task to allow you to select the task Status from the available next transition states.', | ||
'Task Status Menu', | ||
false, | ||
false, | ||
); | ||
|
||
static get values(): Feature[] { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How would this be used in practice? Something simple like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Later: interaction with features is via the settings class. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Look at #678 |
||
return [this.TASK_STATUS_MENU]; | ||
} | ||
|
||
static get settingsFlags(): FeatureFlag { | ||
const featureFlags: { [internalName: string]: boolean } = {}; | ||
|
||
Feature.values.forEach((feature) => { | ||
featureFlags[feature.internalName] = feature.enabledByDefault; | ||
}); | ||
return featureFlags; | ||
} | ||
|
||
/** | ||
* Converts a string to its corresponding default Feature instance. | ||
* | ||
* @param string the string to convert to Feature | ||
* @throws RangeError, if a string that has no corresponding Feature value was passed. | ||
* @returns the matching Feature | ||
*/ | ||
static fromString(string: string): Feature { | ||
const value = (this as any)[string]; | ||
if (value) { | ||
return value; | ||
} | ||
|
||
throw new RangeError( | ||
`Illegal argument passed to fromString(): ${string} does not correspond to any available Feature ${ | ||
(this as any).prototype.constructor.name | ||
}`, | ||
); | ||
} | ||
|
||
private constructor( | ||
public readonly internalName: string, | ||
public readonly index: number, | ||
public readonly description: string, | ||
public readonly displayName: string, | ||
public readonly enabledByDefault: boolean, | ||
public readonly stable: boolean, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How would boolean be used? Seeing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. enabled would be set by the user, this allows us to flag a feature as out of preview and stable so all features in the platform would be able to use it by default for new users and we can flag for existing users if they have it off in the configuration settings. Look at #678 |
||
) {} | ||
|
||
/** | ||
* Called when converting the Feature value to a string using JSON.Stringify. | ||
* Compare to the fromString() method, which deserializes the object. | ||
*/ | ||
public toJSON() { | ||
return this.internalName; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is internalName? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is the string name or key that is used to identify the feature. This makes sense now, seeing how Feature is used in the settings code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it is the key, separate to the nicer one that users see. |
||
} | ||
} |
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.
For my own education, I'm curious as to why
Todo
->TODO
?Asking as it increases the number of changes to review, not because I want to change it.
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.
Consistency in code, as these are constants I have seen they should be upper case in TS/JS. Also makes it super easy to see when reading code that these are not a normal variable.