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

Enhancement : Avoid electron windows to overlap #9560

Merged

Conversation

OmarSdt-EC
Copy link
Contributor

With this enhancement, an electron window, when it's newly opened, has
different x and y coordinates than all the previous ones. Therefore,
there's no overlap between all the opened electron windows.

What it does

Fixes #7431

With this enhancement, an electron window, when it's newly opened, has
different x and y coordinates than all the previous ones. Therefore,
there's no overlap between all the opened electron windows.

How to test

  1. Open a new electron window (by opening a folder, a file or a workspace for example)
  2. The new window has normally different x and y coordinates than the previous one and there's no overlap.

Review checklist

Reminder for reviewers

@OmarSdt-EC OmarSdt-EC force-pushed the os/feature_newCoordinates branch from 6d30f3b to e6e1028 Compare June 7, 2021 10:31
@vince-fugnitto vince-fugnitto added the electron issues related to the electron target label Jun 7, 2021
@vince-fugnitto vince-fugnitto requested a review from kittaakos June 7, 2021 11:53
@@ -214,8 +215,8 @@ export class ElectronMainApplication {
*
* @param options
*/
async createWindow(asyncOptions: MaybePromise<TheiaBrowserWindowOptions> = this.getDefaultBrowserWindowOptions()): Promise<BrowserWindow> {
const options = await asyncOptions;
async createWindow(options: TheiaBrowserWindowOptions = this.getDefaultTheiaWindowOptions()): Promise<BrowserWindow> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you change the type from MaybePromise<TheiaBrowserWindowOptions> to TheiaBrowserWindowOptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I told to myself that the getDefaultBrowserWindowOptions() wasn't a promise. It returns only default options without waiting for them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert the type and keep the original method name. Thank you!

Copy link
Member

@paul-marechal paul-marechal Jun 16, 2021

Choose a reason for hiding this comment

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

@kittaakos I agree that this should be reverted in the context of the current PR, but long term I think the method should just take the TheiaBrowserWindowOptions and not a promise. If someone deals with a promise, they can resolve it first before calling createWindow. Is there anything preventing us from doing that, besides being breaking?

@OmarSdt-EC OmarSdt-EC force-pushed the os/feature_newCoordinates branch from e6e1028 to 4692af4 Compare June 17, 2021 02:48
@OmarSdt-EC OmarSdt-EC force-pushed the os/feature_newCoordinates branch from 4692af4 to c3875bd Compare July 19, 2021 16:21
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

I confirmed that the issue can be reproduced on master and this change addresses it nicely (tested on windows).

Code LGTM as well. 👍

With this enhancement, an electron window, when it's newly opened, has
different x and y coordinates than all the previous ones. Therefore,
there's no overlap between all the opened electron windows.
@OmarSdt-EC OmarSdt-EC force-pushed the os/feature_newCoordinates branch from c3875bd to 2a091bb Compare July 20, 2021 18:14
@msujew
Copy link
Member

msujew commented Aug 9, 2021

Someone willing to merge this?

@colin-grant-work colin-grant-work merged commit 19ace46 into eclipse-theia:master Aug 9, 2021
@msujew msujew mentioned this pull request Sep 28, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
electron issues related to the electron target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[electron] Open the new window with different x, y coordinates than the previous one
6 participants