Keynote Manager : Fix New ADC Bug + New UI

[tay0thman]

Bug Fixes:

• ADC / .NET Framework Compatibility: Patched an issue where Revit 2023 and 2024 refused to launch due to ReadOnlyList subscripting conflicts introduced by the latest Autodesk Desktop Connector updates.

New UI Features & Enhancements:

• Persistent Modeless UI: Transitioned the tool to a modeless window, allowing users to keep the Keynote Manager open and actively interact with the Revit model simultaneously.

• Unified Hierarchical Tree: Replaced the split-view interface with a single, structured tree hierarchy. Users can now organize keynotes intuitively using drag-and-drop, or through new indent/outdent controls.

• Quick Text Formatting: Added a capitalization menu to instantly convert keynote text cases (UPPER, lower, Title, Sentence) directly within the UI.

• Optimized Search Performance: Implemented a debounce timer to intercept rapid keystrokes, drastically improving the responsiveness of search and filtering—especially for large keynote databases.

--

Checklist

Before submitting your pull request, ensure the following requirements are met:

• [OK ] Code follows the PEP 8 style guide.
• [OK ] Code has been formatted with Black using the command:

  pipenv run black {source_file_or_directory}

• [ OK] Changes are tested and verified to work as expected.

---

Related Issues

If applicable, link the issues resolved by this pull request:

• Resolves [Bug]: Keynote Manager fails to load #3135, [Bug]: Keynote Manager '<>z__ReadOnlyList[WorkspacePathToIt... object is not subscriptable #3089 , [Bug]: Keynote manager crash Revit #2654, Keynotes Manager Note Organization #3036,

---

Thank you for contributing to pyRevit! 🎉

[Copilot]

Pull request overview

This PR enhances the Keynote Manager tool with critical bug fixes and significant UI improvements. It addresses ADC/.NET Framework compatibility issues that prevented the tool from working in Revit 2023/2024 when using BIM360/ACC-hosted keynote files, and transforms the UI into a more intuitive and efficient modeless interface.

Changes:

• Fixed ReadOnlyList subscripting bug in ADC integration for .NET Framework (Revit 2023/2024) by implementing a monkey-patch with fallback indexing strategies
• Converted window from modal to modeless with ExternalEvent-based Revit API access, enabling concurrent model interaction
• Unified the tree structure by merging categories and keynotes into a single hierarchical view with drag-and-drop, indent/outdent, and move up/down operations
• Added debounced search (300ms delay) to improve performance on large keynote databases
• Implemented quick text case transformation menu (UPPER, lower, Title, Sentence)

[jmcouffin]

@devloai please review

[devloai]

Review: Keynote Manager — Fix New ADC Bug + New UI

Overall this is a solid, well-structured rewrite with clear intent. The modeless window architecture, debounced search, drag-and-drop tree, and ADC compatibility fix are all good additions. A few issues worth addressing before merge:

---

🔴 High — Hardcoded temp key collision risk (_swap_sibling)

The key-swap operation uses a hardcoded temporary key "__swap_temp__" (and "__swap_ref__" for Revit element references). If a keynote with this key already exists in the database, the operation will corrupt data or fail silently.

# script.py ~line 1049
temp_key = "__swap_temp__"

Fix: generate a unique temp key at runtime:

import uuid
temp_key = "__swap_{}__".format(uuid.uuid4().hex[:8])
# Verify it doesn't already exist before use

---

🔴 High — Global monkey-patching of adc module without idempotency guard

The ADC patches are applied globally to the adc module on import (due to __persistentengine__ = True). Any other pyRevit tool using adc._get_item, adc._get_item_lockstatus, etc. will get the patched versions too — which may not be the intent.

# script.py ~line 125
if not HOST_APP.is_newer_than("2024"):
    adc._get_item = _patched_get_item  # global side effect

Fix: add an idempotency marker so the patch only applies once:

if not HOST_APP.is_newer_than("2024") and not getattr(adc._get_item, '_patched', False):
    _patched_get_item._patched = True
    adc._get_item = _patched_get_item
    # ...

---

🟠 Medium — Potential AttributeError in update_used (keynotesdb.py line 244)

owner_view = doc.GetElement(doc.GetElement(keyid).OwnerViewId)

doc.GetElement(keyid) can return None if the element was deleted since the last refresh. This will throw AttributeError on .OwnerViewId.

Fix:

kel = doc.GetElement(keyid)
if kel:
    owner_view = doc.GetElement(kel.OwnerViewId)
    if owner_view:
        self.tooltip += '\n' + revit.query.get_name(owner_view)

---

🟠 Medium — legacy_kfile may be unbound (keynotesdb.py import_legacy_keynotes)

After the encoding-detection loop, the check if legacy_kfile: references a variable that's only defined inside the with block. If all encodings fail (and an exception is raised), this is fine — but the pattern is fragile. Use knote_lines is not None instead:

knote_lines = None
for idx, encoding in enumerate(encodings):
    try:
        with codecs.open(...) as f:
            knote_lines = f.readlines()
        break
    except Exception:
        ...

if knote_lines is not None:
    conn.BEGIN(KEYNOTES_DB)
    ...

---

🟠 Medium — Silent exception in RevitActionHandler error display

# script.py ~line 163
except Exception:
    pass

When displaying an error alert fails, this silently swallows the failure. At minimum add a debug log:

except Exception as disp_ex:
    logger.debug('Failed to display error in window | %s' % disp_ex)

---

🟡 Medium — File handle leak in _convert_existing

open(self._kfile, 'w').close()  # no context manager

Use a context manager:

with open(self._kfile, 'w'):
    pass

---

🟡 Low — show_keynote closes the modeless window

show_keynote() calls self.Close() before queuing the Revit action, destroying the persistent window just to show results in the output console. This defeats the UX goal of the persistent modeless design. Consider keeping the window open and displaying results in a non-destructive way.

---

📝 Line-ending noise

EditRecord.xaml and keynotesdb.py diffs are exclusively line-ending changes (LF → CRLF). These create noisy diffs without functional change. Consider configuring .gitattributes to normalize line endings for XAML/Python files.

[tay0thman]

@copilot code review[agent] @devloai

QAQC Review Responses — Keynote Manager PR

🔴 High — Hardcoded temp key collision risk (_swap_sibling)
Replaced both hardcoded sentinel keys (swap_temp and swap_ref) with runtime-generated unique keys using uuid.uuid4().hex[:8], eliminating any possibility of collision with user-defined keynote keys.

🔴 High — Global monkey-patching of adc module without idempotency guard
Added an _readonlylist_patched flag on the adc module object so the patch only applies once per engine session regardless of how many times the script is invoked. The patched functions are strict supersets of the originals — they try [0] first (preserving .NET 8 behavior), then fall back to .Item[0] and iteration for .NET Framework — so any other pyRevit tool hitting the same ReadOnlyList bug benefits from the fix as well.

🟡 Medium — Potential AttributeError in update_used (keynotesdb.py line 244)
Since keynotesdb.py is part of pyRevit core and not ours to modify, the fix was applied at the call site in get_used_keynote_elements() with three null guards: element existence (kn is None), parameter existence, and key string existence, handling the case where elements were deleted since the last refresh.

🟡 Medium — legacy_kfile may be unbound (keynotesdb.py import_legacy_keynotes)
Acknowledged — this is in pyRevit's core library and out of scope for this PR. Valid observation that would make a good upstream issue.

🟡 Medium — Silent exception in RevitActionHandler error display
Replaced the bare except: pass with logger.debug('Failed to display error in window | %s' % disp_ex), preserving the non-throwing behavior while making failures traceable in the pyRevit debug log.

🟡 Medium — File handle leak in _convert_existing
Replaced open(self._kfile, 'w').close() with a context manager (with open(self._kfile, 'w'): pass), guaranteeing the handle is released even if an exception occurs — particularly important under IronPython's non-deterministic GC.

🟢 Low — show_keynote closes the modeless window
Removed the self.Close() call entirely — the window now stays open, prints placement results to the pyRevit output console via _revit_run, and updates the status bar with a summary count. Keynotes with zero placements get an inline status bar message without opening the output console at all. Added null-safety on doc.GetElement() inside the deferred closure.

🟢 Low — Line-ending noise
Acknowledged — out of scope. Recommend adding .gitattributes rules (*.py text eol=lf, *.xaml text eol=crlf) in a separate housekeeping PR.

🔴 Follow-up — Revit crash when clicking inside modeless keynote manager
Added WindowInteropHelper to set Revit's HWND as the owner of the WPF window in init, which tells Windows the two windows are parent-child so focus transitions route through the correct Win32 message pump hierarchy instead of colliding.

🔴 Follow-up — Intermittent crash on focus transfer between Revit and modeless window
The WindowInteropHelper.Owner alone wasn't sufficient — added an HwndSource WndProc hook that intercepts WM_MOUSEACTIVATE (0x0021) and returns MA_NOACTIVATE, preventing the synchronous re-entrant activation that corrupts Revit's internal state when it's mid-processing. Activation is deferred to the next idle cycle via Dispatcher.BeginInvoke at Background priority, with an _activation_pending flag to prevent stacking. The hook is installed via SourceInitialized and cleaned up with RemoveHook in window_closing.

🟡 Follow-up — Tree scrolls to top after drag-and-drop / hierarchy changes
Fixed — _update_full_tree() now saves the selected node's key and the TreeView's vertical scroll offset before rebuilding, then restores both after ItemsSource is replaced. Selection restoration walks the new tree to find the matching key, expands ancestor containers, and calls BringIntoView(). Scroll restoration writes back the VerticalOffset on the internal ScrollViewer. Both are deferred via Dispatcher.BeginInvoke at Loaded priority so they execute after WPF completes its layout pass and the item containers actually exist. This applies to all operations that trigger a tree rebuild — drag-drop, indent/outdent, move up/down, add, edit, delete, re-key, and capitalization.

[tay0thman]

@devloai please review the latest commit

[devloai]

Code Review — Latest Commit

The latest commit is a merge of develop into the PR branch (no new logic changes). This review covers the full code as it stands.

🔴 High — keynotesdb.py line 244: Null-dereference crash in update_used

doc.GetElement(keyid) can return None for stale/deleted elements. The result is immediately chained: .OwnerViewId — this throws AttributeError and breaks keynote "in-use" tracking for all subsequent elements in the loop.

🟠 Medium — keynotesdb.py line 579: Wrong truthiness check after import

if legacy_kfile: tests the (always-truthy) file handle object, not whether lines were actually read into knote_lines. Replace with if knote_lines:.

🔴 High — script.py: ADC patch applied without idempotency guard

With __persistentengine__ = True, module-level code executes on every script invocation. The ADC monkey-patch at lines 125–130 runs every time the manager is opened, repeatedly replacing already-patched functions. Add a sentinel flag:

if not HOST_APP.is_newer_than("2024") \
        and not getattr(adc, '_pyrevit_adc_patched', False):
    # ... apply patches ...
    adc._pyrevit_adc_patched = True

🔴 High — script.py _swap_sibling: Hardcoded "__swap_temp__" collision risk

The temp key "__swap_temp__" used during key-swap operations will silently corrupt the database if a real keynote key named __swap_temp__ exists, or if a swap operation is interrupted mid-flight (crash between the 3-step rename). The intermediate state leaves an orphaned __swap_temp__ record. Consider wrapping the 3-step rename in a BulkAction context manager so it's atomic, and/or assert that "__swap_temp__" is not already in the DB before starting.

🟠 Medium — script.py show_keynote / place_keynote: Closes modeless window

Both show_keynote (line 1600) and place_keynote (line 1626) call self.Close() before dispatching to Revit. This defeats the core "persistent modeless UI" goal — users lose their keynote manager state every time they click Find or Place. The window should remain open, and the find/place actions should work in the background via the existing _revit_run mechanism.

🟡 Low — script.py line 777: Unclosed file handle

open(self._kfile, 'w').close()

This opens the file without a context manager. Use open(self._kfile, 'w') as f: pass or simply io.open(...) to guarantee the handle is closed even on exceptions.

[tay0thman]

@devloai after four commits, please run one final review.

[devloai]

Final Review — PR #3136 (after 4 commits)

✅ Fixes confirmed

Both devloai-applied fixes are correctly in place:

• keynotesdb.py:244 — Null-check added before accessing OwnerViewId on a potentially stale element.
• keynotesdb.py:582 — if legacy_kfile: → if knote_lines: correctly tests whether file data was read, not the file handle.

---

Remaining issues in script.py

🟠 get_history() called before null-guard (show_keynote, line 1606)

kel = revit.doc.GetElement(kid)
ehist = revit.query.get_history(kel)   # ← kel may be None here!
if kel:
    ...

get_history(kel) is called one line before the if kel: guard. If kel is None this raises an AttributeError inside the Revit context handler. Move the call inside the if kel: block.

🟡 show_keynote / place_keynote close the window (lines 1600, 1626)

Both methods call self.Close() before dispatching to Revit. This contradicts the PR's stated goal of a "persistent modeless UI". If closing is intentional for these specific flows, the PR description should clarify. If not, remove self.Close() and let the output console open independently.

🟡 ADC monkey-patch re-applies on every tool launch (line 125)

With __persistentengine__ = True, the module-level if not HOST_APP.is_newer_than("2024"): block re-executes every time the button is clicked (because the module is re-imported each run in a persistent engine). The patch is idempotent (reassigning the same function), so this is benign — but worth a comment explaining it to future readers.

🟡 Hardcoded "__swap_temp__" key (line 1049)

The temporary key is still a plain string constant. If a user happens to have a keynote with exactly this key, the swap will corrupt data. A minimal improvement would be using a UUID-based temp key:

import uuid
temp_key = "__swap_%s__" % uuid.uuid4().hex[:8]

---

Summary

The two critical bugs from the initial review have been fixed. The remaining items are medium/low severity. The most important one to address before merge is the get_history(kel) before null-check which will reliably crash when any keynote element is deleted from the model.

[tay0thman]

updated per @devloai , please review the latest commit.

[tay0thman]

@devloai Please review one minor fix