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

Enhance MonitorInfo class #168

Closed
wants to merge 8 commits into from
Closed

Conversation

Jack251970
Copy link

When I was working on a project using WinUIEx, I found that the MonitorInfo class could not provide parameters related to the primary monitor.

So, I went online to search and complete this commit.

Hopefully I'll get this update in the Nuget package soon!

/// <summary>
/// Gets if the monitor is the the primary display monitor.
/// </summary>
public bool IsPrimary { get; }
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if it would be better to move line 50 to here:
public bool IsPrimary => _monitor == PInvoke.MonitorFromWindow(new(IntPtr.Zero), MONITOR_FROM_FLAGS.MONITOR_DEFAULTTOPRIMARY);
That ensures it is up to date, in case of changes, and doesn't require the extra interop call if you're never actually checking this

Copy link
Author

Choose a reason for hiding this comment

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

Oh, it makes sense.

/// <inheritdoc />
public override string ToString() => $"{Name} {RectMonitor.Width}x{RectMonitor.Height}";
public override string ToString() => $"{Name} {(IsPrimary ? " Primary" : "")} {RectMonitor.Width}x{RectMonitor.Height}";
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we should be changing ToString here, What was the reasoning for this?

Copy link
Author

Choose a reason for hiding this comment

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

Ok, we can remove this. I really don't know what should be contained in ToString function.

@Jack251970
Copy link
Author

I have edited my codes with your advice.

@Jack251970 Jack251970 changed the title Add primary information in monitor info class Enhance MonitorInfo class Mar 12, 2024
@Jack251970
Copy link
Author

Add support to get the display monitor closest to a given window.

@Jack251970
Copy link
Author

Add support for relative coordinates.

/// <remarks>
/// <note>For display monitor coordinates, the origin is the upper-left corner of the display monitor.</note>
/// </remarks>
public Point PointMonitor { get; }
Copy link
Owner

Choose a reason for hiding this comment

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

I don't quite get the point of this - it's already available in RectMonitor

public unsafe static MonitorInfo GetWindowDisplayMonitor(IntPtr hwnd)
{
var windowMonitor = PInvoke.MonitorFromWindow(new(hwnd), MONITOR_FROM_FLAGS.MONITOR_DEFAULTTONEAREST);
MonitorInfo monitorInfo = null!;
Copy link
Owner

Choose a reason for hiding this comment

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

Why the null-forgiveness? It seems to me this method could end up returning null if the condition is false.

/// </summary>
/// <param name="hwnd">Window handle</param>
/// <returns>The display monitor that is nearest to a given window.</returns>
public unsafe static MonitorInfo GetWindowDisplayMonitor(IntPtr hwnd)
Copy link
Owner

Choose a reason for hiding this comment

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

Probably better to just name this MonitorFromWindow to match the interop method. What's the difference between a "monitor" and a "display monitor" ?

@Jack251970 Jack251970 requested a review from dotMorten May 24, 2024 09:21
@Jack251970 Jack251970 closed this Jun 8, 2024
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