Merge pull request #3460 from obsidian-tasks-group/feat-validate-include-names

feat: Add error-checking of duplicate names of "Includes"

Author: claremacrae

resources/sample_vaults/Tasks-Demo/.obsidian/plugins/obsidian-tasks-plugin/data.json

@@ -1,5 +1,14 @@
 {
-  "includes": {},
+  "includes": {
+    "hide_all_dates": "# Hide any values for all 6 date fields\nhide due date\nhide scheduled date\nhide start date\nhide created date\nhide done date\nhide cancelled date",
+    "hide_buttons": "# Hide postpone, edit and backinks\nhide postpone button\nhide edit button\nhide backlinks",
+    "hide_other_fields": "# Hide all the non-date fields, keep tags\nhide id\nhide depends on\nhide recurrence rule\nhide on completion\nhide priority",
+    "just_the_description_and_tags": "# Show only description and any tags\ninclude hide_all_dates\ninclude hide_other_fields\ninclude hide_buttons",
+    "just_the_description": "# Show only description, not even the tags\ninclude just_the_description_and_tags\nhide tags",
+    "filter_by_context": "filter by function let fn = (ctx) => task.tags.includes(`#context/${ctx}`); return fn",
+    "extract_contexts_1": "ctx => task.tags.includes(`#context/${ctx}`)",
+    "extract_contexts_2": "(ctx => task.tags.includes(`#context/${ctx}`))"
+  },
   "globalQuery": "# Exclude templates files:\nroot does not include _meta\n\n# Ignore the sample tasks that demonstrate Theme and Snippet support:\npath does not include Manual Testing/SlrVb's Alternative Checkboxes\npath does not include Styling/Snippet -\npath does not include Styling/Theme -\n",
   "globalFilter": "#task",
   "removeGlobalFilter": true,

resources/sample_vaults/Tasks-Demo/How To/Reuse instructions across the vault.md

@@ -0,0 +1,90 @@
+---
+TQ_explain: false
+TQ_extra_instructions: |-
+  ignore global query
+  limit 10
+---
+# Reuse instructions across the vault
+
+These searches are for experimenting with, and understanding, the new "Includes" facility, which was released in Tasks X.Y.Z.
+
+Includes values can be defined in the Tasks settings.
+
+explain: `INPUT[toggle:TQ_explain]`
+
+## List all the known 'include' values in settings
+
+(assuming there is not one called `xxxx`!)
+
+```tasks
+include xxxx
+```
+
+## Show all the fields
+
+explain: `INPUT[toggle:TQ_explain]`
+
+```tasks
+```
+
+## Hide all the fields
+
+explain: `INPUT[toggle:TQ_explain]`
+
+```tasks
+include hide_all_dates
+include hide_other_fields
+```
+
+## Hide all the Tasks user interface elements
+
+explain: `INPUT[toggle:TQ_explain]`
+
+```tasks
+include hide_buttons
+```
+
+## Hide everything, including tags
+
+explain: `INPUT[toggle:TQ_explain]`
+
+```tasks
+include just_the_description_and_tags
+```
+
+## Advanced use: return a function, that takes a parameter from the query source
+
+### Has context 'home' - and group by the Include text - version 1
+
+explain: `INPUT[toggle:TQ_explain]`
+
+```tasks
+# For debug/explanatory purposes, show the source of the Include as a group name:
+group by function const x = "{{includes.filter_by_context}}"; return x
+
+{{includes.filter_by_context}}('home')
+```
+
+### Has context 'home' - and group by the Include text - version 2
+
+explain: `INPUT[toggle:TQ_explain]`
+
+```tasks
+# For debug/explanatory purposes, show the source of the Include as a group name:
+group by function const x = "{{includes.extract_contexts_1}}"; return x
+
+# includes.extract_contexts_1 value needs to be surrounded by parentheses ():
+filter by function return ({{includes.extract_contexts_1}})('home')
+```
+
+### Has context 'home' - and group by the Include text - version 3
+
+explain: `INPUT[toggle:TQ_explain]`
+
+```tasks
+# For debug/explanatory purposes, show the source of the Include as a group name:
+group by function const x = "{{includes.extract_contexts_2}}"; return x
+
+# includes.extract_contexts_1 value has the parentheses, to simplify use:
+filter by function {{includes.extract_contexts_2}}('home')
+```

src/Config/IncludesSettingsService.ts

@@ -1,25 +1,71 @@
 import { renameKeyInRecordPreservingOrder } from '../lib/RecordHelpers';
 import type { IncludesMap } from './Settings';
 
+/**
+ * Represents a map of include keys and their current values
+ * used during validation
+ */
+export interface RenamesInProgress {
+    [originalName: string]: string;
+}
+
+export class RenameResult {
+    public readonly originalName: string;
+    public readonly isValid: boolean;
+    public readonly errorMessage: string | null;
+
+    constructor(originalName: string, isValid: boolean, errorMessage: string | null) {
+        this.originalName = originalName;
+        this.isValid = isValid;
+        this.errorMessage = errorMessage;
+    }
+}
+
+/**
+ * Result of validating multiple include values at once
+ */
+export interface RenameResults {
+    [originalName: string]: RenameResult;
+}
+
 export class IncludesSettingsService {
+    /**
+     * Validates multiple include names against each other
+     * @param renames Map of original keys to their current values in UI
+     * @returns Object mapping each key to its validation result
+     */
+    public validateRenames(renames: RenamesInProgress): RenameResults {
+        const results: RenameResults = {};
+
+        // Check each key against all others
+        for (const [originalName, newName] of Object.entries(renames)) {
+            const includesWithOtherPendingRenames: IncludesMap = {};
+
+            for (const [otherOriginalName, otherNewName] of Object.entries(renames)) {
+                // Skip the name being validated to avoid false duplicate matches.
+                if (otherOriginalName !== originalName) {
+                    includesWithOtherPendingRenames[otherNewName] = '';
+                }
+            }
+
+            // Pass empty string as keyBeingRenamed since we're not excluding any key from duplicate check
+            results[originalName] = this.validateRename(includesWithOtherPendingRenames, '', newName);
+        }
+
+        return results;
+    }
+
     /**
      * Validates if an include name is valid
      * @param includes The current includes map
      * @param keyBeingRenamed The key being renamed
-     * @param proposedName The proposed name to validate
+     * @param newName The proposed name to validate
      * @returns An object with validation result and error message if any
      */
-    public validateIncludeName(
-        includes: Readonly<IncludesMap>,
-        keyBeingRenamed: string,
-        proposedName: string,
-    ): { isValid: boolean; errorMessage: string | null } {
+    public validateRename(includes: Readonly<IncludesMap>, keyBeingRenamed: string, newName: string): RenameResult {
         // Check for empty name
-        if (!proposedName || proposedName.trim() === '') {
-            return {
-                isValid: false,
-                errorMessage: 'Include name cannot be empty or all whitespace',
-            };
+        if (!newName || newName.trim() === '') {
+            return new RenameResult(keyBeingRenamed, false, 'Include name cannot be empty or all whitespace');
         }
 
         for (const existingKey of Object.keys(includes)) {
@@ -28,15 +74,12 @@ export class IncludesSettingsService {
                 continue;
             }
 
-            if (existingKey === proposedName) {
-                return {
-                    isValid: false,
-                    errorMessage: 'An include with this name already exists',
-                };
+            if (existingKey.trim() === newName.trim()) {
+                return new RenameResult(keyBeingRenamed, false, 'An include with this name already exists');
             }
         }
 
-        return { isValid: true, errorMessage: null };
+        return new RenameResult(keyBeingRenamed, true, null);
     }
 
     /**

src/Config/IncludesSettingsUI.scss

@@ -51,3 +51,9 @@
         align-items: unset;
     }
 }
+
+/* Error state for invalid input */
+.tasks-settings .tasks-includes-setting .tasks-includes-key.has-error {
+    border-color: var(--text-error);
+    border-width: 2px;
+}

src/Config/IncludesSettingsUI.ts

@@ -1,6 +1,6 @@
 import { Setting, TextAreaComponent } from 'obsidian';
 import type TasksPlugin from '../main';
-import { IncludesSettingsService } from './IncludesSettingsService';
+import { IncludesSettingsService, type RenamesInProgress } from './IncludesSettingsService';
 import { type IncludesMap, type Settings, getSettings, updateSettings } from './Settings';
 
 type RefreshViewCallback = () => void;
@@ -13,6 +13,7 @@ type RefreshViewCallback = () => void;
 export class IncludesSettingsUI {
     private readonly plugin: TasksPlugin;
     private readonly includesSettingsService = new IncludesSettingsService();
+    private readonly nameFields: Map<string, { inputEl: HTMLInputElement; originalKey: string }> = new Map();
 
     /**
      * Creates a new instance of IncludesSettingsUI
@@ -33,6 +34,9 @@ export class IncludesSettingsUI {
         const renderIncludes = () => {
             includesContainer.empty();
 
+            // Clear the input map when re-rendering
+            this.nameFields.clear();
+
             Object.entries(settings.includes).forEach(([key, value]) => {
                 this.renderIncludeItem(includesContainer, settings, key, value, renderIncludes);
             });
@@ -67,10 +71,16 @@ export class IncludesSettingsUI {
             text.setPlaceholder('Name').setValue(key);
             text.inputEl.addClass('tasks-includes-key');
 
+            // Store reference to this input with its original key
+            this.nameFields.set(key, { inputEl: text.inputEl, originalKey: key });
+
             let newKey = key;
 
             text.inputEl.addEventListener('input', (e) => {
                 newKey = (e.target as HTMLInputElement).value;
+
+                // Validate all inputs to update any that might be affected
+                this.validateAllInputs();
             });
 
             // Handle renaming an include
@@ -109,6 +119,8 @@ export class IncludesSettingsUI {
             });
         });
 
+        // TODO Add reorder button or facility to drag and drop items
+
         // Add delete button
         setting.addExtraButton((btn) => {
             btn.setIcon('cross')
@@ -120,6 +132,36 @@ export class IncludesSettingsUI {
         });
     }
 
+    /**
+     * Validates all input elements and updates their styling
+     */
+    private validateAllInputs() {
+        // Build the current key-value map for validation
+        const currentValues: RenamesInProgress = {};
+
+        this.nameFields.forEach(({ inputEl, originalKey }) => {
+            currentValues[originalKey] = inputEl.value;
+        });
+
+        // Get validation results from the service
+        const validationResults = this.includesSettingsService.validateRenames(currentValues);
+
+        // Apply styling based on validation results
+        this.nameFields.forEach(({ inputEl, originalKey }) => {
+            const result = validationResults[originalKey];
+
+            if (result && !result.isValid) {
+                inputEl.addClass('has-error');
+
+                // Optionally, you could add a title attribute to show the error message on hover
+                inputEl.title = result.errorMessage || '';
+            } else {
+                inputEl.removeClass('has-error');
+                inputEl.title = '';
+            }
+        });
+    }
+
     /**
      * Sets up auto-resizing behaviour for a textarea component
      * @param textArea The textarea component to configure
@@ -165,6 +207,7 @@ export class IncludesSettingsUI {
         settings: Settings,
         refreshView: RefreshViewCallback | null,
     ) {
+        // TODO Consider how this relates to the validation code - should it refuse to save settings if validation fails?
         // Update the settings in storage
         updateSettings({ includes: updatedIncludes });
         await this.plugin.saveSettings();