Don't copy if there's no selection #2446
Merged
Conversation
This file contains 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
This works super well
The args don't seem to go through, this is insane
I'm going to clean this up in the next commit
The EventArgs need to not define their own Properties. If they do, the properties will shadow the ones from their base classes. However, the base class ones will still be used across the ABI or something. The args would revert to their default values across the ABI, which was bad. @carlos-zamora might be interested in this diff (and the parent commit)
This means we can get rid of all the EventArgs that don't actually have extra params. Do what @DHowett-MSFT suggested in https://github.com/microsoft/terminal/pull/1349/files#r313536975 There's a 50-100% chance that link will break when the PR is completed.
otherwise, let the key fallthrough Fixes #2285
zadjii-msft
commented
Aug 15, 2019
zadjii-msft
commented
Aug 15, 2019
zadjii-msft
commented
Aug 15, 2019
DHowett-MSFT
suggested changes
Aug 15, 2019
| @@ -1792,6 +1792,11 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation | |||
| return std::pow(cursorDistanceFromBorder, 2.0) / 25.0 + 2.0; | |||
| } | |||
|
|
|||
| bool TermControl::HasSelection() const | |||
| { | |||
| return _terminal != nullptr && _terminal->IsAreaSelected(); | |||
There was a problem hiding this comment.
there's also isSelectionActive. @carlos-zamora can you clarify which to use here?
There was a problem hiding this comment.
OMG. THEY'RE BOTH THE SAME. Adding to #1327.
| // signatures and define them both for you, because they don't really vary from | ||
| // event to event. | ||
| // Use this in a classes header if you have a Windows.Foundation.TypedEventHandler | ||
| #define TYPED_EVENT(name, sender, args) \ |
There was a problem hiding this comment.
is this just a combined version of DEFINE_..WITH_TYPED and DECLARE_..WITH_TYPED?
There was a problem hiding this comment.
if so please consider using the decltype magic in #1104 to remove the need for sender!
There was a problem hiding this comment.
You better believe it is. However, I'm a little cautious on doing that as much as I want to. There's some stuff in #2208 that explicitly has a Object sender, where the Object is not the current type. So for those situations we explicitly need to say IInspectable - would the magic still work for that case?
ghost
added
the
Needs-Author-Feedback
|
The PR mentions closing 2258 but it should be closing 2285 instead right? |
|
@dsafa good catch. thanks. |
|
I'd prefer the name be a little more.. specific on what it does. Fixing copy eating |
ghost
removed
the
Needs-Author-Feedback
carlos-zamora
requested changes
Aug 16, 2019
| @@ -1792,6 +1792,11 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation | |||
| return std::pow(cursorDistanceFromBorder, 2.0) / 25.0 + 2.0; | |||
| } | |||
|
|
|||
| bool TermControl::HasSelection() const | |||
| { | |||
| return _terminal != nullptr && _terminal->IsAreaSelected(); | |||
There was a problem hiding this comment.
OMG. THEY'RE BOTH THE SAME. Adding to #1327.
ghost
added
Needs-Author-Feedback
miniksa
reviewed
Aug 16, 2019
now TermControl.CopySelectionToClipboard returns a bool if successful
DHowett-MSFT
approved these changes
Aug 16, 2019
DHowett-MSFT
approved these changes
Aug 16, 2019
…ed-kb-events # Conflicts: # src/cascadia/TerminalApp/App.cpp # src/cascadia/TerminalApp/AppKeyBindings.cpp # src/cascadia/TerminalApp/AppKeyBindings.h # src/cascadia/TerminalApp/AppKeyBindings.idl
carlos-zamora
approved these changes
Aug 16, 2019
| { | ||
| // extract text from buffer | ||
| const auto copiedData = _terminal->RetrieveSelectedTextFromBuffer(trimTrailingWhitespace); | ||
| if (_terminal != nullptr && _terminal->IsAreaSelected()) |
There was a problem hiding this comment.
Suggested change
| if (_terminal != nullptr && _terminal->IsAreaSelected()) | |
| if (_terminal->IsAreaSelected()) |
Is this even possible? Can we have a TermControl with no Terminal?
This was referenced Aug 17, 2019
|
🎉 Handy links: |
ghost
mentioned this pull request
4 tasks
fredimachado
added a commit
to fredimachado/terminal
that referenced
this pull request
Sep 9, 2019
The ctrl+c issue was fixed in [microsoft#2446](microsoft#2446)
miniksa
pushed a commit
that referenced
this pull request
Sep 10, 2019
4 tasks
zadjii-msft
added a commit
that referenced
this pull request
Nov 14, 2019
## Summary of the Pull Request
Enables the user to provide arbitrary argument values to shortcut actions through a new `args` member of keybindings. For some keybindings, like `NewTabWithProfile<N>`, we previously needed 9 different `ShortcutAction`s, one for each value of `Index`. If a user wanted to have a `NewTabWithProfile11` keybinding, that was simply impossible. Now that the args are in their own separate json object, each binding can accept any number of arbitrary argument values.
So instead of:
```json
{ "command": "newTab", "keys": ["ctrl+shift+t"] },
{ "command": "newTabProfile0", "keys": ["ctrl+shift+1"] },
{ "command": "newTabProfile1", "keys": ["ctrl+shift+2"] },
{ "command": "newTabProfile2", "keys": ["ctrl+shift+3"] },
{ "command": "newTabProfile3", "keys": ["ctrl+shift+4"] },
```
We can now use:
```json
{ "command": "newTab", "keys": ["ctrl+shift+t"] },
{ "command": { "action": "newTab", "index": 0 }, "keys": ["ctrl+shift+1"] },
{ "command": { "action": "newTab", "index": 1 }, "keys": ["ctrl+shift+2"] },
{ "command": { "action": "newTab", "index": 2 }, "keys": ["ctrl+shift+3"] },
```
Initially, this does seem more verbose. However, for cases where there are multiple args, or there's a large range of values for the args, this will quickly become a more powerful system of expressing keybindings.
The "legacy" keybindings are _left in_ in this PR. They have helper methods to generate appropriate `IActionArgs` values. Prior to releasing 1.0, I think we should remove them, if only to remove some code bloat.
## References
See [the spec](https://github.com/microsoft/terminal/blob/master/doc/specs/%231142%20-%20Keybinding%20Arguments.md) for more details.
This is part two of the implementation, part one was #2446
## PR Checklist
* [x] Closes #1142
* [x] I work here
* [x] Tests added/passed
* [x] Schema updated
## Validation Steps Performed
* Ran Tests
* Removed the legacy keybindings from the `defaults.json`, everything still works
* Tried leaving the legacy keybingings in my `profiles.json`, everything still works.
-------------------------------------------------
* this is a start, but there's a weird linker bug if I take the SetKeybinding(ShortcutAction, KeyChord) implementation out, which I don't totally understand
* a good old-fashioned clean will fix that right up
* all these things work
* hey this actually _functionally_ works
* Mostly cleanup and completion of implementation
* Hey I bet we could just make NewTab the handler for NewTabWithProfile
* Start writing tests for Keybinding args
* Add tests
* Revert a bad sln change, and clean out dead code
* Change to include "command" as a single object
This is a change to make @DHowett-MSFT happy. Changes the args to be a part
of the "command" object, as opposed to an object on their own.
EX:
```jsonc
// Old style
{ "command": "switchToTab0", "keys": ["ctrl+1"] },
{ "command": { "action": "switchToTab", "index": 0 }, "keys": ["ctrl+alt+1"] },
// new style
{ "command": "switchToTab0", "keys": ["ctrl+1"] },
{ "command": "switchToTab", "args": { "index": 0 } "keys": ["ctrl+alt+1"] },
```
* schemas are hard yo
* Fix the build?
* wonder why my -Wall settings are different than CI...
* this makes me hate things
* Comments from PR
* Add a `Direction::None`
* LOAD BEARING
* add some GH ids to TODOs
* add a comment
* PR nits from carlos
donno2048
added a commit
to donno2048/terminal
that referenced
this pull request
Sep 28, 2020
* Fix json settings documentation The ctrl+c issue was fixed in [#2446](microsoft/terminal#2446) * Update UsingJsonSettings.md
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.
Summary of the Pull Request
Converts every action we have to a TypedEventHandler. These are in accordance with #1349. This allows us to not handle some events, like the Copy event.
References
Really heavily inspired by #1349. @DHowett-MSFT thinks we should ship this fix this month an I agree. Hence the PR before the spec is finished.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed