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

[2.3] Prevent preferences dialog from going out of screen #4613

Merged
merged 4 commits into from
May 31, 2022

Conversation

ninomp
Copy link
Contributor

@ninomp ninomp commented Jan 9, 2022

This fixes https://bugs.launchpad.net/mixxx/+bug/1938653.

Could someone check if this causes a regression on macOS?

@github-actions github-actions bot added the ui label Jan 9, 2022
@ninomp ninomp changed the title Prevent preferences dialog from going out of screen [2.3] Prevent preferences dialog from going out of screen Jan 9, 2022
@JoergAtGithub
Copy link
Member

On Windows7 the dialog height fits perfect (using the artifact download from this PR):
grafik

Comment on lines +383 to +388
if (windowDecorationWidth <= 0) {
windowDecorationWidth = 2;
}
if (windowDecorationHeight <= 0) {
windowDecorationHeight = 30;
}
Copy link
Member

Choose a reason for hiding this comment

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

this does not make sense to me. Would you mind giving some explanation?

Copy link
Member

@ronso0 ronso0 Jan 9, 2022

Choose a reason for hiding this comment

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

I'm currently debugging a similiar issue with DlgDevTools¹ and I ended up here, too:

Those magic numbers are used as default window decoration dimensions in QRect DlgPreferences::getDefaultGeometry() in case they cannot be retireved from the OS. !?

¹when I open it the initial window it's like 10.000 pixels wide

Comment on lines 393 to 394
newX = std::max(0, std::min(newX, availableWidth - newWidth));
newY = std::max(0, std::min(newY, availableHeight - newHeight));
Copy link
Member

Choose a reason for hiding this comment

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

https://en.cppreference.com/w/cpp/algorithm/clamp

Suggested change
newX = std::max(0, std::min(newX, availableWidth - newWidth));
newY = std::max(0, std::min(newY, availableHeight - newHeight));
newX = std::clamp(availableWidth - newWidth, 0, newX);
newY = std::clamp(availableHeight - newHeight, 0, newY);

Comment on lines 389 to 392
int availableWidth = screenSpace.width() - windowDecorationWidth;
int availableHeight = screenSpace.height() - windowDecorationHeight;
newWidth = std::min(newWidth, availableWidth);
newHeight = std::min(newHeight, availableHeight);
Copy link
Member

Choose a reason for hiding this comment

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

this does not make sense to me either.

@@ -371,12 +373,29 @@ void DlgPreferences::onShow() {
screenSpace = QSize(800, 600);
}
else {
screenSpace = pScreen->size();
screenSpace = pScreen->availableSize();
Copy link
Member

@ronso0 ronso0 Jan 9, 2022

Choose a reason for hiding this comment

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

availableSize returns QSize(width, height)

Did you consider pScreen->availableGeometry() here? It returns QRect(int x, int y, int width, int height)

newY = std::max(0, std::min(newY, screenSpace.height() - m_geometry[3].toInt()));

// Make sure the entire window is visible on screen and is not occluded by taskbar
// Note: Window geometry excludes window decoration
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Note: Window geometry excludes window decoration
// The window's frameGeometry() includes window decoration, geometry() does not

Copy link
Member

Choose a reason for hiding this comment

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

Do you mind updating the comments in resizeEvent() and moveEvent()?

Comment on lines +383 to +388
if (windowDecorationWidth <= 0) {
windowDecorationWidth = 2;
}
if (windowDecorationHeight <= 0) {
windowDecorationHeight = 30;
}
Copy link
Member

@ronso0 ronso0 Jan 9, 2022

Choose a reason for hiding this comment

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

I'm currently debugging a similiar issue with DlgDevTools¹ and I ended up here, too:

Those magic numbers are used as default window decoration dimensions in QRect DlgPreferences::getDefaultGeometry() in case they cannot be retireved from the OS. !?

¹when I open it the initial window it's like 10.000 pixels wide

@foss-
Copy link
Contributor

foss- commented Jan 13, 2022

Looking fine on a 15" retina MacBook Pro when opening preferences from mixxx in windowed mode.
When using mixxx in fullscreen mode and then pressing ⌘, to open preferences, the view switches away from fullscreen. Iirc this is expected behavior for qt or something like that. However e.g. Apple Pages would behave as expected by opening the preferences over the fullscreen main window and not switching away from the main window. But there may not be a workaround for that problem with qt. Also that is not the scope of this PR.

No obvious regression I was able to spot.

@github-actions
Copy link

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Apr 19, 2022
@ronso0
Copy link
Member

ronso0 commented May 28, 2022

Works nicely. Pref window is moved back into the screen if it has moved out.
Pre-commit complains about misaligned comments https://github.com/mixxxdj/mixxx/runs/6602758840?check_suite_focus=true#step:6:133

@Swiftb0y Any comments?

@ronso0 ronso0 marked this pull request as ready for review May 28, 2022 19:54
@github-actions github-actions bot removed the stale Stale issues that haven't been updated for a long time. label May 29, 2022
@Swiftb0y
Copy link
Member

LGTM once the pre-commit issues have been resolved.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

@ronso0 merge?

@ronso0
Copy link
Member

ronso0 commented May 31, 2022

yup, thank you!

@ronso0 ronso0 merged commit 09a4314 into mixxxdj:2.3 May 31, 2022
@ninomp ninomp deleted the 2.3-fix-prefs-dlg-positioning branch May 31, 2022 16:17
@ninomp
Copy link
Contributor Author

ninomp commented May 31, 2022

Thank you all for reviewing and testing this! ❤️

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

Successfully merging this pull request may close these issues.

5 participants