Polish MarkMode interactions with ExpandSelectionToWord and existing selection by carlos-zamora · Pull Request #13893 · microsoft/terminal

carlos-zamora · 2022-08-31T22:32:07Z

fix: if a selection exists, mark mode should promote the existing selection instead of creating one at the cursor
fix: mark mode --> move to middle of a word --> ExpandSelectionToWord --> Shift+Right/Left had some weird behavior. Fix that
fix: Ctrl+enter on random selection clears selection. It should try to treat selection as a URL.

References

NOTE: 3b53f3c can be serviced

lhecker · 2022-08-31T22:41:02Z

src/cascadia/TerminalCore/TerminalSelection.cpp

        _selection->end = buffer.GetWordEnd(_selection->end, _wordDelimiters);
+
+        // if we're targetting both endpoints, instead just target "end"
+        if (WI_IsFlagSet(_selectionEndpoint, SelectionEndpoint::Start) && WI_IsFlagSet(_selectionEndpoint, SelectionEndpoint::End))


👉 WI_AreAllFlagsSet

I tried 😭. Compiler gave me sass about & not being defined or something like that even though this is at the top of the file:
DEFINE_ENUM_FLAG_OPERATORS(Terminal::SelectionEndpoint);

Huh... that's weird. We do use WI_AreAllFlagsSet in TermControl.cpp already.

with this specific enum type? If so, yes, thisd should work. If not, we probably need the DECLARE_ENUM_BOOLEAN_VALUES thing...

Yeah :(

terminal/src/cascadia/TerminalCore/TerminalSelection.cpp

Line 11 in fecccaa

DEFINE_ENUM_FLAG_OPERATORS(Terminal::SelectionEndpoint);

I'm curious. Imma try that declare enum bool thing.

ugh, yeah I can't find that macro. We'll figure it out later.

It's maddening that WI_AreAllFlagsClear works perfectly well nary 20 lines above.

SIMILAR: microsoft/wil#45

This is likely because of ADL failing on our nested type ;P

carlos-zamora · 2022-09-01T17:02:52Z

@lhecker added a fix for...

Ctrl+enter on random selection clears selection. Maybe it should try to treat selection as a URL?

Shouldn't be controversial but feel free to disagree haha

DHowett · 2022-09-01T19:11:56Z

NOTE: 3b53f3c can be serviced

FYI: Using individual commits makes it somewhat difficult because they require manual cherry picking and can't go through the script. No need to change right now, but just for the future :)

DHowett · 2022-09-01T20:45:56Z

DEFINE_ENUM_FLAG_OPERATORS

DHowett · 2022-09-01T20:48:29Z

src/cascadia/TerminalControl/ControlCore.cpp

+                else
+                {
+                    const auto selectedText = _terminal->GetTextBuffer().GetPlainText(_terminal->GetSelectionAnchor(), _terminal->GetSelectionEnd());
+                    _OpenHyperlinkHandlers(*this, winrt::make<OpenHyperlinkEventArgs>(winrt::hstring{ selectedText }));


isn't this just gonna cause a crash later when it turns out that the thing isn't a hyperlink?

No, we have logic somewhere such that a popup appears saying that it's not a hyperlink (I'll pull up a pic and attach it here later).

I'm pretty sure it's localized too. I'm just using what we have hahaha.

DHowett · 2022-09-07T15:58:19Z

src/cascadia/TerminalCore/TerminalSelection.cpp


-bool Terminal::SelectionIsTargetingUrl() const noexcept
-{
-    return _selectionIsTargetingUrl;


so _selectionIsTargetingUrl is now only used to know whether we are in URL jump mode?

Ah, it's only so that we don't jump to the link we just jumped to when moving forward?

Yup. It's so that we skip over the one that's currently selected.

ghost · 2022-09-07T16:07:36Z

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 · 2022-09-13T17:02:44Z

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

Handy links:

ghost · 2022-09-13T17:05:35Z

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

Handy links:

lhecker approved these changes Aug 31, 2022

View reviewed changes

carlos-zamora requested a review from DHowett August 31, 2022 22:42

carlos-zamora mentioned this pull request Aug 31, 2022

Mark mode hyperlink navigation sometimes skips over entries #13854

Closed

7 tasks

carlos-zamora added 2 commits August 31, 2022 16:55

fix: if sln exists, MaMo should promote it

d8f3e56

fix: MaMo + ExpandSelectionToWord + shift left/right

85b219a

carlos-zamora force-pushed the dev/cazamor/sln-polish branch from 5674123 to 85b219a Compare August 31, 2022 23:55

carlos-zamora added this to the Terminal v1.16 milestone Aug 31, 2022

fix: ctrl+enter in MaMo should always try to treat text as URL

55dc1ea

DHowett reviewed Sep 1, 2022

View reviewed changes

lhecker approved these changes Sep 5, 2022

View reviewed changes

DHowett reviewed Sep 7, 2022

View reviewed changes

DHowett approved these changes Sep 7, 2022

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 7, 2022

ghost merged commit 1ef4a42 into main Sep 7, 2022

ghost deleted the dev/cazamor/sln-polish branch September 7, 2022 16:07

This pull request was closed.

Conversation

carlos-zamora commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlos-zamora commented Sep 1, 2022

Uh oh!

DHowett commented Sep 1, 2022

Uh oh!

DHowett commented Sep 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 7, 2022

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.

Uh oh!

ghost commented Sep 13, 2022

Uh oh!

ghost commented Sep 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

carlos-zamora commented Aug 31, 2022 •

edited

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.