feat: directional move focus #55 #78

denismaxim0v · 2020-11-30T06:00:17Z

No description provided.

denismaxim0v · 2020-11-30T06:05:20Z

that's still wip, I'll write the tests later on today

h3nill · 2020-11-30T06:25:44Z

I have tried the branch locally and it's working great for me. Just leaving some nitpicks which came to my mind from the first look.

h3nill · 2020-11-30T06:29:22Z

src/screen.rs

+ match active_terminal {
+ Some(active) => {
+ let next_index = self
+ .terminals
+ .iter()
+ .enumerate()
+ .filter(|(_, (_, c))| {
+ c.is_directly_above(&active) && c.vertically_overlaps_with(&active)
+ })
+ .max_by_key(|(_, (_, c))| c.get_vertical_overlap_with(&active))
+ .map(|(_, (pid, _))| pid);
+ match next_index {
+ Some(p) => {
+ self.active_terminal = Some(*p);
+ ()
+ }
+ None => {
+ self.active_terminal = Some(active.pid);
+ ()
+ }
+ }
+ }
+ None => {
+ self.active_terminal = Some(active_terminal.unwrap().pid);
+ ()
+ }
+ }


I think this match statements should be reduced to if let, as it improves readability and that's what if let statements are for. And also reduces a level of indent :)

Agree, this can be reduced

h3nill · 2020-11-30T06:29:47Z

src/screen.rs

+ match active_terminal {
+ Some(active) => {
+ let next_index = self
+ .terminals
+ .iter()
+ .enumerate()
+ .filter(|(_, (_, c))| {
+ c.is_directly_right_of(&active) && c.horizontally_overlaps_with(&active)
+ })
+ .max_by_key(|(_, (_, c))| c.get_horizontal_overlap_with(&active))
+ .map(|(_, (pid, _))| pid);
+ match next_index {
+ Some(p) => {
+ self.active_terminal = Some(*p);
+ ()
+ }
+ None => {
+ self.active_terminal = Some(active.pid);
+ ()
+ }
+ }
+ }
+ None => {
+ self.active_terminal = Some(active_terminal.unwrap().pid);
+ ()
+ }
+ }


h3nill · 2020-11-30T06:30:22Z

src/screen.rs

+ match active_terminal {
+ Some(active) => {
+ let next_index = self
+ .terminals
+ .iter()
+ .enumerate()
+ .filter(|(_, (_, c))| {
+ c.is_directly_below(&active) && c.vertically_overlaps_with(&active)
+ })
+ .max_by_key(|(_, (_, c))| c.get_vertical_overlap_with(&active))
+ .map(|(_, (pid, _))| pid);
+ match next_index {
+ Some(p) => {
+ self.active_terminal = Some(*p);
+ ()
+ }
+ None => {
+ self.active_terminal = Some(active.pid);
+ ()
+ }
+ }
+ }
+ None => {
+ self.active_terminal = Some(active_terminal.unwrap().pid);
+ ()
+ }
+ }


h3nill · 2020-11-30T06:31:51Z

src/screen.rs

+ match active_terminal {
+ Some(active) => {
+ let next_index = self
+ .terminals
+ .iter()
+ .enumerate()
+ .filter(|(_, (_, c))| {
+ c.is_directly_left_of(&active) && c.horizontally_overlaps_with(&active)
+ })
+ .max_by_key(|(_, (_, c))| c.get_horizontal_overlap_with(&active))
+ .map(|(_, (pid, _))| pid);
+ match next_index {
+ Some(p) => {
+ self.active_terminal = Some(*p);
+ ()
+ }
+ None => {
+ self.active_terminal = Some(active.pid);
+ ()
+ }
+ }
+ }
+ None => {
+ self.active_terminal = Some(active_terminal.unwrap().pid);
+ ()
+ }
+ }


imsnif · 2020-11-30T16:34:58Z

Hey @denismaxim0v - on the surface this looks great. Before I go deeper into it, do you think you can add some tests for these? There are quite some edge cases here that we should probably be testing. (eg. making sure it picks the terminal with the largest overlap in each case, etc.) You can look at some of the resize tests in this repo for inspiration.

denismaxim0v · 2020-11-30T16:42:32Z

Hey @denismaxim0v - on the surface this looks great. Before I go deeper into it, do you think you can add some tests for these? There are quite some edge cases here that we should probably be testing. (eg. making sure it picks the terminal with the largest overlap in each case, etc.) You can look at some of the resize tests in this repo for inspiration.

Yup, working on it, gonna commit those tomorrow

h3nill · 2020-12-01T05:27:31Z

src/screen.rs

+ match next_index {
+ Some(p) => {
+ self.active_terminal = Some(*p);
+ }
+ None => {
+ self.active_terminal = Some(active.pid);
+ }
+ }


Thanks for fixing the previous ones, perhaps these inner ones can too be reduced.

h3nill · 2020-12-01T05:27:50Z

src/screen.rs

+ match next_index {
+ Some(p) => {
+ self.active_terminal = Some(*p);
+ }
+ None => {
+ self.active_terminal = Some(active.pid);
+ }
+ }


h3nill · 2020-12-01T05:28:10Z

src/screen.rs

+ match next_index {
+ Some(p) => {
+ self.active_terminal = Some(*p);
+ }
+ None => {
+ self.active_terminal = Some(active.pid);
+ }
+ }


h3nill · 2020-12-01T05:28:31Z

src/screen.rs

+ match next_index {
+ Some(p) => {
+ self.active_terminal = Some(*p);
+ }
+ None => {
+ self.active_terminal = Some(active.pid);
+ }
+ }


imsnif

Very cool! I'm looking forward to this feature :)

Great work. The code looks very clean and the tests are great. I left two relatively minor comments and after addressing them I think we can merge this.

imsnif · 2020-12-02T13:31:11Z

src/terminal_pane/terminal_pane.rs

+ self.get_x() + self.get_columns(),
+ other.get_x() + other.get_columns(),
+ ) - std::cmp::max(self.get_x(), other.get_x())
+ }


Do you think we can implement these on the Rect trait instead of on terminal_pane? That way when we add other pane types (eg. plugins) they will get this for free.

Yup, don't see why it can't be moved, will do

imsnif · 2020-12-02T13:32:53Z

src/tests/integration/move_focus_down.rs

+ &SPLIT_VERTICALLY,
+ &MOVE_FOCUS_UP,
+ &MOVE_FOCUS_DOWN,
+ &QUIT,


Correct me if I'm wrong, but I think here both pane candidates are of the same size, aren't they? Maybe we can adjust the size of one of them using a resize command in this input? (this is true to the other tests as well, I think)

Yup, my bad, will push it with a resize

Now that I look at it I do the vertical split two times so the panes are 25%, 25%, 50% so it's still testing whether it switches to the most overlapping one

You're right. My bad :)

imsnif

Nice solution with Self - I did not know you could do that :)

denismaxim0v added 2 commits November 30, 2020 07:56

feat: directional move focus zellij-org#55

07310db

resolving merge

ddf180a

h3nill reviewed Nov 30, 2020

View reviewed changes

denismaxim0v added 5 commits December 1, 2020 06:35

wip: refactored match into if let

e636a39

wip: basic tests

c9e4a46

feat: move to most overlapping pane tests

80f7c37

fix: formatting

b4f67b3

Merge branch 'main' into mosaic#55

7b4ca8a

h3nill suggested changes Dec 1, 2020

View reviewed changes

imsnif requested changes Dec 2, 2020

View reviewed changes

denismaxim0v added 3 commits December 4, 2020 07:26

wip: Moved the definitions to the Rect trait

3209819

Merge branch 'main' into mosaic#55

251306e

formatting

bd3252d

denismaxim0v requested a review from imsnif December 4, 2020 05:42

imsnif approved these changes Dec 4, 2020

View reviewed changes

imsnif merged commit b7b3ff7 into zellij-org:main Dec 4, 2020

jovandeginste mentioned this pull request May 10, 2023

Alt+[] also triggered by Ctrl+PageUp/PageDown #2441

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: directional move focus #55 #78

feat: directional move focus #55 #78

denismaxim0v commented Nov 30, 2020

denismaxim0v commented Nov 30, 2020

h3nill commented Nov 30, 2020

h3nill Nov 30, 2020 •

edited

Loading

denismaxim0v Nov 30, 2020

h3nill Nov 30, 2020 •

edited

Loading

h3nill Nov 30, 2020 •

edited

Loading

h3nill Nov 30, 2020 •

edited

Loading

imsnif commented Nov 30, 2020

denismaxim0v commented Nov 30, 2020

h3nill Dec 1, 2020

h3nill Dec 1, 2020

h3nill Dec 1, 2020

h3nill Dec 1, 2020

imsnif left a comment

imsnif Dec 2, 2020

denismaxim0v Dec 2, 2020

imsnif Dec 2, 2020

denismaxim0v Dec 2, 2020

denismaxim0v Dec 3, 2020

imsnif Dec 3, 2020

imsnif left a comment

feat: directional move focus #55 #78

feat: directional move focus #55 #78

Conversation

denismaxim0v commented Nov 30, 2020

denismaxim0v commented Nov 30, 2020

h3nill commented Nov 30, 2020

h3nill Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h3nill Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

h3nill Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

h3nill Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

imsnif commented Nov 30, 2020

denismaxim0v commented Nov 30, 2020

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

imsnif 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

imsnif left a comment

Choose a reason for hiding this comment

h3nill Nov 30, 2020 •

edited

Loading

h3nill Nov 30, 2020 •

edited

Loading

h3nill Nov 30, 2020 •

edited

Loading

h3nill Nov 30, 2020 •

edited

Loading