fix: restore connection tree state when the search filter is cleared - #159
Open
jafin wants to merge 2 commits into
Open
fix: restore connection tree state when the search filter is cleared#159jafin wants to merge 2 commits into
jafin wants to merge 2 commits into
Conversation
Clearing the search box left the tree in a broken state: folders that were expanded before filtering came back collapsed, and the tree could throw an invalid-index exception from EnsureVisible. Two compounding defects in RemoveFilter(): ApplyFilter stored ExpandedObjects as the pre-filter state, but that getter returns a live view over the tree model's expansion map rather than a snapshot. The ExpandAll() that follows rewrote the same map, and the ExpandedObjects setter clears the map before enumerating the value it was handed - the very collection it just emptied - so nothing was restored. Assigning ExpandedObjects also only updates the model's map; the branch structure and row list are not rebuilt. RemoveFilter rebuilt first (via ResetColumnFiltering) and mutated the map afterwards, leaving expansion state and row indexes reading from different sources. That mismatch is what let IndexOf hand EnsureVisible a row past the end of the list. The pre-filter state is now materialized into a list, and RemoveFilter uses RebuildAll to restore expansion, selection and the row list together.
Clearing the filter runs three passes: UseFiltering and the column filter reset each trigger UpdateFiltering, then the tree is rebuilt to restore the pre-filter expansion state. Each pass repainted and re-ran the column auto-resize, which measures the text of every visible row. Hold painting across the whole method and suspend the per-pass auto-resize, doing it once at the end against the restored rows. The early return when there is no saved expansion state became a conditional so that final resize runs on every path. Freeze()/Unfreeze() would not help here: TreeListView is a virtual list, and VirtualObjectListView.BuildList(bool) overrides the base without the "if (Frozen) return" guard, so freezing suspends painting while adding one more BuildList in DoUnfreeze. Collapsing the two UpdateFiltering passes into one would mean changing the vendored ObjectListView filtering workflow, which is out of scope here. No behavior change; the existing filter regression tests cover it.
PR Summary by QodoRestore connection tree state when clearing search filter
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1. UI tests lack message pump
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+38
to
+39
| private List<object>? _preFilterExpandedObjects; | ||
| private bool _columnAutoResizeSuspended; |
There was a problem hiding this comment.
1. Underscore-prefixed private fields added 📘 Rule violation ⚙ Maintainability
The newly introduced private fields use a leading underscore (e.g., _preFilterExpandedObjects), which violates the required camelCase naming convention for non-public fields. This reduces consistency with the enforced style rule and can cause style-check failures.
Agent Prompt
## Issue description
New non-public fields were added with leading underscores, which violates the non-public field naming rule.
## Issue Context
The rule requires non-public field names to be camelCase with no leading underscores.
## Fix Focus Areas
- mRemoteNG/UI/Controls/ConnectionTree/ConnectionTree.cs[38-39]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+176
to
+178
| [Test] | ||
| public void RemoveFilter_RestoresPreFilterExpansionState() | ||
| { |
There was a problem hiding this comment.
2. New tests violate underscore pattern 📘 Rule violation ▣ Testability
The newly added test method names use only two underscore-separated segments instead of the required three-part MethodName_Scenario_ExpectedBehavior pattern. This can break consistency and any tooling that enforces/depends on the naming convention.
Agent Prompt
## Issue description
New NUnit tests were added whose method names do not follow the required `MethodName_Scenario_ExpectedBehavior` naming pattern.
## Issue Context
The rule requires exactly three non-empty segments separated by single underscores.
## Fix Focus Areas
- mRemoteNGTests/UI/Controls/ConnectionTreeExpansionTests.cs[176-219]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+179
to
+182
| RunWithMessagePump(tree => | ||
| { | ||
| var model = new ConnectionTreeModel(); | ||
| var root = new RootNodeInfo(RootNodeType.Connection); |
There was a problem hiding this comment.
3. Ui tests lack message pump 📘 Rule violation ▣ Testability
The newly added UI tests rely on RunWithMessagePump, but that helper does not start a WinForms message loop via Application.Run(...) nor ensure Application.ExitThread() in a finally/cleanup path. This can lead to flaky or invalid UI test execution for ObjectListView/TreeListView-based controls.
Agent Prompt
## Issue description
Tests that exercise `ConnectionTree` (TreeListView/ObjectListView-based UI) must run under a real WinForms message loop. The current `RunWithMessagePump` helper uses `form.Show()` on an STA thread without `Application.Run(form)` and without guaranteed `Application.ExitThread()` cleanup.
## Issue Context
The compliance rule requires the RunWithMessagePump pattern: `Application.Run(form)` and a try/finally that calls `Application.ExitThread()`.
## Fix Focus Areas
- mRemoteNGTests/UI/Controls/ConnectionTreeExpansionTests.cs[16-51]
- mRemoteNGTests/UI/Controls/ConnectionTreeExpansionTests.cs[176-259]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Pull request overview
Fixes a regression in the Connections tree where clearing a search filter could restore an incorrect expansion state and leave the virtual row list out of sync (leading to expand/collapse inconsistencies and potential invalid-index errors in visibility/selection operations).
Changes:
- Materialize the pre-filter expansion state into a real snapshot (
List<object>) beforeExpandAll()mutates the underlying expansion map. - When clearing the filter, rebuild the tree via
RebuildAll(...)to restore expansion + selection + row layout in a consistent pass. - Batch UI updates during filter removal to avoid redundant repaints and repeated column auto-resize work.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| mRemoteNG/UI/Controls/ConnectionTree/ConnectionTree.cs | Fixes expansion snapshotting and uses RebuildAll + update batching when clearing the search filter. |
| mRemoteNGTests/UI/Controls/ConnectionTreeExpansionTests.cs | Adds regression tests covering expansion-state restoration and row-layout rebuild correctness after clearing a filter. |
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.
Problem
Filtering the Connections tree with the search box and then clearing it leaves the tree broken: folders that were expanded before the filter come back collapsed, expand/collapse gets out of sync with what the tree reports, and in one case
EnsureVisiblethrew on an invalid index.Root cause
Two compounding defects in
ConnectionTree.RemoveFilter():1. The "pre-filter" snapshot was not a snapshot.
ApplyFilterstoredExpandedObjects, whose getter returns a liveDictionary.KeyCollectionover the tree model's expansion map (TreeListView.cs:317) rather than a copy. TheExpandAll()immediately after rewrote that same map, so the saved state became the filtered state. TheExpandedObjectssetter then doesClear()before enumerating the value it was given — the very collection it just emptied — soRemoveFilterrestored nothing and every folder collapsed.2. The display was never rebuilt. Assigning
ExpandedObjectsonly updates the model's map; OLV documents thatRebuildAllmust follow.RemoveFilterinstead rebuilt first (viaResetColumnFiltering) and mutated the map afterwards, leaving the branch structure and row list on the filtered layout. Expansion state and row indexes then read from different sources, which is howIndexOfhandedEnsureVisiblea row index past the end of the list.Fix
_preFilterExpandedObjectsis materialized into aList<object>so it survives theExpandAll().RemoveFiltercallsRebuildAll(SelectedObjects, _preFilterExpandedObjects, null), restoring expansion, selection and the row list in one consistent pass.Follow-up commit: batching
Clearing the filter runs three passes —
UseFilteringand the column filter reset each triggerUpdateFiltering, then the tree rebuild above. Each pass repainted and re-ranAutoResizeColumn, which measures the text of every visible row. The second commit holds painting across the whole method and collapses the three auto-resizes into one against the restored rows.Note
Freeze()/Unfreeze()is not usable here: theif (Frozen) return;guard lives inObjectListView.BuildList(bool), butVirtualObjectListViewoverrides that method without the guard, andTreeListViewis a virtual list. Freezing would suspend painting while adding one moreBuildListinDoUnfreeze.Tests
Two regression tests in
ConnectionTreeExpansionTests, both verified to fail against the unfixed code:RemoveFilter_RestoresPreFilterExpansionStateExpected: True / But was: False— root and Folder1 came back collapsedRemoveFilter_RebuildsRowsToMatchRestoredExpansionStateExpected: 4 / But was: 5— rows kept the filtered layoutFull suite on this branch: 6343 passed, 2 failed. The failure (
StartupConnectionPathReturnsSavedPathWhenItIsTheSoleCandidate, counted twice by overlapping groups) is pre-existing and unrelated — connections-file discovery picking up a candidate next to the test binary on the dev box; it fails in isolation on an unmodified tree too.