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.

Sign up for GitHub

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

Jump to bottom

feat: implement win.setAspectRatio() on Linux #19516

Merged
merged 1 commit into from
Nov 1, 2019
Merged

feat: implement win.setAspectRatio() on Linux #19516

merged 1 commit into from
Nov 1, 2019

Conversation

erickzhao
Copy link
Member

@erickzhao erickzhao commented Jul 29, 2019 •
edited by codebytere
Loading

Description of Change

Ref #8036

Implements win.setAspectRatio() on Linux using Chromium widget's setAspectRatio function. Works on Ubuntu 19.04.

Although the Chromium API is available on both Windows and Linux, a separate PR (#18306) is already opened with a Windows-specific approach.

Limitations to this approach

  • extraSize option not available
  • Cannot unset the forced aspect ratio

Checklist

Release Notes

Notes: Implemented win.setAspectRatio() on Linux.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jul 29, 2019
@erickzhao erickzhao mentioned this pull request Jul 29, 2019
Closed
5 tasks
@erickzhao erickzhao changed the title [WIP] feat: implement BrowserWindow setAspectRatio() on Linux/Windows [WIP] feat: implement BrowserWindow setAspectRatio() on Linux Jul 29, 2019
@erickzhao erickzhao changed the title [WIP] feat: implement BrowserWindow setAspectRatio() on Linux feat: implement win.setAspectRatio() on Linux Jul 30, 2019
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jul 30, 2019
@erickzhao erickzhao requested a review from ckerr July 30, 2019 21:23
@erickzhao erickzhao removed the wip ⚒ label Jul 30, 2019
@erickzhao erickzhao requested a review from a team as a code owner August 1, 2019 17:53
MarshallOfSound
MarshallOfSound requested changes Aug 1, 2019
View reviewed changes
patches/chromium/feat_implement_unset_window_aspect_ratio_on_linux.patch Outdated Show resolved Hide resolved
patches/chromium/feat_implement_unset_window_aspect_ratio_on_linux.patch Outdated Show resolved Hide resolved
patches/chromium/feat_implement_unset_window_aspect_ratio_on_linux.patch Outdated
@@ -0,0 +1,49 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
Copy link
Member

Choose a reason for hiding this comment

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

The default should be to try upstream this patch, before we land this patch into electron can you submit upstream and see if it lands?

ckerr
ckerr reviewed Aug 4, 2019
View reviewed changes
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

Assuming other reviewers' issues are resolved, LGTM

ckerr
ckerr reviewed Aug 15, 2019
View reviewed changes
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

I agree with @MarshallOfSound that the default should be to try and upstream changes like this, but I don't think that needs to block this PR since we can patch locally in the interim.

As long as there's an open upstream ticket for this, I'm fine with landing this locally.

@erickzhao
Copy link
Member Author

@ckerr will try to get the upstream ticket up before end of week!

MarshallOfSound
MarshallOfSound approved these changes Aug 19, 2019
View reviewed changes
Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

Approving but there is a conflict

@erickzhao
Copy link
Member Author

@MarshallOfSound fixed the conflict in the .patches file by adding the incoming patch from master and removing one that was removed on master (due to being upstreamed).

also regarding this patch's upstream: will need to get my local setup ready, but might take a while because internet back home is real slow. 😬

@codebytere
Copy link
Member

@erickzhao can you rebase this?

@erickzhao
Copy link
Member Author

@codebytere Don't have write access to the repo anymore so I can't 😱

Can't seem to change the branch that this PR was from, either.

@codebytere
Copy link
Member

ah ok i can do it then :)

@codebytere codebytere force-pushed the intern/set-aspect-ratio branch from 23702e2 to 169fbf8 Compare November 1, 2019 15:09
@codebytere codebytere force-pushed the intern/set-aspect-ratio branch from 169fbf8 to d7f7792 Compare November 1, 2019 15:10
@codebytere codebytere merged commit dcf6f04 into master Nov 1, 2019
@codebytere codebytere deleted the intern/set-aspect-ratio branch November 1, 2019 16:22
@release-clerk
Copy link

release-clerk bot commented Nov 1, 2019

Release Notes Persisted

Implemented win.setAspectRatio() on Linux.

@jaehayoo
Copy link

jaehayoo commented Apr 6, 2020

@codebytere When will it be available on really important windows?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ckerr ckerr ckerr left review comments

@MarshallOfSound MarshallOfSound MarshallOfSound approved these changes

@codebytere codebytere codebytere approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@erickzhao @codebytere @jaehayoo @ckerr @MarshallOfSound