First three interactivity fixes #9980

zadjii-msft · 2021-04-28T20:14:35Z

Summary of the Pull Request

This PR encompasses the first three bugs we found post-#9820.

A: Mousedown, select, SCROLL does a weird thing with endpoints that doesn't happen in stable

We were using the terminal position to set the selection anchor, when we should have used the pixel position.

This is fixed in 4f4df01.

B: Trackpad scrolling down with small increments seems buggy

This one's the most complicated. The touchpad sends very many small scroll deltas, less than one row at a time. The control scrollbar can store a double, so small deltas can accumulate. Originally, these would accumulate in the scrollbar, and we'd only read that out as an int in the scrollbar updater, which is throttled.

In the interactivity split, there's no place for us to store that double. We immediately narrow to an int for ControlInteractivity::_updateScrollbar.

So this introduces a double inside ControlInteractivity as a fake scrollbar, with which to accumulate to.

This is fixed in 33d29fa...0fefc5b

C: Looks like there's a selection issue when you click and drag too quickly.

The diff for this one is:

1.8

main

if (_singleClickTouchdownPos)
{
    // Figure out if the user's moved a quarter of a cell's smaller axis away from the clickdown point
    auto& touchdownPoint{ *_singleClickTouchdownPos };
    auto distance{ std::sqrtf(std::powf(cursorPosition.X - touchdownPoint.X, 2) + std::powf(cursorPosition.Y - touchdownPoint.Y, 2)) };
    const til::size fontSize{ _actualFont.GetSize() };

    const auto fontSizeInDips = fontSize.scale(til::math::rounding, 1.0f / _renderEngine->GetScaling());
    if (distance >= (std::min(fontSizeInDips.width(), fontSizeInDips.height()) / 4.f))
    {
        _terminal->SetSelectionAnchor(_GetTerminalPosition(touchdownPoint));
        // stop tracking the touchdown point
        _singleClickTouchdownPos = std::nullopt;
    }
}

if (_singleClickTouchdownPos)
{
    // Figure out if the user's moved a quarter of a cell's smaller axis away from the clickdown point
    auto& touchdownPoint{ *_singleClickTouchdownPos };
    float dx = ::base::saturated_cast<float>(pixelPosition.x() - touchdownPoint.x());
    float dy = ::base::saturated_cast<float>(pixelPosition.y() - touchdownPoint.y());
    auto distance{ std::sqrtf(std::powf(dx, 2) +
                              std::powf(dy, 2)) };

    const auto fontSizeInDips{ _core->FontSizeInDips() };
    if (distance >= (std::min(fontSizeInDips.width(), fontSizeInDips.height()) / 4.f))
    {
        _core->SetSelectionAnchor(terminalPosition);
        // stop tracking the touchdown point
        _singleClickTouchdownPos = std::nullopt;
    }
}

        _terminal->SetSelectionAnchor(_GetTerminalPosition(touchdownPoint));

vs

        _core->SetSelectionAnchor(terminalPosition);

We're now using the location of the drag event as the selection anchor, instead of the location that the user initially clicked. Oops.

PR Checklist

Checks three boxes, though I'll be shocked if they're the last.
I work here
Tests added/passed 🎉🎉🎉
[n/a] Requires documentation to be updated

Detailed Description of the Pull Request / Additional comments

All three have tests, 🙌🙌🙌🙌

Validation Steps Performed

Manual, and automated via tests

ironic that all these bugs are all in the pixelPosition code that I yeeted in late on the last Wednesday I was working on the original PR

github-actions · 2021-04-28T20:18:36Z

Misspellings found, please review:

draging
infor
temrinal

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('"aef aspnet boostorg BSODs Cac COINIT dahall DEFCON developercommunity fde fea fmtlib hotkeys HPCON isocpp llvm mintty msvcrtd Nc NVDA pinam QOL remoting UNINIT Unk 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/1c414a7723158887c8f4217a3fd4a26a71e23b65.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('"cac coinit draging hpcon infor LLVM MSVCRTD nc Remoting temrinal uninit unk "');
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).

DHowett · 2021-04-28T20:49:13Z

src/cascadia/TerminalControl/ControlInteractivity.cpp

-        const auto rowDelta = mouseDelta / (-1.0 * WHEEL_DELTA);
+        const double rowDelta = mouseDelta / (-1.0 * 120.0);


It seems very important to continue using the correct constant.

Yea I just got spooked by int math vs float math. I'll revert this bit

DHowett · 2021-04-28T20:50:53Z

src/cascadia/TerminalControl/ControlInteractivity.cpp

+        // If the new scrollbar position, rounded to an int, is at a different
+        // row, then actually update the scroll position in the core, and raise
+        // a ScrollPositionChanged to inform the control.
+        int viewTop = ::base::saturated_cast<int>(::std::round(_internalScrollbarPosition));


Hmm. Does rounding match the pre-split behavior?

It might actually be better, a little smoother in both directions now. Hard to say for sure comparing Debug vs Release

DHowett · 2021-04-28T20:52:56Z

src/cascadia/TerminalControl/ControlInteractivity.cpp

+    void ControlInteractivity::_coreScrollPositionChanged(const IInspectable& /*sender*/,
+                                                          const Control::ScrollPositionChangedArgs& args)
+    {
+        _internalScrollbarPosition = args.ViewTop();


Just making sure: this fires every single time a line is printed to the terminal for the first 9001 lines. Is adding another event recipient worth it? Is there a way you can do this without having a double version of the viewport top every time it changes? :/

You know, I was being cautious here. I'm not positive this is needed. The event might get caught by the TermControl, throttled, then have TermControl call UpdateScrollbar, which might fix it for us.

DHowett · 2021-04-28T20:53:26Z

src/cascadia/TerminalControl/TermControl.cpp

@@ -1302,8 +1302,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
            return;
        }

-        const auto newValue = static_cast<int>(args.NewValue());
-        _core->UserScrollViewport(newValue);


…st-pass

carlos-zamora

I feel like there's a good amount of auto you can keep to leverage the base::ClampedNumeric stuff. But I guess that's a bit scary.

carlos-zamora · 2021-05-03T23:34:22Z

src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp

+        // auto scrollChangedHandler = [&](auto&&, const Control::ScrollPositionChangedArgs& args) mutable {
+        //     // This mock emulates how the TermControl updates the
+        //     // interactivity's internal scrollbar when the core changes its
+        //     // viewport.
+        //     //
+        //     // In reality, the TermControl throttles scrollbar updates, and only
+        //     // calls back to UpdateScrollbar once every 60 seconds.
+        //     const auto newValue = args.ViewTop();
+        //     if (newValue != core->ScrollOffset())
+        //     {
+        //         interactivity->UpdateScrollbar(newValue);
+        //     }
+        // };
+        // core->ScrollPositionChanged(scrollChangedHandler);


carlos-zamora · 2021-05-03T23:42:08Z

src/cascadia/TerminalControl/ControlInteractivity.cpp

+                const double currentOffset = ::base::ClampedNumeric<double>(_core->ScrollOffset());
+                const double newValue = numRows + currentOffset;


I thought you'd want to keep auto currentOffset. That way you get a safe chromium math add for newValue.

carlos-zamora · 2021-05-03T23:43:07Z

src/cascadia/TerminalControl/ControlInteractivity.cpp

@@ -428,35 +430,50 @@ namespace winrt::Microsoft::Terminal::Control::implementation
    // - Scroll the visible viewport in response to a mouse wheel event.
    // Arguments:
    // - mouseDelta: the mouse wheel delta that triggered this event.
-    // - point: the location of the mouse during this event
+    // - pixelPosition: the location of the mouse during this event


Suggested change

// - pixelPosition: the location of the mouse during this event

// - pixelPosition: the pixel location of the mouse during this event

eh. i hate when the name is obvious and we are forced to document the obvious name

carlos-zamora · 2021-05-03T23:45:08Z

src/cascadia/TerminalControl/ControlInteractivity.cpp

+        const int currentInternalRow = ::base::saturated_cast<int>(::std::round(_internalScrollbarPosition));
+        const int currentCoreRow = _core->ScrollOffset();
+        const double currentOffset = currentInternalRow == currentCoreRow ?
+                                         _internalScrollbarPosition :
+                                         currentCoreRow;


Don't you want auto or base::ClampedNumerics here? Isn't there a chance that currentCoreRow won't fit in a double?

carlos-zamora · 2021-05-03T23:50:10Z

src/cascadia/TerminalControl/ControlInteractivity.cpp

+        // If the new scrollbar position, rounded to an int, is at a different
+        // row, then actually update the scroll position in the core, and raise
+        // a ScrollPositionChanged to inform the control.
+        int viewTop = ::base::saturated_cast<int>(::std::round(_internalScrollbarPosition));


Suggested change

int viewTop = ::base::saturated_cast<int>(::std::round(_internalScrollbarPosition));

const auto viewTop = ::base::saturated_cast<int>(::std::round(_internalScrollbarPosition));

src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp

DHowett

thanks!

ghost · 2021-05-04T22:53:44Z

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.

ghost · 2021-05-25T19:09:09Z

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

Handy links:

zadjii-msft added 7 commits April 28, 2021 10:32

This is a test for #9955.c

5d05e95

Nevermind, this is the real test

167ff7c

Fixes #9955.C

9d33e89

Add a test for #9955.a

be7ea6f

Fixes #9955.a

4f4df01

ironic that all these bugs are all in the pixelPosition code that I yeeted in late on the last Wednesday I was working on the original PR

This fixes #9955.b

33d29fa

add tests for #9955.b as well

0fefc5b

DHowett requested changes Apr 28, 2021

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Apr 28, 2021

zadjii-msft added 3 commits April 29, 2021 07:59

Merge remote-tracking branch 'origin/main' into dev/migrie/b/9955-fir…

0ad3770

…st-pass

turns out, we can get away without the event

7a2bc6a

whoops, dead code

9fb46b1

carlos-zamora approved these changes May 3, 2021

View reviewed changes

DHowett reviewed May 4, 2021

View reviewed changes

src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp Outdated Show resolved Hide resolved

Update src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp

20018bd

DHowett approved these changes May 4, 2021

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label May 4, 2021

ghost merged commit c3ca94c into main May 4, 2021

ghost deleted the dev/migrie/b/9955-first-pass branch May 4, 2021 22:54

zadjii-msft mentioned this pull request May 21, 2021

Interactivity bugs post-#9820 #9955

Closed

5 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First three interactivity fixes #9980

First three interactivity fixes #9980

zadjii-msft commented Apr 28, 2021

github-actions bot commented Apr 28, 2021

DHowett Apr 28, 2021

zadjii-msft Apr 28, 2021

DHowett Apr 28, 2021

zadjii-msft Apr 28, 2021

DHowett Apr 28, 2021

zadjii-msft Apr 28, 2021

DHowett Apr 28, 2021

carlos-zamora left a comment

carlos-zamora May 3, 2021

carlos-zamora May 3, 2021

carlos-zamora May 3, 2021

DHowett May 4, 2021

carlos-zamora May 3, 2021

DHowett May 4, 2021

carlos-zamora May 3, 2021

DHowett left a comment

ghost commented May 4, 2021

ghost commented May 25, 2021

		const auto rowDelta = mouseDelta / (-1.0 * WHEEL_DELTA);
		const double rowDelta = mouseDelta / (-1.0 * 120.0);

		const double currentOffset = ::base::ClampedNumeric<double>(_core->ScrollOffset());
		const double newValue = numRows + currentOffset;

	// - pixelPosition: the location of the mouse during this event
	// - pixelPosition: the pixel location of the mouse during this event

	int viewTop = ::base::saturated_cast<int>(::std::round(_internalScrollbarPosition));
	const auto viewTop = ::base::saturated_cast<int>(::std::round(_internalScrollbarPosition));

First three interactivity fixes #9980

First three interactivity fixes #9980

Conversation

zadjii-msft commented Apr 28, 2021

Summary of the Pull Request

A: Mousedown, select, SCROLL does a weird thing with endpoints that doesn't happen in stable

B: Trackpad scrolling down with small increments seems buggy

C: Looks like there's a selection issue when you click and drag too quickly.

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

github-actions bot commented Apr 28, 2021

Misspellings found, please review:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

ghost commented May 4, 2021

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.

ghost commented May 25, 2021

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.