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

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

Merged
merged 4 commits into from
May 31, 2022
Merged

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

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cc86bf3
Prevent preferences dialog from going out of screen
ninomp Dec 26, 2021
6f8f8dc
Merge branch '2.3' into 2.3-fix-prefs-dlg-positioning
ninomp May 13, 2022
cc0b7bb
Prevent preferences dialog from being shown with buttons behind taskbar
ninomp May 14, 2022
3f7fc18
Resolve code style issue pointed out by clang-format pre-commit hook
ninomp May 30, 2022
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
29 changes: 24 additions & 5 deletions src/preferences/dialog/dlgpreferences.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,8 @@ void DlgPreferences::onShow() {
}
int newX = m_geometry[0].toInt();
int newY = m_geometry[1].toInt();
int newWidth = m_geometry[2].toInt();
int newHeight = m_geometry[3].toInt();

const QScreen* const pScreen = mixxx::widgethelper::getScreen(*this);
QSize screenSpace;
Expand All @@ -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 •
edited
Loading

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)

}
newX = std::max(0, std::min(newX, screenSpace.width() - m_geometry[2].toInt()));
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()?

int windowDecorationWidth = frameGeometry().width() - geometry().width();
int windowDecorationHeight = frameGeometry().height() - geometry().height();
if (windowDecorationWidth <= 0) {
windowDecorationWidth = 2;
}
if (windowDecorationHeight <= 0) {
windowDecorationHeight = 30;
}
Comment on lines +383 to +388
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 •
edited
Loading

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

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.

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

m_geometry[0] = QString::number(newX);
m_geometry[1] = QString::number(newY);
m_geometry[2] = QString::number(newWidth);
m_geometry[3] = QString::number(newHeight);

// Update geometry with last values
#ifdef __WINDOWS__
Expand All @@ -393,8 +412,8 @@ void DlgPreferences::onShow() {
newY += offsetY;
setGeometry(newX, // x position
newY, // y position
m_geometry[2].toInt(), // width
m_geometry[3].toInt()); // height
newWidth, // width
newHeight); // height
#endif // __LINUX__ / __MACOS__
// Move is also needed on linux.
move(newX, newY);
Expand Down