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

Fix navigation calculations and focus handling #1187

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

uvera
Copy link

@uvera uvera commented Jan 30, 2025

I encountered with multi monitor setup.

If I have my laptop display and external display like this
image

focusing to left direction did not work.
This PR also handles the case where monitors are next to each other, where left direction focusing also didn't work.

I used this JS script to test overlap logic

function checkYOverlap(monitorA, monitorB) {
    return (
        monitorA.y < monitorB.y + monitorB.height &&
        monitorA.y + monitorA.height > monitorB.y
    );
}

// Test cases
const testCases = [
    {
        name: "Partial Overlap",
        monitorA: { y: 100, height: 100 },
        monitorB: { y: 150, height: 150 },
        expected: true,
    },
    {
        name: "Complete Overlap",
        monitorA: { y: 120, height: 200 },
        monitorB: { y: 100, height: 300 },
        expected: true,
    },
    {
        name: "No Overlap (Above)",
        monitorA: { y: 50, height: 50 },
        monitorB: { y: 150, height: 100 },
        expected: false,
    },
    {
        name: "No Overlap (Below)",
        monitorA: { y: 300, height: 100 },
        monitorB: { y: 150, height: 100 },
        expected: false,
    },
    {
        name: "Edge Touch (No Overlap)",
        monitorA: { y: 100, height: 100 },
        monitorB: { y: 200, height: 100 },
        expected: false,
    },
    {
        name: "Edge Overlapping (Just Touching)",
        monitorA: { y: 100, height: 101 },
        monitorB: { y: 200, height: 100 },
        expected: true,
    },
    {
        name: "Smaller bigger 1",
        monitorA: { y: 100, height: 100 },
        monitorB: { y: 80, height: 150 },
        expected: true,
    },
    {
        name: "Smaller bigger 2",
        monitorA: { y: 80, height: 150 },
        monitorB: { y: 100, height: 100 },
        expected: true,
    }
];

// Run tests
for (const testCase of testCases) {
    const result = checkYOverlap(testCase.monitorA, testCase.monitorB);
    console.log(
        `${testCase.name}: ${result} (Expected: ${testCase.expected})`,
        result === testCase.expected ? "✅" : "❌"
    );
}

uvera added 3 commits January 30, 2025 16:46
Focus left/right direction should handle workspace switching first rather than switching output, in some cases navigating from workspace 3 -> 2 will result in switching monitors

Signed-off-by: Dusan <[email protected]>
We need to error like this since saturating sub does nothing on 0usize

Signed-off-by: Dusan <[email protected]>
Some(direction),
true,
)
match direction {
Copy link
Author

@uvera uvera Jan 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behaviour makes the most sense to me, we should go to previous or next workspace (depending on direction). Those actions also fall back to SwitchOutput if corresponding functions return InvalidWorkspaceIndex

.saturating_sub(1);
let workspace = shell.workspaces.active_num(&current_output).1;

if workspace == 0 {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is here as workspace is of type usize where 0usize.saturating_sub(1) results in 0 which isn't an invalid index. This is here to allow PreviousWorkspace action to fallback to SwitchOutput on error.

@@ -1723,12 +1723,12 @@ impl Shell {
let geo = o.geometry();
match direction {
Direction::Left | Direction::Right => {
!(geo.loc.y + geo.size.h < current_output_geo.loc.y
|| geo.loc.y > current_output_geo.loc.y + current_output_geo.size.h)
geo.loc.y < current_output_geo.loc.y + current_output_geo.size.h
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flipping the logic around helped avoid the issue where SwitchOutput to left resulted in output switch where it shouldn't as my external monitor is above my laptop regarding display.

@Drakulix
Copy link
Member

Drakulix commented Feb 4, 2025

Thanks for looking into this.

I'll take a closer look at the code once working on #1005 (very soon), which also revises shortcuts around output and workspace switching.

Given that will likely conflict a bit, I'll most likely cherry-pick parts of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants