Add every new custom list to the Lists area, not just the last one - #1075
Draft
jasonleenaylor wants to merge 1 commit into
Draft
Add every new custom list to the Lists area, not just the last one#1075jasonleenaylor wants to merge 1 commit into
jasonleenaylor wants to merge 1 commit into
Conversation
The Lists area compared a cached count of ownerless lists against the current one and, when they differed, appended a single list picked by index. It now compares the full set of ownerless lists against the tool nodes already in the window configuration, matching on list guid, and adds only the ones that are missing. Tools whose list no longer exists are skipped when the display is filled, so a list deleted in one main window does not break the sidebar in another. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
NUnit Tests 1 files 1 suites 11m 33s ⏱️ Results for commit 7e704c3. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1075 +/- ##
==========================================
+ Coverage 38.05% 38.10% +0.04%
==========================================
Files 1499 1499
Lines 350128 350147 +19
Branches 40239 40244 +5
==========================================
+ Hits 133232 133412 +180
+ Misses 187609 187458 -151
+ Partials 29287 29277 -10
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Lists area compared a cached count of ownerless lists against the current one and,
when they differed, appended a single list picked by index. It now compares the full set
of ownerless lists against the tool nodes already in the window configuration, matching on
list guid, and adds only the ones that are missing.
Tools whose list no longer exists are skipped when the display is filled, so a list deleted
in one main window does not break the sidebar in another.
Why it failed
FillListAreaListpolled: it counted the ownerless lists and compared that against a cachedm_ccustomLists. On a difference it addedcustomLists[customLists.Count - 1]— one list,chosen by index. Two problems followed.
Any caller creating more than one unowned list in a session only got one of them in the
sidebar.
PrepFLExDBDll/Preparer.cscreates two andParserCore/XAmplePropertiesPreparer.cscreates a third, so that is reachable in normal use. The index was unsafe as well:
AllInstances()is backed by aHashSet, so enumeration order is unspecified and the "last"element is not the newest list.
Because the counter only advanced by one, the counts stayed mismatched and the next refresh
re-added the same list. The tool, clerk, command and context-menu nodes are all appended with
no duplicate guard, so the configuration accumulated duplicates — and since
CommandSetis aHashtable, the duplicate command id made the third refresh throwArgumentException: Item has already been added.Both counters are gone. The set difference answers the same questions directly, and the guid
key does not depend on enumeration order or on list names, which can collide.
The deletion guard
Deleting a custom list already works and rebuilds the window it was deleted in, which reloads
the configuration from disk. But
PropertyTable,MediatorandWindowConfigurationareper main window, and
ReloadAreaToolsreplaces only the active one. With a second main windowopen, that window keeps a tool node whose guid no longer resolves, and the next sidebar refresh
throws
KeyNotFoundExceptionout ofGetListByGuid.Five lines skip tool nodes whose list is gone. Pruning the stale nodes properly would mean
disposing the clerk, unregistering the command, and switching away from the active tool — all
of which only matter in the window that is being torn down anyway, so this fix deliberately
stops at the display.
Verification
.\test.ps1 -SkipNative -TestProject "Src/LexText/LexTextDll/LexTextDllTests"— 6/6 pass,build clean.
Each new test was run against the unfixed code and fails there:
FillListAreaList_AddsEveryListCreatedSinceLastRefreshExpected: < "SecondListEdit", "ThirdListEdit" > But was: < "ThirdListEdit" >FillListAreaList_RepeatedRefresh_DoesNotDuplicateConfigNodesArgumentException : Item has already been added. Key in dictionary: 'CmdJumpToThirdListList'FillListAreaList_ListDeletedInAnotherWindow_DropsItsToolKeyNotFoundException : Key 9c0d3612-… not found in identity mapNotes for review
Preparer.csandXAmplePropertiesPreparer.cswere requiring a full FieldWorks reload to appear.tracked as Creating an unowned CmPossibilityList issues no PropChanged liblcm#397. Detection here is still poll-on-display.
AreaListener.GetToolForListhas the same stale-guid exposure throughRecordList.cscase "unowned", which callsGetObjectwith noTryvariant. Left alone —RecordList.Initasserts the result is non-null, so relaxing that shared API is a different risk profile. Worth
its own ticket.
🤖 Generated with Claude Code
This change is