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

Center tourWindow & dialog on primary display #144

Merged
merged 18 commits into from
Jan 18, 2018
Merged

Center tourWindow & dialog on primary display #144

Changes from 5 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
13083c1
add exportDB / ImportDB
AlphaStyle Jan 11, 2018
08c14a2
Revert "add exportDB / ImportDB"
AlphaStyle Jan 11, 2018
8843ee5
center tour window on primary display
AlphaStyle Jan 11, 2018
76d52e9
Merge branch 'dev' into multiple-displays
hql287 Jan 12, 2018
a01067b
Fixed eslint issue
hql287 Jan 12, 2018
0f5ecb1
Merge branch 'dev' into multiple-displays
hql287 Jan 12, 2018
c32a860
Merge branch 'dev' into multiple-displays
hql287 Jan 13, 2018
c626021
improve center tour window on primary display
AlphaStyle Jan 13, 2018
ec86afa
Fix app/renderers/dialog.js popup centering on primary display as well
AlphaStyle Jan 13, 2018
9ddf8af
Merge branch 'multiple-displays' of github.com:AlphaStyle/Manta into …
AlphaStyle Jan 13, 2018
4b4633d
Merge branch 'multiple-displays' of github.com:AlphaStyle/Manta into …
AlphaStyle Jan 13, 2018
3d158c0
Merge branch 'multiple-displays' of github.com:AlphaStyle/Manta into …
AlphaStyle Jan 13, 2018
430e812
forgot to change width and height to the variable
AlphaStyle Jan 13, 2018
2c71827
remove unnecessary if/else statement
AlphaStyle Jan 15, 2018
ca237a4
Merge branch 'dev' into multiple-displays
hql287 Jan 16, 2018
bd99db7
Simplified center-primary-display
hql287 Jan 17, 2018
63acc29
Renamed display to winPOS
hql287 Jan 17, 2018
ad2c3eb
prettify, lint, consistent winPOS naming
AlphaStyle Jan 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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! 👍


// Prevent Linux GPU Bug
// https://github.com/electron/electron/issues/4322
Expand All @@ -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! 😄

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();

// Creating a New Window
tourWindow = new BrowserWindow({
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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 🙂

width,
height,
show: false,
frame: false,
resizable: false,
Expand Down Expand Up @@ -248,8 +257,13 @@ function migrateData() {
},
});
// Omit old keys
return omit(migratedConfigs, ['info', 'appSettings', 'printOptions', 'test']);
}
return omit(migratedConfigs, [
'info',
'appSettings',
'printOptions',
'test',
]);
},
};
// Get the current Config
const configs = appConfig.getAll();
Expand Down