Enable PgUp/PgDown and Home/End in the command palette #7835

ghost · 2020-10-06T03:59:03Z

DHowett · 2020-10-06T04:31:36Z

Hey there! You can keep updating the same pull request, you don’t have to make a new one every time! 😄 it might be a little easier for you to manage that way!

zadjii-msft · 2020-10-07T19:54:31Z

My only real qualm with this PR is hardcoding the height to 42. I'd really rather not do that, and use the ActualHeight of the individual ListViewItem at runtime. However, I'm not positive that there's actually a way to do that. I don't know if it's possible to get a singular ListViewItem from a list view, which seems bizarre to me

ghost · 2020-10-08T01:26:43Z

Yes I don't like it is being hardcoded either. I will try a little bit more.

Don-Vito · 2020-10-09T06:38:13Z

src/cascadia/TerminalApp/CommandPalette.cpp

        const auto selected = _filteredActionsView().SelectedIndex();
        const int numItems = ::base::saturated_cast<int>(_filteredActionsView().Items().Size());
        // Wraparound math. By adding numItems and then calculating modulo numItems,
        // we clamp the values to the range [0, numItems) while still supporting moving
        // upward from 0 to numItems - 1.
-        const auto newIndex = ((numItems + selected + (moveDown ? 1 : -1)) % numItems);
+        const auto newIndex = ((numItems + selected + (moveDown ? (pageButtonPressed ? numVisibleItems : 1) : (pageButtonPressed ? -numVisibleItems : -1))) % numItems);


The logic duplication can be avoided by extracting a "delta" variable: moveDown? delta : -delta

Don-Vito · 2020-10-09T06:40:31Z

src/cascadia/TerminalApp/CommandPalette.cpp

    {
+        const auto listHeight = ::base::saturated_cast<int>(_filteredActionsView().ActualHeight());
+        const int numVisibleItems = listHeight / 42;


If 42 cannot be computed, consider extracting it to a const and documenting why this value was chosen

Don-Vito · 2020-10-09T06:46:58Z

src/cascadia/TerminalApp/CommandPalette.cpp

-    // - moveDown: if true, we're attempting to move to the next item in the
-    //   list. Otherwise, we're attempting to move to the previous.
+    // - moveDown: if true, we're attempting to move to the next or a few next item in the
+    //   list. Otherwise, we're attempting to move to the previous or not very many previous.


Document the new parameter

src/cascadia/TerminalApp/CommandPalette.cpp

ghost · 2020-10-10T10:06:03Z

I've been trying to generate ListViewItem in CommandPalette.xaml.g.h like _filteredActionView() is so that I can access a height property of ListViewItem, but I keep failing it. Though it's possible right ?

DHowett · 2020-10-13T01:27:43Z

I wonder if we can get rid of the 42 by using... ListView::ContainerFromIndex() and measuring the object it gives us to get its ActualHeight()?

ghost · 2020-10-14T01:50:10Z

I have no idea how to measure the object that I got from ListView::ContainerFromIndex(). I have been trying to get top y coordinate and bottom y coordinate of the object and subtract one from another, but I just can not do it. Or is there specific API to measure the height of it ?

ghost · 2020-10-14T02:18:26Z

Is this commit better than before? At least hardcoded 42 is removed.

previous commit was draft

zadjii-msft

What happened to trying ContainerFromIndex? Does the following work to get the item's height?

        auto f = _filteredActionsView().ContainerFromIndex(0);
        auto g = f.try_as<winrt::Windows::UI::Xaml::Controls::ListViewItem>();
        auto h = g.ActualHeight();

I think that should work, but g might end up being null. If it is, I'd maybe try casting it to a FrameworkElement instead

zadjii-msft · 2020-10-15T07:49:11Z

src/cascadia/TerminalApp/CommandPalette.cpp

    {
+        const auto itemHeight = ::base::saturated_cast<int>(_parentCommandText().Height());


I wouldn't take a dependency on the height of the parentCommandText element, there's not necessarily a guarantee that element is the same height as the items of the list view

src/cascadia/TerminalApp/CommandPalette.xaml

Using ListView::ContainerFromIndex() and casted into ListViewItem to get its ActualHeight

zadjii-msft

As much as I hate this ternary, I'd rather get this in for now and file a follow-up PR to clean it up. Someone else on the team can override that opinion though. Thanks for your patience!

src/cascadia/TerminalApp/CommandPalette.cpp

DHowett

Thanks for the contribution! This is great. I'm going to address my own comment about the App.xaml file and merge it.

DHowett · 2020-10-22T00:08:18Z

src/cascadia/TerminalApp/App.xaml

+                        We need to manually create the error text brush as a
+                        theme-dependent brush. SystemControlErrorTextForegroundBrush
+                        is unfortunately static.
+                    -->


I don't totally understand why this file changed

Yes. I don't either. Sorry for the mess.

DHowett · 2020-10-22T00:11:50Z

This is excellent work. 😄

ghost · 2020-10-22T00:20:01Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

DHowett · 2020-10-22T04:05:55Z

/azp run

azure-pipelines · 2020-10-22T04:06:11Z

Azure Pipelines successfully started running 1 pipeline(s).

ghost · 2020-10-22T04:37:04Z

Apologies, while this PR appears ready to be merged, it looks like main is a protected branch and I have not been granted permission to perform the merge.

Closes #7729 (cherry picked from commit 293ad27)

ghost · 2020-11-11T18:06:53Z

🎉Windows Terminal v1.4.3141.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2020-11-11T18:11:59Z

🎉Windows Terminal Preview v1.5.3142.0 has been released which incorporates this pull request.:tada:

Handy links:

enabling page up/down button

93979d7

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 Oct 6, 2020

changed variable name

bfb3eea

Don-Vito reviewed Oct 9, 2020

View reviewed changes

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

kaihugo added 4 commits October 10, 2020 09:38

Added MinHeight

bdbeb88

Update CommandPalette.cpp

d1a8f14

fixed careless mistakes

357281b

Deleted an unnecessary line

7f92f5f

KiminoriKaburagi added 2 commits October 12, 2020 09:06

extracted 42 to const

93945f6

Enabled Home and End buttons

dbb4e23

extract parentCommandText's height

6fcc5c3

true commit

4382302

previous commit was draft

zadjii-msft requested changes Oct 15, 2020

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 Oct 15, 2020

Removing hardcoded 42

6590132

Using ListView::ContainerFromIndex() and casted into ListViewItem to get its ActualHeight

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 16, 2020

Documented new parameter for SelectNextItem()

d142e5e

DHowett added the hacktoberfest-accepted label Oct 16, 2020

zadjii-msft approved these changes Oct 19, 2020

View reviewed changes

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

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

KiminoriKaburagi and others added 4 commits October 20, 2020 10:40

Split SelectNextItem() into 3 functions

b085799

Delete ternary in GoEnd()

b7687b5

fix trivial mistake

a30b997

Merge branch 'master' into fet/7729-pageUpDown

23ae653

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Oct 21, 2020

ghost requested review from miniksa, carlos-zamora, DHowett, leonMSFT and PankajBhojwani October 21, 2020 07:46

DHowett approved these changes Oct 22, 2020

View reviewed changes

Revert app.xaml

2a82844

code format

de2d629

DHowett changed the title ~~enabling page up/down button~~ Enable PgUp/PgDown and Home/End in the command palette Oct 22, 2020

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 22, 2020

microsoft-github-updates bot changed the base branch from master to main October 22, 2020 00:27

DHowett added AutoMerge Marked for automatic merge by the bot when requirements are met and removed AutoMerge Marked for automatic merge by the bot when requirements are met labels Oct 22, 2020

ghost merged commit 293ad27 into microsoft:main Oct 22, 2020

ghost deleted the fet/7729-pageUpDown branch October 22, 2020 09:33

DHowett pushed a commit that referenced this pull request Oct 27, 2020

Enable PgUp/PgDown and Home/End in the command palette (#7835)

bb5549a

Closes #7729 (cherry picked from commit 293ad27)

ghost mentioned this pull request Nov 11, 2020

Command Palette should support page up/down to move faster thru the options #7729

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable PgUp/PgDown and Home/End in the command palette #7835

Enable PgUp/PgDown and Home/End in the command palette #7835

ghost commented Oct 6, 2020 •

edited by DHowett

Loading

DHowett commented Oct 6, 2020

zadjii-msft commented Oct 7, 2020

ghost commented Oct 8, 2020

Don-Vito Oct 9, 2020

Don-Vito Oct 9, 2020

Don-Vito Oct 9, 2020

ghost commented Oct 10, 2020 •

edited by ghost

Loading

DHowett commented Oct 13, 2020

ghost commented Oct 14, 2020 •

edited by ghost

Loading

ghost commented Oct 14, 2020 •

edited by ghost

Loading

zadjii-msft left a comment

zadjii-msft Oct 15, 2020

zadjii-msft left a comment

DHowett left a comment

DHowett Oct 22, 2020

ghost Oct 22, 2020 •

edited by ghost

Loading

DHowett commented Oct 22, 2020

ghost commented Oct 22, 2020

DHowett commented Oct 22, 2020

azure-pipelines bot commented Oct 22, 2020

ghost commented Oct 22, 2020

ghost commented Nov 11, 2020

ghost commented Nov 11, 2020

		{
		const auto itemHeight = ::base::saturated_cast<int>(_parentCommandText().Height());

Enable PgUp/PgDown and Home/End in the command palette #7835

Enable PgUp/PgDown and Home/End in the command palette #7835

Conversation

ghost commented Oct 6, 2020 • edited by DHowett Loading

DHowett commented Oct 6, 2020

zadjii-msft commented Oct 7, 2020

ghost commented Oct 8, 2020

Don-Vito Oct 9, 2020

Choose a reason for hiding this comment

Don-Vito Oct 9, 2020

Choose a reason for hiding this comment

Don-Vito Oct 9, 2020

Choose a reason for hiding this comment

ghost commented Oct 10, 2020 • edited by ghost Loading

DHowett commented Oct 13, 2020

ghost commented Oct 14, 2020 • edited by ghost Loading

ghost commented Oct 14, 2020 • edited by ghost Loading

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft Oct 15, 2020

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

DHowett Oct 22, 2020

Choose a reason for hiding this comment

ghost Oct 22, 2020 • edited by ghost Loading

Choose a reason for hiding this comment

DHowett commented Oct 22, 2020

ghost commented Oct 22, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

DHowett commented Oct 22, 2020

azure-pipelines bot commented Oct 22, 2020

ghost commented Oct 22, 2020

ghost commented Nov 11, 2020

ghost commented Nov 11, 2020

ghost commented Oct 6, 2020 •

edited by DHowett

Loading

ghost commented Oct 10, 2020 •

edited by ghost

Loading

ghost commented Oct 14, 2020 •

edited by ghost

Loading

ghost commented Oct 14, 2020 •

edited by ghost

Loading

ghost Oct 22, 2020 •

edited by ghost

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.