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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃崕 let Cocoa handle keeping aspect ratio on window resize #5644

Merged
merged 6 commits into from May 23, 2016

Conversation

Projects
None yet
3 participants
@leethomas
Contributor

leethomas commented May 22, 2016

Closes #5395.

@leethomas leethomas changed the title from 馃崕 let Cocoa handle keeping the aspect ratio whenever the edges are d鈥 to 馃崕 let Cocoa handle keeping aspect ratio on window resize May 22, 2016

roundf((newSize.height - extraHeightPlusFrame) * aspectRatio +
extraWidthPlusFrame);
}
[sender setAspectRatio:NSMakeSize(newSize.width, newSize.height)];

This comment has been minimized.

@danhp

danhp May 22, 2016

Member

This is better called inside BrowserWindow.setAspectRatio as setting the ratio on every resize is a bit off.

This comment has been minimized.

@leethomas

leethomas May 22, 2016

Contributor

you're right, I'll change it.

@@ -719,6 +706,33 @@ - (void)drawRect:(NSRect)dirtyRect {
return [window_ styleMask] & NSResizableWindowMask;
}
void NativeWindowMac::SetAspectRatio(double aspect_ratio,
const gfx::Size& extra_size) {

This comment has been minimized.

@danhp

danhp May 23, 2016

Member

line the arguments up.

}
newSize.height =
roundf((newSize.width - extraWidthPlusFrame) / aspectRatio +
extraHeightPlusFrame);

This comment has been minimized.

@danhp

danhp May 23, 2016

Member

You can probably set 馃敟 to this function given the resizing calculation is completely handled by the system.

This comment has been minimized.

@leethomas

leethomas May 23, 2016

Contributor

@danhp I don't think so, since we still want to support the extraSize optional arg. Whatever is returned from windowWillResize is what is used for the window size. While this is mentioned in the documentation, I also tested this by hard coding the method to return a 100 X 100 window, despite having set the aspect ratio to 16/9. The window size was always 100 X 100.

Using Cocoa's setAspectRatio enables dragging by the left and right edges to expand the window. I couldn't get this to work properly by manual calculation. When I tried, I was able to get the edges to drag properly, but then dragging by the corner was causing the resize animation to stutter and spaz out. Enabling setAspectRatio fixed this, I'm guessing because the system is setting some constraints on drag actions. Just a guess.

This comment has been minimized.

@danhp

danhp May 23, 2016

Member

You're probably right about the extraSize having to be calculated here. This should do just fine. 馃槃
But it might duplicating code with the extraSize calculations in setAspectRatio.

This comment has been minimized.

@danhp

danhp May 23, 2016

Member

The 100x100 just means that the function is overriding the values.
As for the spazzing out, that's because the function is continuously called on every window size change and it sometimes doesn't know to prefer to expand width or height.

This comment has been minimized.

@leethomas

leethomas May 23, 2016

Contributor

Yeah I was just playing around with it to verify for myself that it was behaving as it should. And you're totally right about disregarding the extraSize when we first set the aspect ratio. Fixing. Thanks 馃憤

Edit: I also had tried to normalize the spazzing by trying out different tactics, choosing the greatest delta, setting thresholds, but I failed.

@@ -719,6 +706,34 @@ - (void)drawRect:(NSRect)dirtyRect {
return [window_ styleMask] & NSResizableWindowMask;
}
void NativeWindowMac::SetAspectRatio(
double aspect_ratio, const gfx::Size& extra_size) {

This comment has been minimized.

@danhp

danhp May 23, 2016

Member

Sorry I meant

void NativeWindowMac::SetAspectRatio(double aspect_ratio,
                                     const gfx::Size& extra_size) {

This comment has been minimized.

@leethomas

leethomas May 23, 2016

Contributor

I just went with the existing convention of the file. This for example: https://github.com/electron/electron/blob/master/atom/browser/native_window_mac.mm#L687-688

This comment has been minimized.

@danhp

danhp May 23, 2016

Member

That's a single argument that goes over 80

This comment has been minimized.

@leethomas

leethomas May 23, 2016

Contributor

ah i see, fixed

@danhp

This comment has been minimized.

Member

danhp commented May 23, 2016

Think this should be just fine like this.

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented May 23, 2016

馃憤

@zcbenz zcbenz merged commit 1b9bced into electron:master May 23, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment