fix: close a PuTTY tab in one step, without PuTTY's own prompt - #158
Open
jafin wants to merge 1 commit into
Open
fix: close a PuTTY tab in one step, without PuTTY's own prompt#158jafin wants to merge 1 commit into
jafin wants to merge 1 commit into
Conversation
Closing a PuTTY session from the tab's X asked the user to confirm twice
and then left the tab behind:
- mRemoteNG posts WM_CLOSE to the PuTTY window, which answers with its own
warn-on-close box ("Are you sure you want to close this session?") on top
of the confirmation mRemoteNG had already shown. The graceful close now
polls while waiting for the process to exit and acknowledges that box on
the user's behalf. The match is deliberately narrow - owning process,
dialog class #32770 and an "Exit Confirmation" title - so the host key
Security Alert and the settings dialog stay under user control.
- KeepTabsOpenAfterDisconnect (on by default) cancelled the form close for
every close path, so an explicitly closed tab stayed in place showing the
reconnect panel. That option now applies only to a disconnect request:
the tab context menu's Disconnect entry sets the new disconnectOnly flag,
while the tab's X and Ctrl+W always take the tab down. HandleProtocolClosed
no longer revives a tab that is already disposing.
PR Summary by QodoFix PuTTY tab close double-prompt and ensure tab closes on explicit close
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
There was a problem hiding this comment.
Pull request overview
This PR improves tab/session closing behavior by (1) auto-dismissing PuTTY’s warn-on-close dialog after mRemoteNG has already confirmed the disconnect, and (2) ensuring “Keep tabs open after disconnect” only preserves tabs for explicit disconnect actions (not tab close).
Changes:
- Update PuTTY graceful-close logic to detect and acknowledge PuTTY’s “Exit Confirmation” dialog during shutdown polling.
- Adjust tab close vs. disconnect behavior via a new
disconnectOnlyflag and prevent closed-state UI from reviving a tab that’s already closing. - Add STA, headless unit tests covering tab close/disconnect permutations and
PuttyBase.IsPuttyExitConfirmation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| mRemoteNGTests/UI/Tabs/ConnectionTabCloseTests.cs | Adds tests validating tab-close vs disconnect-only behavior under KeepTabsOpenAfterDisconnect. |
| mRemoteNGTests/Connection/Protocol/PuttyBaseTests.cs | Adds tests for the “Exit Confirmation” window predicate used to auto-dismiss PuTTY’s warn-on-close box. |
| mRemoteNG/UI/Window/ConnectionWindow.cs | Marks Disconnect-menu closes as disconnect-only and avoids reviving already-closing tabs with the closed-state panel. |
| mRemoteNG/UI/Tabs/ConnectionTab.cs | Applies KeepTabsOpenAfterDisconnect only for disconnect-only closes. |
| mRemoteNG/Connection/Protocol/PuttyBase.cs | Polls for graceful exit and auto-acknowledges PuTTY’s warn-on-close confirmation dialog. |
| mRemoteNG/App/NativeMethods.cs | Adds P/Invoke for GetWindowText used by the PuTTY dialog detection. |
Comment on lines
+1845
to
1846
| selectedTab.disconnectOnly = true; | ||
| selectedTab.Close(); |
Code Review by Qodo
1. disconnectOnly never cleared
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| /// showing the reconnect panel when KeepTabsOpenAfterDisconnect is enabled. | ||
| /// Closing the tab itself always removes the tab, regardless of that option. | ||
| /// </summary> | ||
| public bool disconnectOnly { get; set; } |
There was a problem hiding this comment.
1. disconnectonly property not pascalcase 📘 Rule violation ⚙ Maintainability
A new public property disconnectOnly is introduced with a non-PascalCase name. This violates the required naming convention for properties and reduces consistency/readability across the API surface.
Agent Prompt
## Issue description
A new public property `disconnectOnly` was added using camelCase, but property names must be PascalCase.
## Issue Context
This property is referenced from production code and tests, so renaming requires updating all call sites.
## Fix Focus Areas
- mRemoteNG/UI/Tabs/ConnectionTab.cs[33-39]
- mRemoteNG/UI/Window/ConnectionWindow.cs[1845-1847]
- mRemoteNGTests/UI/Tabs/ConnectionTabCloseTests.cs[72-96]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| } | ||
|
|
||
| [Test] | ||
| public void ClosingTheTabClosesItEvenWhenTabsAreKeptOpenAfterDisconnect() |
There was a problem hiding this comment.
2. Test names missing underscores 📘 Rule violation ▣ Testability
New/modified test methods do not follow the required MethodName_Scenario_ExpectedBehavior naming pattern. This reduces consistency and makes tests harder to scan/search by convention.
Agent Prompt
## Issue description
Several newly added/modified NUnit test methods do not follow the `MethodName_Scenario_ExpectedBehavior` naming convention (exactly 3 segments separated by 2 underscores).
## Issue Context
This convention is required for all test methods in the changed files.
## Fix Focus Areas
- mRemoteNGTests/UI/Tabs/ConnectionTabCloseTests.cs[59-96]
- mRemoteNGTests/Connection/Protocol/PuttyBaseTests.cs[129-143]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1845
to
1846
| selectedTab.disconnectOnly = true; | ||
| selectedTab.Close(); |
There was a problem hiding this comment.
3. Disconnectonly never cleared 🐞 Bug ≡ Correctness
ConnectionTab.disconnectOnly is set on the Disconnect menu path but is never reset, so when the same tab is later reused for reconnect, closing the tab via X/Ctrl+W can still be treated as “disconnect-only” and the close gets cancelled again when KeepTabsOpenAfterDisconnect is enabled.
Agent Prompt
## Issue description
`disconnectOnly` is a persistent per-tab flag that is set to `true` when the user chooses **Disconnect** from the tab context menu. The PR uses this flag to decide whether to cancel `OnFormClosing` (keeping the tab open with the reconnect panel).
However, the flag is never reset to `false`. Because tabs can be *reused* for reconnect, the stale `disconnectOnly=true` can leak into subsequent sessions in the same tab and cause normal tab closes (X/Ctrl+W) to be cancelled again.
## Issue Context
- Disconnect menu sets `disconnectOnly=true` and calls `Close()`.
- `ConnectionTab.OnFormClosing` uses `disconnectOnly && KeepTabsOpenAfterDisconnect` to decide whether to cancel the close.
- Reconnect/open paths reuse closed tabs (`GetOrAddConnectionTab`), but do not clear `disconnectOnly`.
## Fix Focus Areas
- mRemoteNG/UI/Tabs/ConnectionTab.cs[198-246]
- mRemoteNG/UI/Tabs/ConnectionTab.cs[258-265]
- mRemoteNG/UI/Window/ConnectionWindow.cs[858-876]
### Concrete fix approach (one acceptable option)
1. Make `disconnectOnly` a **one-shot** flag:
- In `ConnectionTab.OnFormClosing`, capture `bool keepOpen = KeepTabOpenAfterDisconnect;` early.
- Immediately reset `disconnectOnly = false;` once a disconnect-only close attempt is being handled.
- Use the captured `keepOpen` to set `e.Cancel`.
2. Additionally (defensive), when reusing a closed tab in `ConnectionWindow.GetOrAddConnectionTab`, clear any stale close flags:
- `reusableTab.disconnectOnly = false;`
- (Optional) also clear `silentClose`/`protocolClose` if those can persist across reuse.
3. Add/extend a test:
- Disconnect via menu (sets `disconnectOnly`), then simulate tab reuse for reconnect, then close via X and assert `Cancel == false` even when KeepTabsOpenAfterDisconnect is enabled.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
Closing a PuTTY session from the tab's X asked the user to confirm twice, and the tab stayed behind afterwards.
The double prompt
TryClosePuttyGracefullypostsWM_CLOSE, and PuTTY answers that with its own warn-on-close box ("Are you sure you want to close this session?") — on top of the confirmation mRemoteNG had just shown. The wait for the process to exit now polls every 50 ms (same 1 s budget as before) and acknowledges that box on the user's behalf.The match is deliberately narrow — owning process, dialog class
#32770, and an "Exit Confirmation" title — so PuTTY's host key Security Alert and the settings dialog are never auto-answered. That predicate is extracted asPuttyBase.IsPuttyExitConfirmationand covered by tests.Side effect: PuTTY now normally exits on its own instead of being killed after the 1 s timeout.
The tab that would not close
KeepTabsOpenAfterDisconnect(on by default) cancelled the form close on every close path, so an explicitly closed tab stayed put showing the reconnect panel. That option now applies only to a disconnect request:disconnectOnlyflag → tab stays, reconnect panel shows (Connect button (see attached image) does not work #61 behaviour, unchanged)HandleProtocolClosedalso no longer revives a tab that is already disposing.Tests
New
ConnectionTabCloseTestsdrivesConnectionTab.OnFormClosingwith a stub protocol (headless — confirmation setting forced to Never) across all three combinations of close path and setting, plus 6 cases forIsPuttyExitConfirmationinPuttyBaseTests.Full suite: 6379/6381. The 2 remaining failures are one pre-existing environment-dependent test (
StartupConnectionPathReturnsSavedPathWhenItIsTheSoleCandidate, counted in two groups) that fails identically on a clean tree — aconfCons.xmlnext to the test DLL wins over the temp path the test sets.