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

Center tourWindow & dialog on primary display #144

Merged
merged 18 commits into from
Jan 18, 2018

Conversation

AlphaStyle
Copy link
Contributor

Center the tour window on the primary monitor.

fixes Issue 91

Copy link
Owner

@hql287 hql287 left a comment

Choose a reason for hiding this comment

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

I don't have an external screen to test this out but I think it should be fine :)
Just curious where do mainScreen and previewScreen appear on your screens?

app.js Outdated
@@ -11,6 +11,7 @@ const omit = require('lodash').omit;

// Electron Libs
const { app, BrowserWindow, ipcMain } = require('electron');
const electron = require('electron');
Copy link
Owner

Choose a reason for hiding this comment

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

I think you can just add screen to the above line like so

const { app, BrowserWindow, ipcMain, screen } = require('electron');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the API Documentation for screen you posted in the issue.

Note: In the renderer / DevTools, window.screen is a reserved DOM property, so writing let {screen} = require('electron') will not work.

I could try and see what happens 😛 ??

Copy link
Owner

Choose a reason for hiding this comment

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

Ahh, nice catch! 👍

app.js Outdated
const width = 700;
const height = 600;
// Get Primary Display (screen / monitor)
const primaryDisplay = electron.screen.getPrimaryDisplay();
Copy link
Owner

Choose a reason for hiding this comment

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

Then

const primaryDisplay = screen.getPrimaryDisplay();

app.js Outdated
@@ -27,10 +28,18 @@ let mainWindow = null;
let previewWindow = null;

function createTourWindow() {
// Easier to work with when centering tour
// window on primary display.
const width = 700;
Copy link
Owner

Choose a reason for hiding this comment

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

Also, why do we need to declare width and height here?

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 use the width and height variable in two places, for the tour window and for centering the tour window on your display/monitor.

So if you ever have to change the width and height you only have to change it one place and the tour window will still be centered. I just thought that would make it easier, but I can of course hard code the number if you like?

Copy link
Owner

Choose a reason for hiding this comment

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

Oh I see it now, sorry 😛

Copy link
Owner

Choose a reason for hiding this comment

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

but I can of course hard code the number if you like?

And please don't! 😄

@AlphaStyle
Copy link
Contributor Author

I have 2 monitors and so far mainScreen and previewScreen appear on my primary monitor.
And I think my OS remember where it was before closing, but I can test this later today.

@hql287
Copy link
Owner

hql287 commented Jan 12, 2018

And I think my OS remember where it was before closing

It's actually one of Manta's features.

@hql287
Copy link
Owner

hql287 commented Jan 12, 2018

I think this PR is good to go.

Can you fix the eslint errors before I merge it? Thanks!

Update: Fixed!

app.js Outdated
width: 700,
height: 600,
x: primaryDisplay.bounds.x / 2 - width / 2 + primaryDisplay.bounds.x,
y: primaryDisplay.bounds.y / 2 + height / 2 + primaryDisplay.bounds.y,
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, can you explain what line 39 and line 40 do?
Wouldn't primaryDisplay.bounds.x and primaryDisplay.bounds.y be resolved to 0?

Copy link
Contributor Author

@AlphaStyle AlphaStyle Jan 12, 2018

Choose a reason for hiding this comment

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

On my monitor primaryDisplay.bounds.x would be 1920 and primaryDisplay.bounds.y would be 1080. (That would be my top left corner)

Btw I took a second look at the API and I see screen.getDisplayNearestPoint(point). I think this means I can figure out what monitor you have Manta opened on, maybe I could try and attach the Tour window in the center of the used monitor?
Never mind this, the tour window is only meant to be showen at first startup and to make this work I need to get the tour window to track the mainwindow position and that is alot of complexety for a small detail. By default Manta opens on my primary display anyway.

Copy link
Owner

Choose a reason for hiding this comment

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

Is your primaryDisplay on the right? I guess 1920x1080 is the screen size of the other one and it's placed on the left, so the primaryDisplay.bounds.x is actually 0+1920 and primaryDisplay.bounds.y would be 0 + 1080?

I'm using only my laptop and got this:

primaryDisplay:  { x: 0, y: 0, width: 1920, height: 1200 }

What would happen if you use only 1 monitor?
Would the code still work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct, my primary display is on the right.

x: primaryDisplay.bounds.x / 2 - width / 2 + primaryDisplay.bounds.x,
y: primaryDisplay.bounds.y / 2 + height / 2 + primaryDisplay.bounds.y,

x = ((0 / 2 = 0) - (700 / 2 = 350) + (0)) = -350
y = ((0 / 2 = 0) + (600 / 2 = 300) + (0)) = 300
It will not work 😝

If I only use:

x: primaryDisplay.bounds.x,
y: primaryDisplay.bounds.y,

For me this is 1920, 1080, but for you 0, 0. It will show on correct screen but not in center 😞

Copy link
Contributor Author

@AlphaStyle AlphaStyle Jan 12, 2018

Choose a reason for hiding this comment

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

const primaryDisplay = electron.screen.getPrimaryDisplay();
let x = undefined
let y = undefined

if (primaryDisplay.bounds.x === 0 && primaryDisplay.bounds.y === 0) {
  x = primaryDisplay.bounds.width / 2 - width / 2 
  y = primaryDisplay.bounds.height / 2 - height / 2
} else {
  x = primaryDisplay.bounds.x / 2 - width / 2 + primaryDisplay.bounds.x
  y = primaryDisplay.bounds.y / 2 + height / 2 + primaryDisplay.bounds.y
}

If PrimaryDisplay is Monitor 1 we can assume it start at 0, 0, right?
x = ((1920 / 2 = 960) - (700 / 2 = 350)) = 610
y = ((1080 / 2 = 540) - (600 / 2 = 300)) = 240

If PrimaryDisplay is Monitor 2 or higher:
x = ((1920 / 2 = 960) - (700 / 2 = 350) + (1920)) = 2530
y = ((0 / 2 = 0) + (600 / 2 = 300) + (0)) = 300
x and y is 1920, 0 if I have monitors horizontal to each other - [Monitor1][Monitor2]

Tests (The tour window is in center)
Two Monitors: (ran tests with both vertical and horizontal monitors)
I made Monitor 1 my primary display and tested, it worked.
I made Monitor 2 my primary display and tested, it worked.

One Monitor:
I unplugged monitor 1 so I only have 1 monitor and tested, it worked.


If you have some weird monitor configure, such as if right top corner monitor 1 is linked to bottom left corner monitor 2 than tour window wont be 100% in center, but somewhere close to it 😛

Copy link
Owner

Choose a reason for hiding this comment

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

What if we use this in all cases?

x = primaryDisplay.bounds.width / 2 - width / 2 
y = primaryDisplay.bounds.height / 2 - height / 2

Because what we want is for the tourWindow to be in the center of the primaryDisplay, regardless if the setup? (Meaning the only thing that it needs to know is the dimensions of the primaryDisplay to calculate the position of the tourWindow)

Copy link
Contributor Author

@AlphaStyle AlphaStyle Jan 13, 2018

Choose a reason for hiding this comment

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

For the sake of this example I use the same resolution for all monitors.
untitled

if I use

x = primaryDisplay.bounds.width / 2 - width / 2 
y = primaryDisplay.bounds.height / 2 - height / 2

x and y will always be the same because the resolution is the same on all monitors. This means the tour window will always show on monitor 1 regardless whether it is the primary display or not. If one monitor would be a different resolution and it was the primary display, it would still show on monitor 1 but not in center.

Let's assume monitor 5 is the primary display (worst case scenario).
x = ((1920 / 2 = 960) - (700 / 2 = 350)) = 610
y = ((1080 / 2 = 540) - (600 / 2 = 300)) = 240
This is x, y range of Monitor 1


x = primaryDisplay.bounds.x / 2 - width / 2 + primaryDisplay.bounds.x
y = primaryDisplay.bounds.y / 2 + height / 2 + primaryDisplay.bounds.y

x = ((3840 / 2 = 1920) - (700 / 2 = 350) + (3840)) = 5410
y = ((1080 / 2 = 540) + (600 / 2 = 300) + (1080)) = 1920
This would be within the x, y range of monitor 5 (3840, 1080 to 5760, 2160).
But this is still wrong because I take half of x and y coordinations which does not have the same scaling as the screen resolution.


I believe this should work.

x = primaryDisplay.bounds.width / 2 - width / 2 + primaryDisplay.bounds.x
y = primaryDisplay.bounds.height / 2 - height / 2 + primaryDisplay.bounds.y

x = ((1920 / 2 = 960) - (700 / 2 = 350) + (3840)) = 4450
y = ((1080 / 2 = 540) - (600 / 2 = 300) + (1080)) = 1320
This would be within the x, y range of monitor 5 (3840, 1080 to 5760, 2160)

I dont have more monitors to test this theory on 😛 I need someone smart to tell me whether I am wrong or not 🙂

@hql287
Copy link
Owner

hql287 commented Jan 15, 2018

@AlphaStyle Did you delete your previous comment? The one with the screens illustration in it? I didn't have time to read it carefully yesterday and wanted to check it again but I can't find it now.

@AlphaStyle
Copy link
Contributor Author

AlphaStyle commented Jan 15, 2018

I did a few changes to the PR, not sure if that made it hidden.
showconv

@hql287
Copy link
Owner

hql287 commented Jan 15, 2018

Oh ok, found it! Thanks.
You've made a lot of progress so far 👍, do you think it solves our problem yet?

@AlphaStyle
Copy link
Contributor Author

I think I made something that is supposed to be simple very complex 😝 I am sorry about that!


Since the exact same issue applyes for showModalWindow (Errors / Warnings) app/renderers/dialog.js I made this a helper function app/helpers/center-on-primary-display.js. Im not very good with naming 😛 Should I name it something else?


I belive the window will appear on the same place (center) on every monitor.

Monitor 1 (0, 0 to 1920, 1080)
x = ((1920 / 2 = 960) - (700 / 2 = 350) + (0)) = 610
y = ((1080 / 2 = 540) - (600 / 2 = 300) + (0)) = 240
1920 - 610 = 1310
1080 - 240 = 840

Monitor 2 (1920, 0 to 3840, 1080)
x = ((1920 / 2 = 960) - (700 / 2 = 350) + (1920)) = 2530
y = ((1080 / 2 = 540) - (600 / 2 = 300) + (0)) = 240
3840 - 2530 = 1310
1080 - 240 = 840

Monitor 3 (3840, 0 to 5760, 1080)
x = ((1920 / 2 = 960) - (700 / 2 = 350) + (3840)) = 4450
y = ((1080 / 2 = 540) - (600 / 2 = 300) + (0)) = 240
5760 - 4450 = 1310
1080 - 240 = 840

Monitor 4 (1920, 1080 to 3840, 2160)
x = ((1920 / 2 = 960) - (700 / 2 = 350) + (1920)) = 2530
y = ((1080 / 2 = 540) - (600 / 2 = 300) + (1080)) = 1320
3840 - 2530 = 1310
2160 - 1320 = 840

Monitor 5 (3840, 1080 to 5760, 2160)
x = ((1920 / 2 = 960) - (700 / 2 = 350) + (3840)) = 4450
y = ((1080 / 2 = 540) - (600 / 2 = 300) + (1080)) = 1320
5760 - 4450 = 1310
2160 - 1320 = 840

@hql287
Copy link
Owner

hql287 commented Jan 16, 2018

@AlphaStyle: Have to admit that I have a feeling that this should be a simple fix as well 😅.

So my theory is this: when you call screen.getPrimaryDisplay().bounds, you get an object with all x, y, width, and height values of that screen (the primary screen) calculated for you, regardless of your monitors' setup.

So from this, it's quite simple to calculate the x, and y value for the tourWindow.

Here's my super complicated algorithm to calculate the x and y position the of tourWindow based on its size and the bounds of the display

winX = displayX + ( displayWidth - winWidth ) / 2;
winY = displayY + ( displayHeight - winHeight ) / 2;

I've made a new PR based on this theory, would you be able to test it out?

Thanks!

@AlphaStyle
Copy link
Contributor Author

You would think a contributor help solving issues, I try to solve issues that are not there 😮 😛
I ended up with the same algorithm just with different syntax sugar 🙂

I guess we can close and delete this now that you have a new PR 😉

@hql287
Copy link
Owner

hql287 commented Jan 17, 2018

You would think a contributor help solving issues, I try to solve issues that are not there 😮 😛 I ended up with the same algorithm just with different syntax sugar 🙂

I think you're on track, I guess it's just the name and syntax that made it look more complex.

I guess we can close and delete this now that you have a new PR 😉

Nah, I would love to have your commits in the repo. And as we came to the same conclusion, (I'm very glad about that), I'll close my PR instead.

@hql287
Copy link
Owner

hql287 commented Jan 17, 2018

I just simplified the center-on-primary-display helper a little bit. It's working on my machine now, would you be able to run the last check before I merge this?

Thanks!

Better to be explicit.
@hql287 hql287 changed the title Center tour window on primary display Center tourWindow & dialog on primary display Jan 17, 2018
@AlphaStyle
Copy link
Contributor Author

In last commit I removed whitespace, bad comments, and did prettify and checked linting.
Changed display variable to winPOS in app/renderer/dialog.js to keep the naming consistent.

@hql287 The tour window and popup modal appears in the center on my primary display with a 2 monitor setup where monitor number 2 is primary display 🙂

@hql287 hql287 merged commit 282e615 into hql287:dev Jan 18, 2018
@hql287
Copy link
Owner

hql287 commented Jan 18, 2018

Merged. It's been a pleasure doing business with you, @AlphaStyle 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tour window has wrong position on dual monitor setup
2 participants