Repository navigation
Add F11 Detach Mode - #749
mfkeeq199458 wants to merge 7 commits into
Conversation
F11 now detaches the active log window into its own top-level window and hides the main window; F11 or ESC returns to the normal mode. The float window gets min/max buttons, a resizable border, a taskbar button, its own stable icon and a maximizable double-click-title behaviour; dark title bars are re-applied after the style-driven handle recreation. TabController gains GetPlacedWindows/GetNextWindow/GetPreviousWindow and a guarded CloseAllExcept; the View menu offers the toggle as "Log Only View" (en/de/zh-CN) with F11 as a display-only shortcut hint. DetachMode service, message filter and a NativeWindow caption hook live in DetachModeService; 17 controller tests plus 6 filter tests cover it.
Hirogen
left a comment
There was a problem hiding this comment.
Thanks @mfkeeq199458 — a focus mode for a single log is a nice idea, and it's good that F11 doesn't clash with anything and that closing the app while detached leaves the saved main-window layout intact. Before this can go in, there are a few behavioural issues, plus some housekeeping.
ESC handling (blocking)
The filter catches ESC for the whole app, and the text-input guard doesn't work as intended:
Form.ActiveForm?.ActiveControl is TextBoxBase(DetachMessageFilter.cs:31) stops at the nearest nested container, so it sees theLogWindow, not the filter/search box or the in-grid edit control. ESC in those inputs therefore leaves detach mode instead of doing its normal job. A check likeGetFocus()→Control.FromHandle(...) is TextBoxBase(or walking theContainerControlchain) would find the real focused control.- ESC on the grid normally cancels a search and clears highlighting (
LogWindow.cs:6989). The filter's own comment says that should keep working, but while detached it exits the mode instead. - If a modal dialog is open (e.g. from the float window), ESC is swallowed: the dialog stays open and the main window reappears behind it. Please only react when the active form is the detached float window.
The ESC tests stub _isTextInputFocused, so they can't catch this. It needs a manual check, or a test that exercises the real focus lookup.
Restoring state on exit
Exitdocks the window again withShow(_dockPanel, DockState.Document)(DetachModeController.cs:107), so the original tab position and any split pane are lost. Please restore the original pane and index.- If the window was already floating before F11,
Exitleaves the patched chrome on it (no owner, taskbar button, sizable border).ReleaseFloatWindowonly removes the hook. _detachModeControlleris never disposed inLogTabWindow.Dispose, and the message filter is only removed whenExit()runs. Please make the controllerIDisposableand dispose it next to_tabController.
Scope
- "Close other tabs" now goes through
CloseAllExceptand asks for confirmation, where before it closed directly. That's a user-visible change detach mode doesn't need. Please split it into its own PR (or drop it). - Ctrl+Tab cycling, "opening a file replaces the detached window" and "X on the float detaches the next hidden window" are reasonable, but they aren't in the description. Please document them. I'd also argue the X button should simply return to the main window; that's what most users will expect.
Housekeeping
- The PR adds 10 new analyzer warnings (CA1060, CA5392, CA1051, CA1303, CA1001, CA1031). Please get the build back to the base warning count.
LogExpert.UIalready uses Vanara: please useVanara.PInvoke.User32forGetWindowLongPtr/CallWindowProc, and putDwmSetWindowAttributeinNativeMethods(or use Vanara.DwmApi) instead of newDllImports.WM_KEYDOWNand the autorepeat bit are defined in both the filter andLogTabWindow. The restore-dock-state block is repeated three times in the controller. Please extract both.ToggleDetachMode()only callsEnter(). Please rename it, or make it actually toggle.- Comments: we keep them minimal in this repo.
DetachModeControlleris about 20% comments, many of them multi-paragraph. Please trim them to the non-obvious "why" lines. - Naming: the code says "detach mode", the UI says "Log Only View". Please pick one concept name and use it throughout.
- Test seams: prefer constructor injection over mutable
internalfields. - The new blocks in
LogTabWindow.designer.cslook hand-edited (//without the trailing space). Please let the designer generate them.
Description
- The title says "Replace compact mode", but there's no compact mode on
Development. Please retitle to something like "Add F11 Log Only View". - The test count is off: the code has 9 controller + 8 filter + 6 TabController tests.
Thanks — the core idea is solid. ESC handling and restoring the original state on exit are the two I need fixed; the rest is cleanup.
Adds F11 detach mode: F11 hides the main window and keeps the active log window floating as a stand-alone viewer. F11 brings the main window back and restores the window to its original pane and tab index. Behaviour: - F11 enters and leaves the mode. ESC is not part of detach mode and keeps its normal behaviour (cancelling a search on the grid, editing in text boxes, dismissing dialogs). - Ctrl+Tab / Ctrl+Shift+Tab cycle through log windows while detached. - Opening a file while detached replaces the detached window. - Closing the detached float (X) ends the mode and returns to the main window. - A window that was already floating keeps its original chrome on exit; a docked window is restored to its original pane and tab position. - The keyboard filter only reacts while the detached float is the active form, so modal dialogs keep F11 and Ctrl+Tab for themselves. Review follow-up: - Removed the ESC exit path and the old text-input guard; the mode is left with F11 only. - Restore the original dock pane and content index instead of docking to DockState.Document. - Restore the patched chrome for windows that were already floating on entry. - Make DetachModeController IDisposable and dispose it next to TabController; the message filter is removed in Dispose as well. - Revert the "Close other tabs" change: it was not part of this feature and is now identical to Development. - Clear the new analyzer warnings: remove the unused GetFocus/SetFocus P/Invokes, add DefaultDllImportSearchPaths for the DWM call, and suppress CA1303, CA1031 and CA1001 with justifications. - Share the WM_KEYDOWN and autorepeat constants through NativeMethods and keep a single restore-dock-state block. - Rename ToggleDetachMode to EnterDetachMode, trim comments to the non-obvious "why", and use the concept name "Detach Mode" in code, designer text and all three resource files. - Use constructor injection for the keyboard filter test seams. Tests: 9 controller, 8 filter and 3 TabController tests. Full LogExpert.UI.Tests suite: 391 passed, 0 failed. Note: the previous description said "Replace compact mode", but Development has no compact mode; this change adds F11 Detach Mode. The earlier test count and the Ctrl+Tab, file-replacement and X-button behaviour were missing from the description and are documented above. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com>
Two detach mode regressions: - Closing the detached float through its X left an empty, unclosable tab in the main window. OnWindowDisposed fired while LogWindow.IsDisposed was still false, so Exit() re-showed the closing window into its original pane. The close path no longer calls Exit(): it releases the hook without touching the float chrome, clears the captured placement and shows the main window on the message loop after the suite finished closing, which keeps the document panel empty. - In dark mode a detached float kept a light title bar until it was moved or resized. DWM only repaints the caption on a real move/resize, so the immersive dark flag is applied after the float is shown and a one pixel height nudge forces the repaint. A maximized float only gets the flag because the nudge would overwrite its restore bounds. ApplyDarkTitleBar replaces SetImmersiveDarkTitleBar, Exit keeps restoring chrome for the F11 path, and ClearOriginalPlacement is shared by both exit paths. Tests: 18 DetachModeController tests pass, including a regression test that closing the float leaves no document in the panel; LogExpert.UI and LogExpert build without errors.
Hirogen
left a comment
There was a problem hiding this comment.
Thanks @mfkeeq199458 — both blocking items from last round are addressed. ESC is no longer touched, F11/Ctrl+Tab only react while the float is the active form (modal dialogs and text boxes are safe now), the original pane and index are restored, and the controller is IDisposable and disposed next to _tabController. Vanara, the NativeMethods move, the EnterDetachMode rename, the single RestoreWindow and the constructor-injected seams all look good too. "Close other tabs" now behaves exactly as on Development.
Behaviour (please fix)
- Dark title bar on exit:
RestoreChrome(DetachModeController.cs:~247) setsFormBorderStyle, which recreates the handle, and then only re-applies the dark flag, without the one-pixel nudge from 204c456. A window that was already floating before F11 will probably get a light caption again on exit. Also, a float that was maximized while detached staysMaximizedafter itsMaximizeBoxis removed. - Release order:
SwitchWindowandOnWindowAddedcallRestoreWindowbeforeDetach, andDetachis what callsReleaseFloatWindow. So the hook gets released from a float that may already be disposed.Exitdoes it in the right order: release first, then restore. - Ctrl+Tab order:
GetNextWindow/GetPreviousWindowwalk the_windowsdictionary, not the tab strip, but the doc comment says "tab order". Please use the visible tab order. - X button: X on the float now closes the log file. I said "return to the main window", which could also mean re-docking it. Was closing intentional? If so, please say so in the description.
Housekeeping
Resources.Designer.cs:2664-2672looks hand-edited:logOnlyViewsits beforelockInstance, and the blank lines are missing the generated indent. Please regenerate it from the resx.LogTabWindow.cs:2460-2461: please revert the whitespace-only re-indent.- One name: the UI says "Detach Mode", but the menu item and resx keys are
logOnlyView…. Either name is fine, as long as it's the same everywhere. Please also use it in the PR title. DetachFloatWindowHook: afterAssignHandle,base.WndProc(ref m)already forwards to the previous window procedure, so_originalProc,GetWindowProcand theCallWindowProccall can go._originalDockState/_originalPane/_originalContentIndexare reset inClearOriginalPlacementand again by hand at:172-173, which skips the index. Please use one path, e.g. a small record likeChromeSnapshot.DetachModeControllerTests.cs:27: please implementIDisposablelike the neighbouring fixtures instead of suppressing CA1001.- Comments: still a bit above our minimum. Examples: the XML doc on a private field (
LogTabWindow.cs:84-87), comments that restate the code (:855,:1497,:1830), the<remarks>added inTabController, and the "Arrange —" essays in the tests.
Tests
Exit_RestoresTheWindowAndTheMainFormonly checksDockState.Document. Please also assert the original pane and the content index, because that was the fix.Exit_KeepsAnAlreadyFloatingWindowFloatingshould assert that Owner, ShowInTaskbar and the border style are restored.
Description
The title and description still describe the first version: "Replace compact mode", "F11 or ESC", GetPlacedWindows, CloseAllExcept, and 17+6 tests (it's 18 controller + 3 TabController now). Please update both to match e78c47f/204c4561, including Ctrl+Tab, file replacement and the X behaviour.
Getting close. The title bar on exit and the release order are the two I'd like fixed. The rest is polish. Thanks!
- Restore the window state and dark title bar when leaving detach mode; show the main form before the float gets its owner back and re-dock before walking the tab order on Ctrl+Tab - Keep the main window visible when the suite cannot float a window - Simplify the caption hook to base.WndProc and drop the unused import - Regenerate Resources.Designer.cs from Resources.resx - Rename logOnlyView members and resources to detachMode
Hirogen
left a comment
There was a problem hiding this comment.
Thanks @mfkeeq199458 — round 3 fixes both behaviour items. The hook is released before restore in SwitchWindow/OnWindowAdded, and RestoreChrome re-applies the dark flag and then nudges, with WindowState set last. The snapshot records, the slimmer DetachFloatWindowHook, the stronger Exit tests, the reverted re-indent, the documented X behaviour and the consistent "Detach Mode" naming all look good. Build adds no warnings compared to Development (the 10 from round 1 are gone), and the 51 Detach Mode/TabController tests pass.
Before merge
-
Resources.Designer.cslocale. The file was regenerated, but by a Chinese-language VS 17: the header and ~760 doc comments are now in Chinese, andGeneratedCodeAttributewent from18.0.0.0to17.0.0.0. Please regenerate it with an English VS UI (Tools → Options → Environment → International Settings), ideally VS 2026/18. Heads-up: #746 has the same issue, and both PRs regenerate this file, so the second one to merge will need one more Run Custom Tool after rebasing. -
Exceptions halfway through a switch. If
Detachthrows insideSwitchWindoworOnWindowAdded, the main form stays hidden while_detachedWindowis still set, and the user can end up with no visible window. The same applies toEnterif something throws afterPatchFloatWindow. Atry/catch(orfinally) that falls back toLeaveDetachedState()and shows the main form would make this safe.
Small things
- Comments are close but not quite there. Please remove the XML docs on the private
IsAutoRepeat/LeaveDetachedState, the 3-line comment inProcessCmdKeythat repeats its doc, the extra sentence onGetNextWindow, the XML docs on theNativeMethodsconstants and the paragraph-long summary on the test class. In the tests,NativeMethods.WM_KEYDOWNis better than0x0100, // WM_KEYDOWN. - Ctrl+Tab in the normal main window now also follows tab-strip order (
SwitchToNextWindow→GetNextWindow). I think that's an improvement, but please mention it in the description, since it changes behaviour outside detach mode. - Description:
GetAllWindowsFromDockPanelalready exists on Development. And the icon that gets disposed and replaced comes from LogExpert's ownSetTabIcon, not the suite. Dispose→Exit()→_mainForm.Show()runs while the form is being disposed. It might be cleaner to skip the show path when disposing.
Optional
- The auto-repeat check exists both in
LogTabWindow.IsAutoRepeatand inline inDetachMessageFilter, and the "release → restore → detach-or-leave" sequence is repeated inSwitchWindow/OnWindowAdded. Small helpers would remove both duplicates. DwmSetWindowAttributeis now the onlyDllImportinsrc.Vanara.PInvoke.DwmApiwould remove it, together with the localWM_NCLBUTTONDBLCLK/VK_*ints (Vanara has enums for those).- Nullable annotations:
GetNextWindow(LogWindow current = null),_captionHookandDetachedFloatshould be nullable.
Please also do a quick manual check that a window which was already floating before F11 keeps its dark title bar after leaving detach mode. There's no test for that path.
With 1 and 2 done, I'm happy to merge. Thanks — this turned out really well!
- Catch a throw inside Detach() and roll back to a visible window: the detached window is cleared first, so Enter() cannot keep believing the mode is on with no float, and a window that was already floated returns to its place - Guard SwitchWindow/OnWindowAdded as a whole; ReleaseAndRestore and the tab walk also run with the main form hidden, so nothing may escape them - Skip showing the main form from RestoreMainForm when it is being disposed - Share the auto-repeat check through NativeMethods.IsAutoRepeat, and use the Vanara User32 enums for WM_NCLBUTTONDBLCLK / VK_F11 / VK_TAB. DwmSetWindowAttribute stays a local DllImport: the generated Vanara one carries no search-path restriction and dwmapi.dll is not a KnownDLL - Drop the XML docs and duplicated comments the review called out
|
@Hirogen — round 3 is in (868a51c). Behaviour and the small items are addressed:
On the Vanara suggestion for One favour: Local verification: |
Hirogen
left a comment
There was a problem hiding this comment.
Thanks @mfkeeq199458. Round 4 covers nearly all of the "Small things": the comment trims, NativeMethods.WM_KEYDOWN in the tests, the shared IsAutoRepeat, the ReleaseAndRestore helper, Vanara enums instead of the local VK_*/WM_NCLBUTTONDBLCLK ints, skipping the show on Dispose, and the Ctrl+Tab/description notes. The justification on the remaining DwmSetWindowAttribute import is fine with me.
Still open from last round
-
Resources.Designer.cslocale. Not done yet. The header and doc comments are still Chinese andGeneratedCodeAttributeis still17.0.0.0. Please run Custom Tool once with an English VS UI. This is the one item that blocks the merge. -
Exception safety. This is mostly there, but a few gaps remain:
- Partial patch is never rolled back.
Detach()callsPatchFloatWindow(floatWindow)beforeDetachFloatWindowHook.Attach(...). IfAttachthrows,_captionHookis still null, soReleaseFloatWindow()inRollBackFailedDetachdoes nothing and the original chrome is never restored. For a window that was already floating,RestoreWindowreturns early onDockState.Float, so it keeps the detach chrome for good. Either attach the hook first, or callRestoreChrome(_originalChrome)directly in the rollback. Entercatches too little. It only catchesObjectDisposedException or InvalidOperationException. Anything else fromInstallFilter()/Hide()escapes with the mode still on, which is the exact state the comment there warns about.SwitchWindow/OnWindowAddedcatch everything for the same reason, so please use one policy.- The fallbacks can throw.
LeaveDetachedState()in theSwitchWindow/OnWindowAddedcatch blocks, andExit()(called from the filter on F11 and fromLoadSession), aren't protected themselves. I'd accept a best-effort "show the main form" that can't throw, e.g. in afinally. - Tests. There's one rollback test, and it uses a disposed target, so it never checks where a window goes after a failed detach. Please add at least the
OnWindowAddedrollback and the "throw after the chrome was patched" case.
- Partial patch is never rolled back.
-
Nullable annotations.
GetNextWindow/GetPreviousWindow(LogWindow current = null)inITabController/TabControllerare unchanged. InDetachModeController, also:_detachedWindow, theFindFloatWindowreturn type,PlacementSnapshot.Pane,ChromeSnapshot.Owner, theRestoreWindow/ReleaseAndRestoreparameters andToggleMaximize(object? sender, …).DetachMessageFilter'sFunc<Form>should beFunc<Form?>.
Small things
- The comment in the
SwitchWindowcatch repeats the CA1031Justificationword for word. Please drop it. The two-line comments inTryDetachandRollBackFailedDetachcan be one line each. SwitchWindowandOnWindowAddedstill share the try → release → detach → leave shape, and the CA1031 suppression appears four times. ASwitchTo(LogWindow target)helper would collapse both.DetachModeKeys_MatchTheWin32Valuesonly tests Vanara's own constants, so I'd remove it. Most filter tests still use(IntPtr)0x7A // VK_F11, while the rest of the file usesUser32.VK.VK_F11.DisposecallsReleaseFloatWindow()andUninstallFilter()again afterExit()has already done both.- Description: it's 21 controller tests, not 22.
With the Designer.cs regeneration and the partial-patch rollback fixed, I'm happy to merge. The rest can be a quick follow-up if you'd rather not do another round. Thanks for sticking with this!
|
You can keep the Designer File I will regenerate it, after the merge :), just the partial-patch rollback fix would be awesome, thx for all the hard work |
- Take the chrome snapshot before the first style change and restore it unconditionally in RollBackFailedDetach, so a throw from Attach() no longer leaves an already floating window with the detach chrome for good - Restore the chrome and the placement independently: a failure in one no longer skips the other - Use one exception policy across Enter, Exit, LeaveDetachedState, OnWindowDisposed and RestoreMainForm; Exit() takes restoreMainForm so FormClosing does not show the hidden form again - Collapse SwitchWindow/OnWindowAdded into SwitchTo; the target is resolved through a delegate because the tab walk has to run after the float was released - Only swallow F11 when Enter() succeeded, and put the minimized/maximized state back when it did not - Annotate the nullable members of ITabController/TabController, DetachModeController and DetachMessageFilter - Replace the disposed-target rollback test with real ones: SwitchWindow and OnWindowAdded rollback, the chrome restore of an already floating window, and Exit(restoreMainForm: false); drop DetachModeKeys_MatchTheWin32Values and use User32.VK in the filter tests - Dispose() now only calls Exit()
|
Thanks @Hirogen — round 5 is in as Partial-patch rollback The chrome snapshot is now taken before the first style change, and Exception policy
SwitchTo
Tests 23 controller tests now (the disposed-target case is gone): added Small things
Resources.Designer.cs Left untouched, as you suggested. It is still the zh-CN / One item beyond your list, so it is not a surprise: F11 is only swallowed when |
Hirogen
left a comment
There was a problem hiding this comment.
Thanks @mfkeeq199458 — round 5 closes both merge blockers. The partial-patch rollback now restores the chrome even when Attach() throws and the window was already floating, and the rollback tests check where the window actually ends up. Enter/Exit follow one exception policy, all the nullable annotations are in, Dispose only calls Exit(), and the 55 Detach Mode/TabController tests pass with no new build warnings. I'll regenerate Resources.Designer.cs after merging, as agreed.
A few things for a follow-up, none of them blocking:
Logging
- The empty
catchblocks inDetachModeController(about 15 now) swallow failures without a trace. Please log them through NLog like the neighbouring services do, e.g._logger.Warn(ex, "..."). Then a "best effort" step that fails is still visible in the log. - Round 5 removed the existing
_logger.Error(ex, "Leaving detach mode while closing failed")fromOnLogTabWindowFormClosing. The method-level CA1031 suppression added there also covers the shutdowncatchthat was already on Development. Please put the log line back and keep the suppression on the code that needs it.
Duplication
- The CA1031 suppressions went from 4 to 8, and the same try/catch shape appears about 15 times. A small
TryBestEffort(Action step, string what)helper (which would also be the one place to log) would remove most of it.LeaveDetachedStateandRollBackFailedDetachalso end with the same release → restore → clear sequence. TabController.GetNextWindow/GetPreviousWindowdiffer only in direction. OneStep(current, ±1)would do.
Comments
- The comment count roughly doubled this round (39 → 71 lines in the controller). Most catch blocks have their own comment, two are copied word for word (175/296 and 188/318), and new XML docs appeared on the private
SwitchToandEnterDetachMode. TheIsAutoRepeattext I asked you to drop in round 3 is back as a//comment.
Small things
DWMWA_USE_IMMERSIVE_DARK_MODEsits between methods in the controller. It belongs inNativeMethods, next toDwmSetWindowAttribute.- Tests:
(IntPtr)(1L << 30)→NativeMethods.KEYDOWN_AUTOREPEAT_FLAG. DetachFloatWindowHook._floatWindowplus its pass-through property can be an auto-property.RestorePatchedChromevs.RestoreChrome: the names don't say how they differ.- The PR description could mention that F11 now reaches the normal key handling when entering the mode fails.
I'm happy to merge this as it is. Thanks for sticking with it through five rounds — it turned out really well!
F11 detaches the active log window into its own top-level window and hides the main window; pressing F11 again returns to the normal layout. The View menu offers the same toggle as "Detach Mode" with F11 shown as a shortcut hint.
While detached
SetTabIconand its handle is replaced on every tail refresh, so the float takes the main window's long-lived icon instead. The dark title bar is re-applied after the style-driven handle recreation and the caption is repainted with a one-pixel nudge; the original chrome and window state are restored on exit, including for windows that were already floating.TabController.GetNextWindow/GetPreviousWindow). This also changes the normal main window:SwitchToNextWindow/SwitchToPreviousWindownow follow the tab strip instead of registration order.Keyboard handling
A message filter serves the keys while the main form is hidden. It only reacts when the detached float is the active form, so modal dialogs and text boxes keep F11 and Ctrl+Tab for themselves; the auto-repeat burst of F11 is swallowed so the mode does not toggle on every repeat.
Restoring
Leaving the mode shows the main form first (assigning it as the owner of a still-hidden form would hide the float before its content is docked back), then releases the caption hook, re-docks the window into its original pane and tab index and restores the chrome and window state. The window state is applied last, so a window that was maximized before detach comes back maximized even when the suite's chrome has no maximize box.
If the suite refuses to float a window, the mode is not entered at all: the main window stays visible. A throw halfway through a detach is handled the same way: the detached window is cleared, the float is released and the window goes back to its place before the mode ends, so a switch or a newly opened window can never leave the main form hidden without a float.
Naming
The menu item and its en/de/zh-CN resources are called "Detach Mode" everywhere (
detachModeToolStripMenuItemwith matching resource keys).Resources.Designer.csis generated fromResources.resx; the current file still needs one regeneration with an English VS UI, because it was produced by a zh-CN VS 17 and carries 17.0.0.0 inGeneratedCodeAttribute.GetAllWindowsFromDockPanelalready exists on Development; this PR only trims its remarks.TabControllergains the tab-order-aware next/previous lookup. The detach mode controller, message filter and caption hook live inDetachModeService. Tests: 23 controller tests (8 of them drive the message filter) plus 4 newTabControllertests.