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

feat: implement BrowserWindow.moveTop on X11 #16629

Merged
merged 1 commit into from Feb 7, 2019

Conversation

@CapOM
Copy link
Contributor

commented Jan 30, 2019

It was implemented on Mac and Win but not on X11.
Tested on Ubuntu 16.04 and 18.04.

Resolves #12516.

notes: Implemented BrowserWindow.moveTop() on Linux/x11

@CapOM CapOM requested review from as code owners Jan 30, 2019

@CapOM CapOM force-pushed the CapOM:implement_moveTop_on_x11 branch from a1cabf5 to 9e643f1 Jan 31, 2019

@CapOM

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2019

Just uploaded a new version as I realized that moveTop should not give focus ("move window to top position without focus")

@CapOM

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2019

@deepak1556 deepak1556 requested review from zcbenz and ckerr Jan 31, 2019

@codebytere

This comment has been minimized.

Copy link
Member

commented Feb 2, 2019

@CapOM would you mind rebasing this on latest master?

It might also be a candidate for the 5-0-x beta cycle, since it's a backwards-compatible change.

@zcbenz

zcbenz approved these changes Feb 4, 2019

@CapOM CapOM force-pushed the CapOM:implement_moveTop_on_x11 branch from 9e643f1 to 98d6ef3 Feb 5, 2019

@CapOM

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

Hi, thx for approving the merge, what are the remaining errors ? Thx

@deepak1556

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

@CapOM running npm run lint should list the error, once thats fixed it should be good to merge. Also can you rebase on latest master. Thanks!

Julien Isorce Julien Isorce
feat: implement BrowserWindow.moveTop on X11
It was implemented on Mac and Win but not on X11.
Tested on Ubuntu 16.04 and 18.04.

Also added a unit test in spec/api-browser-window-spec.js.
This test BrowserWindow.moveTop verifies that calling moveTop
on a window does not give the focus to this window.

notes: BrowserWindow.moveTop is now available on Linux/x11

#12516

@CapOM CapOM force-pushed the CapOM:implement_moveTop_on_x11 branch from 98d6ef3 to 9dd9751 Feb 7, 2019

@CapOM

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

@zcbenz @deepak1556 Hi I ran npm run lint and added a unit test

@codebytere

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

thanks @CapOM! i'll merge as soon as we go green!

@CapOM

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

The failing test seems unrelated to my patch and I can see the new unit test succeeding in Linux, Mac and Win bots. What to do next ? Thx

@codebytere

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

Failing on MAS:

webContents module getWebPreferences() API should not crash when called for devTools webContents - should not crash when called for devTools webContents

This is unrelated to the changes.

@codebytere codebytere merged commit 27bd47a into electron:master Feb 7, 2019

9 of 10 checks passed

build-mac Workflow: build-mac
Details
Absolute Zero
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Feb 7, 2019

Release Notes Persisted

Implemented BrowserWindow.moveTop() on Linux/x11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.