Adding auto-UI shortcuts to menu based on keymappings by timheuer · Pull Request #924 · microsoft/terminal

timheuer · 2019-05-21T20:55:28Z

Summary of the Pull Request

This modifies the UI menu to display the shortcuts for those options based on the new keybinding mappings from #748.

References

Enhances #748 to respond to custom keybinding mappings

PR Checklist

Closes Surface keyboard shortcuts in Menu when shown #791
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Requires documentation to be updated
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Added a few static methods to AppKeyBindings to find a keybinding from the loaded map and create a keyboard UI shortcut for it.
Two static helpers exist to map Terminal KeyChord values to WinRT VirtualKey and VirtualKeyModifiers. These should be removed/not needed once KeyChord should just use the existing WinRT types instead of defining our own #877 is decided on (assuming it would go to WinRT APIs).
Special casing the comma character due to a bug in the XAML UI framework that is not yet resolved and on backlog for WinUI 3.0 roadmap (noted in comments)

… requried.

…rosoft#683

…have shortcuts)

…UM) on using 'Ctrl' instead of 'Control'

…icrosoft#791

DHowett-MSFT · 2019-05-21T21:26:46Z

Hey @timheuer,
Thanks for the contribution!
Would you mind sharing a screenshot? This is exciting!

src/cascadia/TerminalApp/App.cpp

src/cascadia/TerminalApp/AppKeyBindings.cpp

timheuer · 2019-05-21T21:30:28Z

Sure thing here you go. These are all generated from the keybindings in profiles.json.

DHowett-MSFT

I'm comfortable with this, and it's really cool too

src/cascadia/TerminalApp/App.cpp

src/cascadia/TerminalApp/AppKeyBindings.cpp

zadjii-msft

This is really cool, great work

zadjii-msft · 2019-05-22T17:08:59Z

/azp run

DHowett-MSFT · 2019-05-22T17:51:06Z

/azp run

zadjii-msft · 2019-05-22T18:22:32Z

@miniksa do you know why this test keeps failing?
https://dev.azure.com/ms/Terminal/_build/results?buildId=16264


Passed   BasicWriteConsoleOutputATest
Win32BoolSucceeded(SetConsoleWindowInfo(consoleOutputHandle, true, &window)) - Value (6)
Failed   WriteConsoleOutputWOutsideBuffer
Error Message:
 Win32BoolSucceeded(SetConsoleWindowInfo(consoleOutputHandle, true, &window)) - Value (6)
Stack Trace:
at OutputTests::WriteConsoleOutputWOutsideBuffer() in d:\a\1\s\src\host\ft_host\api_outputtests.cpp:line 140
Standard Output Messages:
 Win32BoolSucceeded(CloseHandle(_hConsole)): Removing our test screen buffer.
 Restoring v1/v2 console state to original '1'
 Win32BoolFailed(RegSetValueExW(_consoleKey.get(), pwszForceV2ValueName, 0, REG_DWORD, (LPBYTE)&_dwForceV2Original, sizeof(_dwForceV2Original)))
 AreEqual(ERROR_SUCCESS, lstatus)
 Backing up v1/v2 console state.
 Win32BoolFailed(lstatus): Assert querying ForceV2 key was successful.
 Setting v1/v2 console state to desired '1'
 Win32BoolFailed(RegSetValueExW(_consoleKey.get(), pwszForceV2ValueName, 0, REG_DWORD, (LPBYTE)&ForceV2StateDesired, sizeof(ForceV2StateDesired)))
 SUCCEEDED(RuntimeParameters::TryGetValue(L"TestDeploymentDir", value))
 SUCCEEDED(StringCchCopyW(str, cchNeeded, (WCHAR*)value.GetBuffer()))
 Win32BoolSucceeded(nullptr != hJob)
 Win32BoolSucceeded(SetInformationJobObject(hJob.get(), JobObjectExtendedLimitInformation, &JobLimits, sizeof(JobLimits)))
 Win32BoolSucceeded(CreateProcessW(nullptr, str, nullptr, nullptr, FALSE, CREATE_NEW_CONSOLE | CREATE_SUSPENDED, nullptr, nullptr, &si, pi.addressof()))
 Win32BoolSucceeded(AssignProcessToJobObject(hJob.get(), pi.hProcess))
 Win32BoolSucceeded(-1 != ResumeThread(pi.hThread))
 IsLessThan(dwTotalWait, _dwMaxMillisecondsToWaitOnStartup)
 IsNotNull(pPidList)
 Win32BoolSucceeded(QueryInformationJobObject(hJob.get(), JobObjectBasicProcessIdList, pPidList, cbRequired, nullptr))
 AreEqual(pids.NumberOfAssignedProcesses, pPidList->NumberOfProcessIdsInList)
 AreNotEqual(0u, dwFindPid)
 Win32BoolSucceeded(FreeConsole())
 Win32BoolSucceeded(AttachConsole(dwFindPid))
 AreEqual(0, err)
 AreEqual(0, err)
 AreNotEqual(_hConsole, INVALID_HANDLE_VALUE): Creating our test screen buffer.
 Win32BoolSucceeded(SetConsoleActiveScreenBuffer(_hConsole)): Applying test screen buffer to console
 ***********	WriteConsoleOutputWOutsideBuffer - Test Start		***********
 
 Win32BoolSucceeded(SetConsoleWindowInfo(consoleOutputHandle, true, &window)) - Value (6)
 
 ***********	WriteConsoleOutputWOutsideBuffer - Test End		***********
 ***********	WriteConsoleOutputWOutsideBuffer - Test Failed	***********

azure-pipelines · 2019-05-22T18:49:49Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2019-05-22T19:03:07Z

Azure Pipelines successfully started running 1 pipeline(s).

miniksa · 2019-05-23T17:16:21Z

@zadjii-msft, no idea, file an issue.

timheuer added 14 commits May 8, 2019 08:45

Adding vsconfig file for VS2019 help to prompt for missing components…

e1fb796

… requried.

Merge remote-tracking branch 'upstream/master'

56ce0da

Adding a keybinding for launching the settings. Suggested fix for mic…

c5cadf7

…rosoft#683

merged with updstream and resolved conflict

f00e8ce

Merge remote-tracking branch 'upstream/master'

8833deb

Modified to comma per PR feedback

bd0c8e9

Merge remote-tracking branch 'upstream/master'

06a053d

Merge remote-tracking branch 'upstream/master'

bd46e67

Merge remote-tracking branch 'upstream/master'

12e2d2f

Implements 791 for profile and settings shortcuts (most frequent and …

7cd78fe

…have shortcuts)

Merge remote-tracking branch 'upstream/master'

57f1225

Quick change for consistency (missed in first checkin due to using EN…

465c15d

…UM) on using 'Ctrl' instead of 'Control'

Merge remote-tracking branch 'upstream/master'

f0acd8b

Adding UI shortcut generation to new keybinding mappings. Resolving m…

3a75c74

…icrosoft#791

DHowett-MSFT suggested changes May 21, 2019

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 21, 2019

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 21, 2019

Making a few changes on reviewer feedback for shortcut UI.

6f7b2b0

timheuer mentioned this pull request May 22, 2019

Adding UI for keyboard shortcuts on menu #805

Closed

DHowett-MSFT approved these changes May 22, 2019

View reviewed changes

src/cascadia/TerminalApp/App.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/AppKeyBindings.cpp Show resolved Hide resolved

Additional reviewer feedback on variable name change (not a member var)

50103e1

zadjii-msft approved these changes May 22, 2019

View reviewed changes

zadjii-msft merged commit e9a3d16 into microsoft:master May 22, 2019

zadjii-msft mentioned this pull request May 23, 2019

WriteConsoleOutputWOutsideBuffer test fails occasionally #959

Closed

Conversation

timheuer commented May 21, 2019

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Uh oh!

DHowett-MSFT commented May 21, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timheuer commented May 21, 2019

Uh oh!

DHowett-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

zadjii-msft commented May 22, 2019

Uh oh!

DHowett-MSFT commented May 22, 2019

Uh oh!

zadjii-msft commented May 22, 2019

Uh oh!

azure-pipelines bot commented May 22, 2019

Uh oh!

azure-pipelines bot commented May 22, 2019

Uh oh!

miniksa commented May 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants