Skyline: Extended keyboard shortcut help #3626
Conversation
brendanx67
commented
Oct 4, 2025
brendanx67
commented
- Added keyboard help for grids
- Replaced deprecated WebBrowser (IE-based) with WebView2 (Chromium based)
- Enabled DocumentationViewer test in StandardTypeTest
- Added keyboard help for grids - Replaced deprecated WebBrowser (IE-based) with WebView2 (Chromium based) - Enabled DocumentationViewer test in StandardTypeTest
|
I have run the tests that bring up the DocumentationViewer many hundreds of times in all languages without failure. Though, this level of consistency was not easy to achieve, while still testing that the HTML shows up in the browser. |
|
|
||
| private void RunUI(Action act) | ||
| { | ||
| Invoke(act); |
There was a problem hiding this comment.
I would recommend using "CommonActionUtil.SafeBeginInvoke" here because it handles the case where the form has already been closed.
If it were important that you use "Invoke" instead of "BeginInvoke" then you would not want to use SafeBeginInvoke, but I don't think we care in this case.
There was a problem hiding this comment.
Sure. I also changed the function name to RunUIAsync to be clearer about what it is doing.
| Invoke(act); | ||
| } | ||
|
|
||
| private void CleanupTestDataFolder() |
There was a problem hiding this comment.
Can you move this code into "DocumentationViewerHelper"?
There was a problem hiding this comment.
Cursor/Claude left things a bit messy. I moved the DocumentationViewHelper the IDisposable I asked the LLM agent to create out to AbstractFunctionalTestEx fairly late in the process, and clearly didn't move everything I could have to it. After moving a bunch more, I was also able to get rid of the extra debug flags on the WebView2 environment. Much cleaner and simpler now without the complicating need of trying to clean up the test environment in Form_Closed(), and it ran thousands of times without a failure. The test code is now responsible for creating and deleting the environment temp folder, and tries the folder deletion in a WaitForCondition() loop.
| public class KeyboardShortcutHelpTest : AbstractFunctionalTestEx | ||
| { | ||
| [TestMethod, NoParallelTesting(TestExclusionReason.WEB_BROWSER_USE)] | ||
| public void TestKeyboardShortcutHelp() |
There was a problem hiding this comment.
I wonder if we should have two different [TestMethod]'s in this class: "TestKeyboardShortcutHelp" and "TestDebugKeyboardShortcutHelp" where only the "TestDebug" method sets "TestWebView2EnvironmentDirectory" and is able to assert more things about the HTML content.
The regular "TestKeyboardShortcutHelp" might not be able to assert as many things but would at least be able to verify that nothing bad happens when WebView2 is invoked without all those debug arguments.
There was a problem hiding this comment.
Not in this PR. Willing to consider a PR from you later that improves on what I have done.
…teoWizard/pwiz into Skyline/work/20250930_keyboard_extra
| DocumentationViewer.TestWebView2EnvironmentDirectory = testContext.GetTestResultsPath(@"WebView2"); | ||
| Directory.CreateDirectory(DocumentationViewer.TestWebView2EnvironmentDirectory); | ||
|
|
||
| DocViewer = ShowDialog<DocumentationViewer>(SkylineWindow.ShowKeyboardShortcutsDocumentation); |
There was a problem hiding this comment.
The passed-in Action "showViewer" should be used instead of SkylineWindow.ShowKeyboardShortcutsDocumentation
2 participants
