Skip to content

Add monitor_from_point API#2649

Closed
amrbashir wants to merge 2 commits intorust-windowing:masterfrom
amrbashir:monitor-from-point
Closed

Add monitor_from_point API#2649
amrbashir wants to merge 2 commits intorust-windowing:masterfrom
amrbashir:monitor-from-point

Conversation

@amrbashir
Copy link
Copy Markdown
Contributor

@amrbashir amrbashir commented Jan 24, 2023

  • Tested on all platforms changed
    • Windows
    • X11
    • macOS
    • wayland
    • iOS
    • android
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

Copy link
Copy Markdown
Member

@jackpot51 jackpot51 left a comment

Choose a reason for hiding this comment

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

Orbital code looks good

Copy link
Copy Markdown
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

I don't really like such an API, what's the use case for it? It seems like you wanted to pair it with the cursor position, but I think it's not a good thing to do.

What I'd suggest is to add current_monitor on the EventLoopWindowTarget, so you can get the currently focused monitor on the system, since that's all that matters and what you want anyway.

That way it'll work naturally with the rest of the MonitorHandle APIs. So for example on platforms when you don't know such a thing it'll return None, and given that most calls accept that None it could be used that way.

@amrbashir
Copy link
Copy Markdown
Contributor Author

I don't really like such an API, what's the use case for it? It seems like you wanted to pair it with the cursor position, but I think it's not a good thing to do.

Most times, it will be paired with cursor_position but another use-case for tauri users, is to center the Window on a specific monitor, so the users provide the x,y and we use it to detect the nearest monitor and center the window on that monitor, but why isn't it a good thing?

What I'd suggest is to add current_monitor on the EventLoopWindowTarget, so you can get the currently focused monitor on the system, since that's all that matters and what you want anyway.

Most cases yeah but there is a chance you may need other monitors.

@kchibisov
Copy link
Copy Markdown
Member

Most times, it will be paired with cursor_position but another use-case for tauri users, is to center the Window on a specific monitor, so the users provide the x,y and we use it to detect the nearest monitor and center the window on that monitor, but why isn't it a good thing?

But you can least monitors and provide user to pick a monitor? Winit allows setting fullscreen on any monitor and you have them available on the event loop.

@amrbashir
Copy link
Copy Markdown
Contributor Author

amrbashir commented Jan 27, 2023

But you can least monitors and provide user to pick a monitor?

by tauri users, I meant developers, not end-users.

@dhardy
Copy link
Copy Markdown
Contributor

dhardy commented Jan 28, 2023

We had a little discussion regarding units across multiple monitors in #2645. Short version: I don't think using PhysicalPosition for coordinates across multiple screens makes sense, though that is currently what other APIs use.

@kchibisov
Copy link
Copy Markdown
Member

The thing is that coordinate space system is using could be negative, don't match, and so on.

I think that there's no value in such API, given that we should use either None(let the system decide) or provide exact monitor (from `available_monitors').

The API with the points will be very limited and will add much complexity given that:

  1. Not supported by most display servers.
  2. Coordinate space could have negative values (at least on Wayland and it'll require custom protocols, yet you have no information where your cursor is), so the API need some restrictions.

My main issue is that lack of use case for such an API, since if you want a current monitor, right now you pass None to all APIs accepting output, and you'll get it. If you want a particular monitor you just pass a particular monitor. The location of outputs relative to each other is not that established concept (It's not even a rectangle shape to begin with).

@amrbashir
Copy link
Copy Markdown
Contributor Author

My main issue is that lack of use case for such an API, since if you want a current monitor

I agree with that to an extent, the use case is very much rare so I don't really want to push hard on this since the functionality could be implemented in user code by iterating over the available monitors and comparing its bounds with the point.

@amrbashir amrbashir closed this Jan 29, 2023
@amrbashir amrbashir deleted the monitor-from-point branch January 29, 2023 21:40
@amrbashir amrbashir mentioned this pull request Jan 30, 2023
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants