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

Dialogue to show all keyboard shortcuts #5849

Closed
wants to merge 18 commits into from
Closed

Dialogue to show all keyboard shortcuts #5849

wants to merge 18 commits into from

Conversation

Chips1234
Copy link
Contributor

Summary of the Pull Request

Adds a dialogue that shows all keyboard shortcuts. If I'm missing anything, feel free to share!

References

Connected to #4695.

PR Checklist

Detailed Description of the Pull Request / Additional comments

A new dialogue launched by clicking on a new tab menu item.

@ghost ghost added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels May 11, 2020
@Chips1234
Copy link
Contributor Author

Wait, why are there commits unrelated to this?

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.

Overall, looks like a great start.

I'd say the big thing that's missing here is that the dialog only displays a few of the default keybindings. Users could have set any key combination to any number of actions, including disabling these default keybindings. I'd think that the next step in this functionality would be enumerating all of those keybindings that are bound at runtime, and actually filling ShortcutsDialog with those bindings at runtime

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 11, 2020
@github-actions
Copy link

New misspellings found, please review:

  • defult
To accept these changes, run the following commands
remove_obsolete_words=$(mktemp)
echo '#!/usr/bin/perl -ni
my $re=join "|", qw('"
bitfield
Emojis
HREF
memcpying
OUTPATHROOT
textblock
usr
vpack
"');
next if /^($re)(?:$| .*)/;
print;' > $remove_obsolete_words
chmod +x $remove_obsolete_words
for file in .github/actions/spell-check/whitelist/alphabet.txt .github/actions/spell-check/whitelist/web.txt .github/actions/spell-check/whitelist/whitelist.txt; do $remove_obsolete_words $file; done
rm $remove_obsolete_words
(
echo "
defult
"
) | sort -u -f | perl -ne 'next unless /./; print' > new_whitelist.txt && mv new_whitelist.txt '.github/actions/spell-check/whitelist/ce61d8f6be1e1a59ef1ed4ad736cc7ea1cb0b00e.txt'
✏️ Contributor please read this
  • If the items listed above are names, please add them to .github/actions/spell-check/dictionary/names.txt.
  • If they're APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
  • If they're just things you're using, please add them to an appropriate file in .github/actions/spell-check/whitelist/.
  • If you need to use a specific token in one place and it shouldn't generally be used, you can
    add an item in an appropriate file in .github/actions/spell-check/patterns/.

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

⚠️ Reviewers

At present, the action that triggered this message will not show its ❌ in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

@thernstig
Copy link

@zadjii-msft Might I ask why you believe user defined keybindings? I'd look at it like a quick way to show default keybindings, to "learn and discover" what one can do. If one wants to see all keybindings it is always possible to open the settings.json file.

@Chips1234
Copy link
Contributor Author

@zadjii-msft, thus I was asking how to get data from json file to xaml.

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 11, 2020
@github-actions
Copy link

New misspellings found, please review:

  • defult
To accept these changes, run the following commands
remove_obsolete_words=$(mktemp)
echo '#!/usr/bin/perl -ni
my $re=join "|", qw('"
bitfield
Emojis
HREF
memcpying
OUTPATHROOT
textblock
usr
vpack
"');
next if /^($re)(?:$| .*)/;
print;' > $remove_obsolete_words
chmod +x $remove_obsolete_words
for file in .github/actions/spell-check/whitelist/alphabet.txt .github/actions/spell-check/whitelist/web.txt .github/actions/spell-check/whitelist/whitelist.txt; do $remove_obsolete_words $file; done
rm $remove_obsolete_words
(
echo "
defult
"
) | sort -u -f | perl -ne 'next unless /./; print' > new_whitelist.txt && mv new_whitelist.txt '.github/actions/spell-check/whitelist/93cb22dafb49404847ba24b117a3bf2cd2912b5d.txt'
✏️ Contributor please read this
  • If the items listed above are names, please add them to .github/actions/spell-check/dictionary/names.txt.
  • If they're APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
  • If they're just things you're using, please add them to an appropriate file in .github/actions/spell-check/whitelist/.
  • If you need to use a specific token in one place and it shouldn't generally be used, you can
    add an item in an appropriate file in .github/actions/spell-check/patterns/.

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

⚠️ Reviewers

At present, the action that triggered this message will not show its ❌ in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

@Chips1234
Copy link
Contributor Author

Chips1234 commented May 16, 2020

So, I have here a binding that can possibly work. Problem is, I don't know what to put next in the if statement, as I need it to display text. Any help will be appreciated.

winrt::hstring TerminalPage::CopyKeybinding()
    {
       auto keyBindings = _settings->GetKeybindings();
        auto copyKeyCord = keyBindings.GetKeyBindingForAction(ShortcutAction::CopyText);
        if (copyKeyCord)
        {
       }
        return RS_(L"NoCopyData");
    }

@Chips1234
Copy link
Contributor Author

Another thing, how do I get it so that it can be displayed in const auto form?

@Chips1234
Copy link
Contributor Author

Chips1234 commented May 20, 2020

Well, right now, I just can't get it to display based on settings, but what do you say we implement the default keybindings for now and we can work on getting it to change based on the settings.json file later on?

@vadimkantorov
Copy link

If there're profile-specific keybindings supported in future, this dialog should also take some decisions about how to react/display to this specificity

@Chips1234
Copy link
Contributor Author

So is there a way to get the keybinding/profile into string then add it to XAML?

@zadjii-msft
Copy link
Member

@Chips1234 sorry for the delay - I'm assuming #6487 answered your question?

@Chips1234
Copy link
Contributor Author

Yes, but how do you get it inside the dialog? As in, how do you get the code into XAML?

@zadjii-msft
Copy link
Member

Oh so you can just construct the UI element at runtime. Take for example, the context menu on a tab - that's built at runtime in Tab::_CreateContextMenu. Or how we build the errors dialog over in applogic:

void AppLogic::_ShowLoadErrorsDialog(const winrt::hstring& titleKey,
const winrt::hstring& contentKey,
HRESULT settingsLoadedResult)
{
auto title = GetLibraryResourceString(titleKey);
auto buttonText = RS_(L"Ok");
Controls::TextBlock warningsTextBlock;
// Make sure you can copy-paste
warningsTextBlock.IsTextSelectionEnabled(true);
// Make sure the lines of text wrap
warningsTextBlock.TextWrapping(TextWrapping::Wrap);
winrt::Windows::UI::Xaml::Documents::Run errorRun;
const auto errorLabel = GetLibraryResourceString(contentKey);
errorRun.Text(errorLabel);
warningsTextBlock.Inlines().Append(errorRun);
warningsTextBlock.Inlines().Append(Documents::LineBreak{});
if (FAILED(settingsLoadedResult))
{
if (!_settingsLoadExceptionText.empty())
{
warningsTextBlock.Inlines().Append(_BuildErrorRun(_settingsLoadExceptionText, ::winrt::Windows::UI::Xaml::Application::Current().as<::winrt::TerminalApp::App>().Resources()));
warningsTextBlock.Inlines().Append(Documents::LineBreak{});
}
}
// Add a note that we're using the default settings in this case.
winrt::Windows::UI::Xaml::Documents::Run usingDefaultsRun;
const auto usingDefaultsText = RS_(L"UsingDefaultSettingsText");
usingDefaultsRun.Text(usingDefaultsText);
warningsTextBlock.Inlines().Append(Documents::LineBreak{});
warningsTextBlock.Inlines().Append(usingDefaultsRun);
Controls::ContentDialog dialog;
dialog.Title(winrt::box_value(title));
dialog.Content(winrt::box_value(warningsTextBlock));
dialog.CloseButtonText(buttonText);
dialog.DefaultButton(Controls::ContentDialogButton::Close);
_ShowDialog(nullptr, dialog);
}

@jsoref
Copy link
Contributor

jsoref commented Aug 13, 2020

Right now this PR displays to me as:

@Chips1234 wants to merge 18 commits into microsoft:master from unknown repository

that seems like isaacs/github#168 -- but I'm confused because afaict this PR still exists. Does someone happen to know why it says unknown repository? (Is this related to the PR being a draft?)

@zadjii-msft
Copy link
Member

@jsoref That's bizarre, I've never seen anything like that before.

@jsoref
Copy link
Contributor

jsoref commented Aug 13, 2020

@zadjii-msft: I don't suppose you can find someone via the internal directory who could shine some light on this?

@zadjii-msft
Copy link
Member

Huh, I don't think I've ever actually mailed anyone from Github yet. Kinda forgot that they're under the MSFT umbrella 😅

Cold-emailing random @github.com emails doesn't really seem like it would work to find an appropriate engineer. Maybe @DHowett knows someone?

@Chips1234
Copy link
Contributor Author

Yes, unfortunately my fork is all messed up and U needed to delete my old one. Sorry, but I have to close this now.

@Chips1234 Chips1234 closed this Aug 13, 2020
@DHowett
Copy link
Member

DHowett commented Aug 13, 2020

This is very much because the originating fork was deleted.

@jsoref
Copy link
Contributor

jsoref commented Aug 13, 2020

Interesting. I'm surprised that the link presumes that the repository is this one even though it more or less knows it isn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Keyboard shortcut to show all keyboard shortcuts
6 participants