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

Ensure that FreeTube always opens on a display that is currently attached #2205

Closed

Conversation

absidue
Copy link
Member

@absidue absidue commented Apr 16, 2022


Ensure that FreeTube always opens on a display that is currently attached

Pull Request Type

  • Bugfix

Related issue

closes #1938

Description

This pull request fixes the issues with FreeTube being positioned on disconnected displays when started.
If the saved bounds are on a display that is connected then the window will be positioned at the saved bounds.
Alternatively if the display is disconnected the window will be placed on the nearest display to the saved bounds at the same percentage offset from the top left of the screen, this allows it to work even if the displays have different resolutions.
Finally when FreeTube is opened for the first time after the update, the offsets won't be stored in the database. As this would cause issues if the display is disconnected FreeTube falls back to just setting the window size and leaving the positioning up to the operating system, which should always place it on a connected display.

Testing (for code that is not small enough to be easily understandable)

I tested this multiple times by disconnecting my second display after closing FreeTube and checking that FreeTube was always visible. Important to note that if the dev tools are open in a second window they need to be closed before closing the main FreeTube window as FreeTube only saves the window position if there is a single window open at the time of closing.
see:

FreeTube/src/main/index.js

Lines 306 to 309 in 1eab4c8

newWindow.once('close', async () => {
if (BrowserWindow.getAllWindows().length !== 1) {
return
}

Desktop (please complete the following information):

  • OS: Windows
  • OS Version: 10
  • FreeTube version: 7878e9e

@PrestonN PrestonN enabled auto-merge (squash) April 16, 2022 16:09
@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added the PR: waiting for review For PRs that are complete, tested, and ready for review label Apr 16, 2022
Copy link
Member

Choose a reason for hiding this comment

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

Code lgtm but i dont have a second screen so i will leave testing to the others

@MM502
Copy link

MM502 commented May 19, 2022

I am sorry but this is beyond my technical/coding skills. If there is a compiled version available with this fix I am happy to test it on my dual monitor setup but as it stands this is too deep for me.

@absidue
Copy link
Member Author

absidue commented May 19, 2022

@MM502 you can download the compiled version from my fork, i've told it to build this branch. The artifacts are here: https://github.com/absidue/FreeTube/actions/runs/2354670497

@efb4f5ff-1298-471a-8973-3d47447115dc

@absidue do you know how i can reproduce the issue for myself because FT right now seems to be doing the same as your build to me. FT always seems to open correctly for me doesn't matter if the screen is connected or disconnected.

@absidue
Copy link
Member Author

absidue commented May 23, 2022

@efb4f5ff-1298-471a-8973-3d47447115dc I think this might only be a problem on Windows, which is why you can't replicate it.

@efb4f5ff-1298-471a-8973-3d47447115dc

@efb4f5ff-1298-471a-8973-3d47447115dc I think this might only be a problem on Windows, which is why you can't replicate it.

@absidue So i've just took an Win10 machine and a display to test this. Weird enough i couldnt reproduce the issue on the normal FT install but i did get the issue when i had installed your build. Im not sure if im doing anything wrong. Probably just going to wait on the others to report.

When opening the application it opened in my second monitor. Closed the application. Disconnected the monitor and opened the application. But it didnt open. Connected my display again and i still didnt see the application. Had to go into task manager to close FT. After that i could open it again

@absidue
Copy link
Member Author

absidue commented May 24, 2022

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Jun 19, 2022

@absidue i want to give this another try could u provide me some testcases?

@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link
Contributor

This PR is stale because it has been open 28 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@MM502
Copy link

MM502 commented Oct 19, 2022

I deleted my AppData folder and then re-installed v0.17.1 and the problem has gone away. There is now a new settings.db file and I am getting the following error message: Local API Error: TypeError: Cannot read properties of undefined (reading 'contents')

I don't know if this error is related to the issue or if it relates to my history not importing properly after re-intalling.

@github-actions
Copy link
Contributor

This PR is stale because it has been open 28 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions
Copy link
Contributor

This PR is stale because it has been open 28 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@ChunkyProgrammer
Copy link
Member

@absidue I have a second monitor now so I'll be able to test it, you'll need to rebase/merge development in this branch first

@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@ChunkyProgrammer
Copy link
Member

FreeTube seems to not be opening when i remove the display. So I think I'll approve the other PR

@ChunkyProgrammer
Copy link
Member

ChunkyProgrammer commented Jan 12, 2023

@efb4f5ff-1298-471a-8973-3d47447115dc I think this might only be a problem on Windows, which is why you can't replicate it.

@absidue So i've just took an Win10 machine and a display to test this. Weird enough i couldnt reproduce the issue on the normal FT install but i did get the issue when i had installed your build. Im not sure if im doing anything wrong. Probably just going to wait on the others to report.

When opening the application it opened in my second monitor. Closed the application. Disconnected the monitor and opened the application. But it didnt open. Connected my display again and i still didnt see the application. Had to go into task manager to close FT. After that i could open it again

I believe the issue is mainly for if the setup is a display below another display which is why u couldnt reproduce it.

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Jan 14, 2023

Ah okay. The setup i used was with a laptop and an external display that was hanging above the laptop.

Edit: i shared a txt document in IRC a while back with my test results. Just want to mention it to u, maybe its some useful info.

@absidue
Copy link
Member Author

absidue commented Jan 14, 2023

Is this PR still up for consideration or should I close it in favour of the other one?

@ChunkyProgrammer
Copy link
Member

ChunkyProgrammer commented Jan 31, 2023

The other pr seems to work better (for me at least) so I think I'd favor the other one. We should probably wait for @efb4f5ff-1298-471a-8973-3d47447115dc to try the other one when they have time since they have two monitors too before deciding 🙂

@efb4f5ff-1298-471a-8973-3d47447115dc

help needed #3008 (comment)

auto-merge was automatically disabled February 6, 2023 01:11

Pull request was closed

@github-actions github-actions bot removed the PR: WIP label Feb 6, 2023
@absidue absidue deleted the fix-window-positioning branch February 6, 2023 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants