Consolidate Scrolling actions

[carlos-zamora]

Description of the new feature/enhancement

After #8459, we'll have the following scroll actions:

• scrollUp
• scrollDown
• scrollUpPage
• scrollDownPage
• scrollToTop
• scrollToBottom

This seems to be a case of us introducing new actions when we should be introducing new arguments instead. I propose the following designs:

Design 1: 3 args

• action: scroll
• arguments:
  • direction: up|down
  • mode: system|page|buffer
  • rowsToScroll: <optional int>

Personally dislike that rowsToScroll only works if you're in "system" mode. But it avoids the override in Design 2.

Design 2: Override mode

• action: scroll
• arguments:
  • direction: up|down
  • mode: system|page|buffer|<int>

This is basically Design 1, except rowsToScroll is embedded inside mode. If we go with this approach, maybe mode should be renamed to something like "amount"?

Open to discussion. Go!

[DHowett]

@zadjii-msft 2.0 -- fair?

[zadjii-msft]

Well, we've got an open PR here with #8459. Are we expecting to resolve this design before we merge that PR? IMO That PR seems perfectly sensible, and I'd merge it as-is. I do get the push to make it a parametrized action - it's the switchToTab1, switchToTab2 thing all over again.

What if it was

{ "command": { "action": "scroll", "direction":"up" } }, // defaults to the system number of lines
{ "command": { "action": "scroll", "direction":"up", "amount": 5 } },
{ "command": { "action": "scroll", "direction":"up", "amount": "page" } },
{ "command": { "action": "scroll", "direction":"up", "amount": "buffer" } },

Which is like design 2, but without the "system" enum value. Though, I really hate sticking the 5 in the enum.

{ "action": "scroll", "direction":"up" }, // defaults to the system number of lines
{ "action": "scroll", "direction":"up", "amount": 5 }, // defaults to lines
{ "action": "scroll", "direction":"up", "mode": "page" },
{ "action": "scroll", "direction":"up", "mode": "page", "amount": 5 }, // up 5 pages
{ "action": "scroll", "direction":"up", "mode": "buffer" },
{ "action": "scroll", "direction":"up", "mode": "buffer", "amount": 5  }, // sure, 5 buffers up. It's still the top.

Okay I think I like design 1 more

[carlos-zamora]

Well, we've got an open PR here with #8459. Are we expecting to resolve this design before we merge that PR? IMO That PR seems perfectly sensible, and I'd merge it as-is. I do get the push to make it a parametrized action - it's the switchToTab1, switchToTab2 thing all over again.

I'm ok with merging #8459. Mainly because idk when we/somebody would get around to implementing this.

What if it was

{ "command": { "action": "scroll", "direction":"up" } }, // defaults to the system number of lines
{ "command": { "action": "scroll", "direction":"up", "amount": 5 } },
{ "command": { "action": "scroll", "direction":"up", "amount": "page" } },
{ "command": { "action": "scroll", "direction":"up", "amount": "buffer" } },

So the default would be?:

{ "command": { "action": "scroll", "direction":"up", "amount": "system" } },

Which is like design 2, but without the "system" enum value. Though, I really hate sticking the 5 in the enum.

{ "action": "scroll", "direction":"up" }, // defaults to the system number of lines
{ "action": "scroll", "direction":"up", "amount": 5 }, // defaults to lines
{ "action": "scroll", "direction":"up", "mode": "page" },
{ "action": "scroll", "direction":"up", "mode": "page", "amount": 5 }, // up 5 pages
{ "action": "scroll", "direction":"up", "mode": "buffer" },
{ "action": "scroll", "direction":"up", "mode": "buffer", "amount": 5  }, // sure, 5 buffers up. It's still the top.

So the default would be?:

{ "action": "scroll", "direction":"up", "mode": "line", "amount": "system" }, 

I'm fine with either. I agree that your design 1 seems cleaner though. 😊