Add buttons for selecting commands, output to context menu

[zadjii-msft]

Adds a "Select command" and a "Select output" entry to the right-click context menu when the user has shell integration enabled. This lets the user quickly right-click on a command and select the entire commandline or all of its output.

This was a "I'm waiting for reviews" sorta idea. Seemed like a reasonable combination of features. Related to #13445, #11000.

Tested manually.

[DHowett]

@zadjii-msft you need to merge back up

[DHowett]

These don't seem to have been used; should we avoid adding em?

[DHowett]

I was alright with two copies of the same-looking code, but I am starting to get a bit worried about there being six. Are they different enough that introducing a single helper would make the code less clear?

I'm imagining...

enum Position {
Enclosing,
Prior,
Following,
} ;

std::optional<std::pair<coord, coord>> GetExtentForMarkAtPosition(Type t /* well, HasCommand isn't a type exactly... hmm */, Position pos, coord anchor);

since we are using it for...

1. is present at coord
2. get at coord
3. get before coord
4. get after coord

over the types...

1. has command
2. has output

with...

1. different anchor points
2. no anchor (???) bottom of the viewport, or the cursor, or whatever

@lhecker is this the wrong kind of deduplication?

[lhecker]

Hmm so I've had a look and the "duplicates" are basically

• SelectCommand/OutputWithAnchor
• ContextMenuSelectCommand/Output
• ShouldShowSelectCommand/Output

While doing so I found some irregularities:

• SelectOutputWithAnchor is missing the marks.empty() early return that SelectCommandWithAnchor has
• ShouldShowSelectCommand checks for HasOutput instead of HasCommand (this would need to be fixed before merging)

I like the idea of abstracting the common functionality into a GetExtentForMarkAtPosition, but I don't see how that would work for SelectCommand/OutputWithAnchor. It might also complicate the implementation and require more and more boolean parameters and functionality switches, whereas decomposing the functionality into small helpers to simplify building the 6 functions wouldn't have such a problem. Especially because of my former concern (regarding SelectCommand/OutputWithAnchor), I would personally prefer if we deduplicated the 6 functions into 2 functions instead. SelectCommand/OutputWithAnchor would be one and the other can abstract the other 4.

I'm not entirely sure at the moment what the 4 point members in ScrollMark do, but it'd be beneficial if they were til::point positions[3] instead. Without fully understanding how marks work, I'd assume that they have a command-start point (0), a point where the command-end and the output-start is (1), and the output-end (2). Searching for a command would assume that the "start" is positions[0] and the "end" is positions[1]. For the output it would be [1] and [2] respectively. If it were like that, the abstraction could be written easily by specifying either 0 or 1 as an offset parameter and do:

const auto start = m.positions[offset + 0];
const auto end = m.positions[offset + 1];

Edit: If the command and output ranges can be entirely independent of each other, I suppose a til::point_span spans[2] would still work (point_span was introduced after ScrollMark). It would allow for independent ranges, but since it's an array still allow us to sort of formalize the behavior with just an 0/1 parameter.

Without this it's a little bit more difficult but could still be done by using member pointers like so:

// Outside of the loop
const auto endMember = searchingForCommands
                     ? &ScrollMark::commandEnd
                     : &ScrollMark::outputEnd;

// Inside the loop
const auto& end = m.*endMember;
const auto hasContent = end.has_value() && *end != m.end;

The latter might be a little easier to integrate with the current code. The former would be better IMO, because it's a little bit more "formal". It's hard to explain for me, but it feels more "provable" than ternary operators and such, especially since a positions[3] / spans[2] array would represent the positions in the buffer in a sorted order.

In any case, I personally don't mind merging the current code. Apart from the one copy-paste mistake it's correct and works. I'm not concerned with maintainability here, because these functions are not "viral" and don't (negatively) affect the design of other parts of the application. They're basically an internal implementation detail, unlike classes and interfaces (like OutputCellIterator, which is the best example). I'd be fine with fixing it retroactively, although I would prefer if it was refactored in the near term.

[zadjii-msft]

m.*endMember

excuse me wat

I've never seen something so cursed yet so perfect for our need in my life

trick as the code is, end is a point and commandEnd/outputEnd is a point? so not sure I can use it right now, but this is a good idea for future polish before we mark #11000 finished.

[lhecker]

The code example is written a little bit confusingly, because it has a end variable on the stack, which is a std::optional and not the same as m.end which as a point as you said.

If it helps, the way to read member pointers is to think of them as fancy offsetof operators. Basically, when I do &ScrollMark::commandEnd, what I get is something like a uintptr_t which contains the offset in bytes the commandEnd member has inside the ScrollMark struct. Then when you use the special .* or ->* operator with such a member pointer, it'll add the offset onto the base pointer and yield the resulting value. As you can see, they're not /that/ cursed, and rather quite a bit underused, because they're the perfect fit for situations like this one (unless you go with the point_span[2] approach, which is even better IMO).

[lhecker]

I'd be happy if the functions could be abstracted away, but I've already written my thoughts about this here in a comment. I'd generally prefer if it was abstracted away, but I'm fine with a separate cleanup PR. Blocking only because of the bug, but otherwise lgtm.

[lhecker]

ShouldShowSelectCommand checks for HasOutput instead of HasCommand

[lhecker]

You can just write __assume(getSpan) if the compiler is complaining.

[lhecker]

This can also be achieved using regular function pointers like so:

void ControlCore::_contextMenuSelectMark(
    const til::point& pos,
    bool(*)(const DispatchTypes::ScrollMark&) filter,
    til::point_span(*)(const DispatchTypes::ScrollMark&) getSpan
)

The remaining code should still compile the same. This makes it easier on the compiler (std::function is a very large, very very complex template, has lots of exception handling routines and is 64 bytes large) and easier on the debugger, which will have a very easy time tracing raw function pointers.

If you'd like to I'd be happy to help you with such a change.

[DHowett]

I'm assuming these comments went unaddressed

[zadjii-msft]

whoops sorry, fixed in #15233