feat: implement win.setAspectRatio() on Windows

[MadfishDT]

Description of Change

Description of Change
Implemented win.setAspectRatio() on Windows by handling the WM_SIZING message.

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• tests are changed or added
• relevant documentation is changed or added
• PR title follows semantic commit guidelines
• [x ] PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes
Notes: Implemented win.setAspectRatio() on Windows.

[ckerr]

LGTM, though I'd like to have a second reviewer's eyes on this before merging.

[MadfishDT]

@ckerr thanks all your review and advice

[nornagon]

It looks like views already has logic for handling aspect ratios: https://chromium.googlesource.com/chromium/src/+/master/ui/views/win/hwnd_message_handler.cc#890

Is there any way we can reuse that code instead of rewriting the event handler?

[felixrieseberg]

Late to the party, but with @nornagon's comments, this looks good!

[nornagon]

Also, would love to get a test for this behaviour.

[MadfishDT]

@nornagon Thanks your advice, and I will try to reuse chromium code. chromium code look so nice. but I should find wayt how to get this function or class reference from chromium module. I will fully reference this code. very thanks ~

[CapOM]

Hi I have tried to use widget()->setAspectRatio() from chromium api but it was not working properly. The size of the frame was not correct and it also disables the resizing of the window so it looks to me we should go with @MadfishDT 's solution. Here is what I tried:

diff --git a/atom/browser/native_window_views.cc b/atom/browser/native_window_views.cc
index 4526e9365..62a6b78cb 100644
--- a/atom/browser/native_window_views.cc
+++ b/atom/browser/native_window_views.cc
@@ -1098,6 +1098,21 @@ bool NativeWindowViews::IsMenuBarVisible() {
   return root_view_->IsMenuBarVisible();
 }

+void NativeWindowViews::SetAspectRatio(double aspect_ratio,
+                                       const gfx::Size& extra_size) {
+  NativeWindow::SetAspectRatio(aspect_ratio, extra_size);
+  printf("- set aspect ratio to %f\n", aspect_ratio);
+  fflush(stdout);
+  // Reset the behaviour to default if aspect_ratio is set to 0 or less.
+  if (aspect_ratio > 0.0) {
+    printf("- set aspect ratio to %dx%d\n", 16, 9);
+    fflush(stdout);
+    widget()->SetAspectRatio(gfx::SizeF(16, 9));
+  } else {
+    widget()->SetAspectRatio(gfx::SizeF(1, 1));
+  }
+}
+
 void NativeWindowViews::SetVisibleOnAllWorkspaces(bool visible,
                                                   bool visibleOnFullScreen) {
   widget()->SetVisibleOnAllWorkspaces(visible);
diff --git a/atom/browser/native_window_views.h b/atom/browser/native_window_views.h
index 3247b3f10..ee0da1dd5 100644
--- a/atom/browser/native_window_views.h
+++ b/atom/browser/native_window_views.h
@@ -121,6 +121,8 @@ class NativeWindowViews : public NativeWindow,
   bool IsMenuBarAutoHide() override;
   void SetMenuBarVisibility(bool visible) override;
   bool IsMenuBarVisible() override;
+  void SetAspectRatio(double aspect_ratio,
+                      const gfx::Size& extra_size) override;

   void SetVisibleOnAllWorkspaces(bool visible,
                                  bool visibleOnFullScreen) override;

[codebytere]

@MadfishDT if you could please resolve conflicts that would be great

[MadfishDT]

Sorry, I had some ear infection problem. so I could not touch code until yesterday. I will fix it as soon as possible. thanks all comments

[codebytere]

no worries, thanks @MadfishDT!

[codebytere]

This looks ok to me following latest review round of updates, but i'd like @nornagon's final sign-off here given that the last feedback was his.

[erickzhao]

refs: #8036

[erickzhao]

Hi I have tried to use widget()->setAspectRatio() from chromium api but it was not working properly.

@CapOM I tried it exploring that on this branch here: https://github.com/electron/electron/tree/intern/set-aspect-ratio and it worked for me on Ubuntu.

I can test to see if it behaves as intended on Windows.
On my end, it seems like the window is 0 x 0 and can't be resized.

edit: opened #19516 as a tentative PR.

cc @MadfishDT @codebytere @nornagon

[jaehayoo]

I checked setAspectRation for Linux merged in master.
Is there any update for windows with this issue thread?
Maybe, Reviewers are waiting @nornagon's final sign-off.
@nornagon, Could you please follow up this?

Thanks,

[MadfishDT]

I can fix Conflicted files. but I can't handling progress merge step.

[codebytere]

@nornagon would you be able to give this final review?

[ssinam]

@nornagon Could you please give the final review? I'm also waiting merge of this great change since I need this function for my product.

[nornagon]

Hey, thanks for your patience.

I'm still 👎 on merging this:

• There's still no tests for this functionality.
• I think something was wrong with the attempt to reuse Chrome's functionality, because:
  1. it works on Linux
  2. one of the stated symptoms was "can't resize the window", but looking at the code in https://chromium.googlesource.com/chromium/src/+/648cbca3096b452ad67187ff3b2ae8870f5426c5/ui/views/win/hwnd_message_handler.cc#2527, it seems to explicitly handle resizing the window.
• Chrome handles different display resolutions (DIPToScreenSize), which this code doesn't.

[jaehayoo]

@nornagon
This important feature wasn't supported by electron, so I was really looking forward to this code being merged.
I hope the reviewer will help the author and merge the functionality with electron.

[ssinam]

@nornagon
This important feature wasn't supported by electron, so I was really looking forward to this code being merged.
I hope the reviewer will help the author and merge the functionality with electron.

I absolutely agree with you.

[jaehayoo]

@codebytere @nornagon @MadfishDT Is there any update?

[jaehayoo]

ref : #8036 #1336

[jaehayoo]

@MadfishDT Do you have any plans for @nornagon 's request ?

[nornagon]

This PR has not changed since my last review.

[MadfishDT]

Sorry I had some big project in my company and I changed my job(company). so I had no time for handling this issue and marchine for building electron. I am going to start fix code from next week.

[jaehayoo]

@MadfishDT Hi, Is there any update? :)

[zcbenz]

I'm closing this PR in favor of #26941, which reuses Chromium's code for implementing this API.