Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Actions page design spec #9427

Merged
merged 2 commits into from
Aug 24, 2021
Merged

Actions page design spec #9427

merged 2 commits into from
Aug 24, 2021

Conversation

cinnamon-msft
Copy link
Contributor

@cinnamon-msft cinnamon-msft commented Mar 9, 2021

Summary of the Pull Request

This is the design spec for the Actions page, which is proposed as a Keyboard page instead.

Here is the current design that's in production that this spec improves upon:
image

References

#6900

Markdown view link

@github-actions
Copy link

github-actions bot commented Mar 9, 2021

Misspellings found, please review:

  • inteded
  • throught
To accept these changes, run the following commands from this repository on this branch
pushd $(git rev-parse --show-toplevel)
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"aspnet boostorg COINIT dahall fde fea fmtlib isocpp mintty NVDA pinam QOL remoting unte vcrt what3words xamarin "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/8358f8d93f714716dc4ac72bdb107f0469a388b4.txt";
use File::Path qw(make_path);
make_path ".github/actions/spelling/expect";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"coinit inteded Remoting throught "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
popd
✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spelling/dictionary/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spelling/dictionary/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
  • ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

🗜️ If you see a bunch of garbage and it relates to a binary-ish string, please add a file path to the .github/actions/spelling/excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

Comment on lines +33 to +46
Commands with properties:
- sendInput has "input"
- closeOtherTabs has "index"
- closeTabsAfter has "index"
- renameTab has "title"*
- setTabColor has "color"*
- newWindow has "commandline", "startingDirectory", "tabTitle", "index", "profile"
- splitPane has "split", "commandline", "startingDirectory", "tabTitle", "index", "profile", "splitMode", "size"
- copy has "singleLine", "copyFormatting"
- scrollUp has "rowsToScroll"
- scrollDown has "rowsToScroll"
- setColorScheme has "colorScheme"

Majority of these commands listed above are intended for the command palette, so they wouldn't make much sense with keys assigned to them anyway.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole section feels a little weird here tbh. It's not reall a user story - more of a "potential issue"? Like, it's an issue that these actions won't show up in the dropdown by default, because they aren't bound in the default settings. But it's not something to worry about because

  • These aren't actions that really make sense in a default setting, they require the user to actually provide values for their args manually
  • The user can always go in and add these actions to their keybindings manually. Once they do, then these actions will appear in the combobox in the UI


### Future Considerations

One day we'll have actions that can be invoked by items in the dropdown menu. This setting will have to live somewhere. Also, once we get a status bar, people may want to invoke actions from there.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to add here that we're specifically not addressing customizing args as a part of this design. Action args is a big part of the actions, so I think it's good to come out strong and say "we have a plan, but we can't do it quite yet, and here's why".

Throw in that mockup with the content dialog. Then link to the upstream MUX bug tracking content dialogs not working in xaml islands and say something like "once the upstream bug is resolved, this is the UI we plan on".

Comment on lines +127 to +130

## Resources

### Footnotes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can take these out since you didn't really use them ¯\_(ツ)_/¯

@zadjii-msft zadjii-msft added Area-SettingsUI Anything specific to the SUI Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Product-Terminal The new Windows Terminal. labels Mar 10, 2021
@mdtauk
Copy link

mdtauk commented Mar 10, 2021

Summary of the Pull Request

This is the design spec for the Actions page, which is proposed as a Keyboard page instead.

Here is the current design that's in production that this spec improves upon:
image

References

#6900

Markdown view link

The keyboard shortcuts look pretty cramped in those outlined boxes, so perhaps some more padding could be used.

It may also be worth syncing with what PowerToys are doing for displaying keyboard shortcuts so a consistent approach can be found.

image

ghost pushed a commit that referenced this pull request May 5, 2021
This spec covers the settings model work required to create the Actions page in the settings UI (designed in #9427). 

Overall, the idea is to promote `Command` to include the actual `KeyChord`, then introduce an `ActionMap` that handles all of the responsibilities of `KeyMapping` and more (as well as general action management).

[Markdown view](https://github.com/microsoft/terminal/blob/dev/cazamor/spec/tsm-actions/doc/specs/%23885%20-%20Terminal%20Settings%20Model/Actions%20Addendum.md)
@zadjii-msft
Copy link
Member

hey uh, this looks like we're about to just merge the implementation in #9949. Should we just merge this as-is, as a "this is what we planned on"? I think we're all pretty on the same page with this design

@zadjii-msft zadjii-msft added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 12, 2021
ghost pushed a commit that referenced this pull request May 18, 2021
## Summary of the Pull Request

This PR lays the foundation for a new Actions page in the Settings UI as designed in #6900. The Actions page now leverages the `ActionMap` to display all of the key bindings and allow the user to modify the associated key chord or delete the key binding entirely.

## References

#9621 - ActionMap
#9926 - ActionMap serialization
#9428 - ActionMap Spec
#6900 - Actions page
#9427 - Actions page design doc

## Detailed Description of the Pull Request / Additional comments

### Settings Model Changes

- `Command::Copy()` now copies the `ActionAndArgs`
- `ActionMap::RebindKeys()` handles changing the key chord of a key binding. If a conflict occurs, the conflicting key chord is overwritten.
- `ActionMap::DeleteKeyBinding()` "deletes" a key binding by binding "unbound" to the given key chord.
- `ActionMap::KeyBindings()` presents another view (similar to `NameMap`) of the `ActionMap`. It specifically presents a map of key chords to commands. It is generated similar to how `NameMap` is generated.

### Editor Changes

- `Actions.xaml` is mainly split into two parts:
   - `ListView` (as before) holds the list of key bindings. We _could_ explore the idea of an items repeater, but the `ListView` seems to provide some niceties with regards to navigating the list via the key board (though none are selectable).
   - `DataTemplate` is used to represent each key binding inside the `ListView`. This is tricky because it is bound to a `KeyBindingViewModel` which must provide _all_ context necessary to modify the UI and the settings model. We cannot use names to target UI elements inside this template, so we must make the view model smart and force updates to the UI via changes in the view model.
- `KeyBindingViewModel` is a view model object that controls the UI and the settings model. 

There are a number of TODOs in Actions.cpp will be long-term follow-ups and would be nice to have. This includes...
- a binary search by name on `Actions::KeyBindingList`
- presenting an error when the provided key chord is invalid.

## Demo
![Actions Page Demo](https://user-images.githubusercontent.com/11050425/116034988-131d1b80-a619-11eb-8df2-c7e57c6fad86.gif)
@ghost ghost added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label May 23, 2021
@ghost
Copy link

ghost commented May 23, 2021

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

@ghost ghost closed this May 30, 2021
carlos-zamora added a commit that referenced this pull request Jul 2, 2021
This introduces the ability to set the action for a key binding. A combo box is used to let the user select a new action.

## References
#6900 - Actions page Epic
#9427 - Actions page design doc
#9949 - Actions page PR

## Detailed Description of the Pull Request / Additional comments
### Settings Model Changes
- `ActionAndArgs`
   - new ctor that just takes a `ShortcutAction`
- `ActionMap`
   - `AvailableActions` provides a map of all the "acceptable" actions to choose from. This is a merged list of (1) all `{ "command": X }` style actions and (2) any actions with args that are already defined in the ActionMap (or any parents).
   - `RegisterKeyBinding` introduces a new unnamed key binding to the action map.

### Editor Changes
- XAML
   - Pretty straightforward, when in edit mode, we replace the text block with a combo box. This combo box just presents the actions you can choose from.
- `RebindKeysEventArgs` --> `ModifyKeyBindingEventArgs`
- `AvailableActionAndArgs`
   - stores the list of actions to choose from in the combo box
   - _Unfortunately_, `KeyBindingViewModel` needs this so that we can populate the combo box
   - `Actions` stores and maintains this though. We populate this from the settings model on navigation.
- `ProposedAction` vs `CurrentAction`
   - similar to `ProposedKeys` and `Keys`, we need a way to distinguish the value from the settings model and the value of the control (i.e. combo box).
   - `CurrentAction` --> settings model
   - `ProposedAction` --> combo box selected item

## Validation Steps Performed
- Cancel:
   - ✔️ change action --> cancel button --> begin editing action again --> original action is selected
- Accept:
   - ✔️ don't change anything
   - ✔️ change action --> OK! --> Save!
      - NOTE: The original action is still left as a stub `{ "command": "closePane" }`. This is intentional because we want to prevent all modifications to the command palette.
   - ✔️ change action & change key chord --> OK! --> Save!
   - ✔️ change action & change key chord (conflicting key chord) --> OK! --> click ok on flyout --> Save!
      - NOTE: original action is left as a stub; original key chord explicitly unbound; new command/keys combo added.
ghost pushed a commit that referenced this pull request Jul 7, 2021
## Summary of the Pull Request
This adds the "add new" button to the actions page. It build on the work of #10220 by basically just adding a new list item to the top of the key binding list.

This also makes it so that if you click the "accept changes" button when you have an invalid key chord, we don't do anything.

## References
#6900 - Actions page Epic
#9427 - Actions page design doc
#10220 - Actions page PR - set action

## Detailed Description of the Pull Request / Additional comments
- `ModifyKeyBindingEventArgs` is used to introduce new key bindings. We just ignore `OldKeys` and `OldActionName` because both didn't exist before.
- `IsNewlyAdded` tracks if this is an action that was added, but has not been confirmed to add to the settings model.
- `CancelChanges()` is directly bound to the cancel button. This allows us to delete the key binding when it's clicked on a "newly added" action.

## Validation Steps Performed
- Cancel:
   - Deletes the action (because it doesn't truly exist until you confirm changes)
- Accept:
   - Adds the new action.
   - If you attempt to edit it, the delete button is back.
- Add Action:
   - Delete button should not be visible (redundant with 'Cancel')
   - Action should be initialized to a value
   - Key chord should be empty
   - Cannot add another action if a newly added action exists
- Keyboard interaction:
   - escape --> cancel
   - enter --> accept
- Accessibility:
   - "add new" button has a name
- Interaction with other key bindings:
   - editing another action --> delete the "newly added" action (it hasn't been added yet)
   - only one action can be edited at a time
@cinnamon-msft cinnamon-msft reopened this Jul 15, 2021
@ghost ghost removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. labels Jul 15, 2021
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey so I'm approving this, because we're kinda moving forward with this regardless. This can serve to be a "this is what we had in mind while designing the SUI for actions" doc. This PR has done it's job to drive the discussion.

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Aug 24, 2021
@ghost ghost requested review from miniksa and carlos-zamora August 24, 2021 19:55
@ghost ghost requested review from DHowett, leonMSFT, PankajBhojwani and lhecker August 24, 2021 19:55
@carlos-zamora
Copy link
Member

Merging as admin because doc PRs don't run in CI and we have 3 approvals

@carlos-zamora carlos-zamora merged commit f3a49fa into main Aug 24, 2021
@carlos-zamora carlos-zamora deleted the cinnamon/actions-page-spec branch August 24, 2021 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-SettingsUI Anything specific to the SUI Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants