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

System menu for terminal shows up on wrong monitor #15657

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

michalnpl
Copy link
Contributor

Summary of the Pull Request

This Pull Request addresses issue #15421. The system menu opened on the incorrect monitor in a multi-monitor setup when the terminal was maximized.

References and Relevant Issues

Issue: #15421

Detailed Description of the Pull Request / Additional comments

The problem was due to the positioning calculations for the system menu not considering that maximized windows have borders that extend beyond the monitor. This also affected the terminal in the borderless fullscreen (F11) mode.

In this PR, I've updated the positioning calculations to correctly account for the extended borders of maximized windows, ensuring that the system menu appears in the correct location on all monitors and in all display states, including the borderless fullscreen mode.

Additionally, this PR introduces extensive error checking and logging, which will assist in identifying and addressing similar issues in the future.

https://devblogs.microsoft.com/oldnewthing/20150304-00/?p=44543

Validation Steps Performed

I've validated this fix manually under various scenarios, including different monitor configurations, placements, and display states. The system menu now consistently opens correctly in all tested cases.

TAEF tests runner, no change.

PR Checklist

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Windowing Window frame, quake mode, tearout Product-Terminal The new Windows Terminal. labels Jul 5, 2023
@michalnpl michalnpl marked this pull request as ready for review July 5, 2023 19:17
@zadjii-msft zadjii-msft changed the title Michalnpl/system menu for terminal shows up on wrong monitor System menu for terminal shows up on wrong monitor Jul 10, 2023
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

as noted

{
_window->OpenSystemMenu(std::nullopt, std::nullopt);
const NonClientIslandWindow* nonClientWindow{ static_cast<NonClientIslandWindow*>(_window.get()) };
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the user doesn't have "Show tabs in titlebar" enabled? Won't this explode, since this won't be a NonClientIslandWindow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't know about this case. Let me check and iterate. Thank you for your feedback.

// - Gets the height of title bar in physical pixels.
// Return Value
// - The height of title bar in physical pixels.
[[nodiscard]] float NonClientIslandWindow::GetTitleBarPhysicalRenderHeight() const noexcept
Copy link
Member

Choose a reason for hiding this comment

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

It seems a little odd to me that we need to expose this, just so that the app host can ask us for it, then tell us to do something again.

Shouldn't IslandWindow::OpenSystemMenu be able to figure out on its own:

  • if we're maximized
  • if we should correct the menu position for the NCIW (maybe using some protected helper)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a bit odd. Let me take another stab at it with the next iteration.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jul 10, 2023
@zadjii-msft
Copy link
Member

(I'm gonna mark this as a draft for now to clear it out of our todo list before 1.19. Feel free to loop back around on it whenever)

@zadjii-msft zadjii-msft marked this pull request as draft August 28, 2023 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Windowing Window frame, quake mode, tearout Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System menu for Terminal shows up on wrong monitor
2 participants