Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pane focus movement doesn't remember where it came from #4692

Open
keithn opened this issue Feb 22, 2020 · 9 comments
Open

Pane focus movement doesn't remember where it came from #4692

keithn opened this issue Feb 22, 2020 · 9 comments
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal.
Milestone

Comments

@keithn
Copy link

keithn commented Feb 22, 2020

Steps to reproduce

create a vertical split, then on the right pane, create a horizontal split
somewhat like this :

|-----------|
|    |  2   |
|  1 |______|
|    |   3  |
|____|______|

if you move focus from 1 to 2 by moveFocus right
then move focus from 2 to 3 by moveFocus down
then move focus from 3 to 1 by moveFocus left
then moveFocus right

Expected behavior

The focus should go back to 3. Otherwise toggling between 1 and 3 becomes a painful, especially if 2 is ends up more complicated with more panes.

Actual behavior

focus goes to 2

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Feb 22, 2020
@DHowett-MSFT DHowett-MSFT added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Feb 24, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Feb 24, 2020
@DHowett-MSFT DHowett-MSFT added this to the Terminal v1.x milestone Feb 24, 2020
@DHowett-MSFT DHowett-MSFT added the Help Wanted We encourage anyone to jump in on these. label Feb 24, 2020
@DHowett-MSFT DHowett-MSFT changed the title Focus movement doesn't remember where it came from Pane focus movement doesn't remember where it came from Feb 24, 2020
@moswald
Copy link
Member

moswald commented Feb 27, 2020

Agreed. Movement between panes should be MRU, rather than through move left/right/up/down which is ambiguous in a lot of cases.

ghost pushed a commit that referenced this issue Jan 11, 2021
## Summary of the Pull Request

Adds support for the `move-focus` subcommand to `wt.exe`. This subcommand works _exactly_ like `moveFocus(up|down|left|right)`. 

## References
* Will surely conflict with #8183
* Is goodness even in the world where #5464 exists

## PR Checklist
* [x] Closes #6580 
* [x] I work here
* [x] Tests added/passed
* [x] Docs PR: MicrosoftDocs/terminal#209

## Detailed Description of the Pull Request / Additional comments

Bear with me, I wrote this before paternity leave, so code might be a bit stale.

Oddly, after startup, this _does not_ leave the focus on the pane you moved to. If you `move-focus` during startup, at the end of startup, we'll still focus a _random_ pane. This is because the terminal still auto-focus a TermControl when it's done with layout. While we'll maintain the active control just fine during the startup, at the end of startup, all the controls will complete layout in a random order. 

This is no different than the startup right now. `wt sp ; sp ; sp` will focus a random pane at the end. This is left for a future someone to fix

This is also subject to #2398 / #4692. Moving in a direction isn't _totally_ reliable currently. `focus-pane -t ID` will certainly be more reliable, but this will work in the meantime?

## Validation Steps Performed

Opened probably 100 terminals, confirmed that the layout was always correct. Final focused pane was random, but the layout was right.
mpela81 pushed a commit to mpela81/terminal that referenced this issue Jan 28, 2021
## Summary of the Pull Request

Adds support for the `move-focus` subcommand to `wt.exe`. This subcommand works _exactly_ like `moveFocus(up|down|left|right)`. 

## References
* Will surely conflict with microsoft#8183
* Is goodness even in the world where microsoft#5464 exists

## PR Checklist
* [x] Closes microsoft#6580 
* [x] I work here
* [x] Tests added/passed
* [x] Docs PR: MicrosoftDocs/terminal#209

## Detailed Description of the Pull Request / Additional comments

Bear with me, I wrote this before paternity leave, so code might be a bit stale.

Oddly, after startup, this _does not_ leave the focus on the pane you moved to. If you `move-focus` during startup, at the end of startup, we'll still focus a _random_ pane. This is because the terminal still auto-focus a TermControl when it's done with layout. While we'll maintain the active control just fine during the startup, at the end of startup, all the controls will complete layout in a random order. 

This is no different than the startup right now. `wt sp ; sp ; sp` will focus a random pane at the end. This is left for a future someone to fix

This is also subject to microsoft#2398 / microsoft#4692. Moving in a direction isn't _totally_ reliable currently. `focus-pane -t ID` will certainly be more reliable, but this will work in the meantime?

## Validation Steps Performed

Opened probably 100 terminals, confirmed that the layout was always correct. Final focused pane was random, but the layout was right.
@zadjii-msft
Copy link
Member

Huh. This looks like it's the same thing as #2398, so I'm gonna clean this one out of the backlog. Thanks!

/dup #2398

@ghost
Copy link

ghost commented Feb 4, 2021

Hi! We've identified this issue as a duplicate of another one that already exists on this Issue Tracker. This specific instance is being closed in favor of tracking the concern over on the referenced thread. Thanks for your report!

@ghost ghost closed this as completed Feb 4, 2021
@ghost ghost added the Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. label Feb 4, 2021
@zadjii-msft zadjii-msft removed this from the Terminal v2.0 milestone Feb 4, 2021
@mg-christian-axelsson
Copy link

I'd say there's a subtle, non-compatible difference between this and #2398. In this ticket the user want to remember focus in a multicolumn layout while in #2398 the user want to rely on visual alignment.

To clarify this ticket, if the layout was a bit more complicated like

|----------|
|    |  2  |
|    |_____|
|  1 |  3  |
|    |_____|
|    |  4  |
|____|_____|

And we moved from 3 and move left to 1 then if we move right again we should end up at 3. In #2398 if we rely upon visual alignment we should probably choose 2 or 4 (or maybe even 3).

In my opinion behavior in this ticket makes more sense than #2398 but comes down to user preference. In an ideal world perhaps we do both and let the user decide.

@zadjii-msft
Copy link
Member

Ooooh. Okay. That's a great layout test case. Yea, I'll reopen this, to make sure we do this as well. Thanks!

IMO we should just do this always. In the "move right from 1" case, I don't think any one of 2, 3, or 4 is the obvious right one to pick. There's a good case for each of them. But moving to the most recent one - now that makes a lot of sense.

@zadjii-msft zadjii-msft reopened this Feb 4, 2021
@zadjii-msft zadjii-msft added this to the Terminal v2.0 milestone Feb 4, 2021
@zadjii-msft zadjii-msft removed the Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. label Feb 4, 2021
@mg-christian-axelsson
Copy link

Great to hear! There's of course a similar case with up/down when you have the tiling rotated 90 degrees. I haven't thought much about it and am not sure exactly how the two interact.

@Rosefield
Copy link
Contributor

I would argue that this issue and #2398 are complimentary. #2398 defines how to find "neighbor" panes (visually) and this issue gives one possible strategy from selecting from those neighbor panes. Of course there is a dedicated command for move focus to the last used pane (which may not have existed when this issue was created.)

Consider the following case

-------------------------------
|       |         |           |
|   1   |    3    |    2      |
|       |---------|           |
|       |    4    |           |
-------------------------------

Where the tree is

  / \
 /\  2
1 /\
  3 4

Right now if you have 4 selected and move right, you'll hit 2. Then, if you move left

  • current behavior selects 1 as focus
  • 2398 will have you select either 3 or 4 since they are the two neighbors of 2, and I believe 3 would be selected since it is the "first-most" neighbor pane.
  • this issue proposes to select from either 3 or 4, preferring 4 because it was more recent in history.

@zadjii-msft
Copy link
Member

this issue proposes to select from either 3 or 4, preferring 4 because it was more recent in history.

I'd reckon that 4 is the correct pane to chose between the two. Both seem like reasonable choices, but I'd bet that it's more likely that someone wants to return to where they came from rather than just "the first one". Especially if we consider the #4692 (comment) example. Say the user moved from 4->1, then tried moving right again. They probably don't want to go to 4.

To get more complicated:

|----------------|
|  1  |    |  5  |
|_____|    |_____|
|  2  |  4 |  6  |
|_____|    |_____|
|  3  |    |  7  |
|_____|____|_____|

2->4->{right}->4->{left} should probably end on 2 again, since it was the MRU of [1, 2, 3].

That being said, we can probably do #2398, then do this in a follow-up when there's a "tie" between multiple panes.

ghost pushed a commit that referenced this issue Jul 23, 2021
)

## Summary of the Pull Request
Uses the new logic to find visual neighbors of a pane to find which pane is the target when the move-focus commands are used. 

## References
It sounds like this logic will be refined later to meet #4692 

## PR Checklist
* [x] Closes #2398
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

## Validation Steps Performed
Created a grid of panes and confirmed that focus movement went to the right quadrant instead of just the first child of the sibling.
@Rosefield
Copy link
Contributor

If/when this gets implemented, I think it would be nice if it could be controlled by setting to enable/disable it.

@zadjii-msft zadjii-msft modified the milestones: Terminal v2.0, 22H2 Jan 4, 2022
@zadjii-msft zadjii-msft modified the milestones: 22H2, Backlog Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

No branches or pull requests

6 participants