Call setMinimumSize() at each run to enforce a minimum size. Any enlarged size will be saved and restored via the customary method of saving window geometry.
BUG: 389617
Details
Details
- Reviewers
ngraham mart davidedmundson hein - Group Reviewers
Plasma - Commits
- R124:19712a1d22b2: Enlarge default window size. Use (adjusted) calculation in `SizeHint` to…
- Compile systemsettings with patch
- Launch system settings (systemsettings5)
- Determine that window opens at an appropriate size.
- Close System Settings and reopen; ensure size remains set
To test small or rotated screen sizes:
- Adjust display resolution to a size smaller than the standard target of 1024x700
- System settings window should launch with a margin on all sides (not monopolize entire screen space)
Diff Detail
Diff Detail
- Repository
- R124 System Settings
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
What if sizeHint() returns a size which is bigger than screen size? Please use QSize::boundedTo.
Comment Actions
Tested it out! The patch succeeds at altering the default size, but I'm not sure it accomplishes either of the goals of opening large enough to make the home screen and most KCMs look good, nor your intention of making it wide enough to always show both columns (not sure if we should shoot for that, either: Plasma should still be usable on a 1024x768 screen).
My recommendation would be to forget about opening to a size that shows both columns, and focus on opening to a large enough default size that the home screen and most KCMs look good. In my testing, QSize(1020*factor, 700*factor) opens to a much more comfortable size for me that results in nearly all KCMs presented without any scrollbars. It doesn't seem like a coincidence that this is very close to 1024x768; almost as if the KCMs were designed for that size in the first place...
With that change, this patch could be marked as fixing https://bugs.kde.org/show_bug.cgi?id=389617
| app/SettingsBase.cpp | ||
|---|---|---|
| 159 | Don't remove this, or else it becomes possible to resize the window to absurdly, unusably small sizes. The value could be changed, but we shouldn't remove this entirely. | |
Comment Actions
- Adjust calculation; re-add minimumSize() property
Updating D12252: Enlarge window size: wide enough to show both sidebars
Use (adjusted) calculation in SizeHint to determine minimum window width to display both sidebars. Manually apply size with call to resize()
| app/SettingsBase.cpp | ||
|---|---|---|
| 159 | Moved to line 130; keep together with initial window size setting. I went with an arbitrary but common 800x600 for the minimum size. | |
Comment Actions
Much better! I think I'd still prefer 700 for the height, since at 600, the following KCMs have vertical scrollbars, but lose them at 700:
- Fonts
- Screen Edges
- Window Behavior
- KDE Wallet
- File Associations
- Network
- Cookies
- Keyboard
- Energy Saving
Comment Actions
700 comes out a bit too large for me (see very bottom). Maybe Vlad is right and I need to run this through a better bounding function?
Comment Actions
Hmm, that's not the behavior I see at all. For me, factor appears to be 1 with not running a HiDPi scale factor, so I get an actual size that's essentially equal to the number it's multiplied by. (e.g. return QSize(1024*factor, 700*factor) yields window content that's actually 1024x700 ). If I run with QT_SCALE_FACTOR=1.3 systemsettings5, everything is scaled perfectly. Do you have some wonky DPI settings on your test box or something?
Comment Actions
While wonky things do frequently happen when I'm around, this doesn't seem to be the case. KInfoCenter reports a 96x96DPI display. I can't guarantee this is the highest-grade display on the market, though.
I'll leave it at 700 rather than hack away at some weird workaround.
Comment Actions
I think I found the problem: physicalDotsPerInch() is not the correct property to be checking here, as it will vary depending on the display's pixel density. You want to change that to check devicePixelRatio() instead. That gives me perfect results.
Once that's fixed, there's still one more issue here: always calling resize() this means that once the user resizes the window, that custom size will be overridden on the next launch. We want to set a better default size, not enforce a mandatory size. I wonder if we should just remove the resize() call and change setMinimumSize() to enforce 1024x700? Most KCMs look pretty bad at smaller sizes anyway.
| app/SettingsBase.cpp | ||
|---|---|---|
| 83 | you don't need to divide by 96 anymore; devicePixelRatio() gives you a number generally between 1 and 4, depending on the scale factor used. In fact, you might be able to remove the entire qBounds part too, since that won't be forwards-compatible with the next decade's 16k displays... | |
Comment Actions
Once that's fixed, there's still one more issue here: always calling resize() this means that once the user resizes the window, that custom size will be overridden on the next launch. We want to set a better default size, not enforce a mandatory size. I wonder if we should just remove the resize() call and change setMinimumSize() to enforce 1024x700? Most KCMs look pretty bad at smaller sizes anyway.
I thought about that. I figured there wasn't any harm in letting the user resize the window even if it wouldn't be saved. Do we want to go with the standard routine of saving the window geometry and restoring it the next time the user launches the application? I can work that in, maybe with a firstLaunch boolean flag that only triggers the manual setting of the window size on the very first run. The window still won't get smaller than minimumSize, but if someone wants to enlarge it, we could save that size and restore it in the future.
Comment Actions
User-chosen window sizes should always be saved. The only alternative is to make the window non-resizable in the first place.
All of this is already implemented and works as expected if you simply remove the resize() call. What needs to be done is to specify the default size without always calling resize(). Either that, or enforce 1024x700 as the minimum.
Comment Actions
All of this is already implemented and works as expected if you simply remove the resize() call. What needs to be done is to specify the default size without always calling resize(). Either that, or enforce 1024x700 as the minimum.
Well then it's already implemented! We're only calling setMinimumSize() now; no longer using resize().
(I just discovered that this code was already there as I went looking around to see where it would belong... and it's already there!)
Comment Actions
Ah, so it is! Any objections to increasing the effective minimum size to 1024x768? It seems that this is the size that the KCMs were actually designed for, so it makes sense to me as a sensible default, but let's collect some more opinions first.
Comment Actions
Heh, that wasn't a request targeted at you, but rather a question for others. :) This patch will make System Settings effectively unusable for screens smaller than 1024x768 (it's currently technically usable, but not in practice since you need to scroll nearly every KCM to see anything; it's a really bad user experience). My vote is a yes since the KCMs are themselves mostly unusable at smaller sizes, but we should still ask first.
Comment Actions
Heh, that wasn't a request targeted at you, but rather a question for others. :)
Should I back those 68 pixels back out (for now?)
Comment Actions
These defaults are really big, especially height. What about folks with 1366x768 screen resolution? http://gs.statcounter.com/screen-resolution-stats/desktop/worldwide
Comment Actions
KWin adds a titlebar to the chosen window size that is itself of variable height depending on the titlebar font size. So if we want to maintain compatibility with an actual 1024x768 display, we'll need to at reduce our minimum height by 30 px or so--possibly more. Might be saner to just to back down to 700.
Comment Actions
The issue is that the KCMs themselves mostly look good at a minimum size of 1024x768. There's unfortunately such variation in their designs that some look fine at much smaller size, and a very small number require something even better, but for the most part, I think we should choose a sensible default. For people with 1336x768 screens, it's not like allowing the window to be smaller is going to be very useful to them anyway. That screen resolution is sufficiently small and low resolution that in my experience, people using it just maximize everything anyway.
| app/SettingsBase.cpp | ||
|---|---|---|
| 83 | You forgot const. Also, you could multiply whole QSize by factor so Qt will round width and height. ;-) | |
| app/SettingsBase.cpp | ||
|---|---|---|
| 131 | Why? I use some systems in portrait mode, 768x1024 pixels. | |
Comment Actions
Perhaps we could indeed make use of QSize::boundedTo() to make sure that no dimension of the minimum size is ever bigger than a dimension of the current screen size. That would help folks like Christoph with his 768x1024 vertical screen.
I really think a minimum size of 1024x7somethingssomething represents a big improvement in usability here. With a smaller minimum size than this (given enough screen space of course), I always found myself endlessly resizing the window. With this patch, I now don't. It lends a feeling of polish and stability to System Settings that's quite unexpected and welcome.
Comment Actions
- Add checking & sizing calc for smaller or rotated screens
availableSize() returns size minus any size reserved by the window manager for taskbars, etc. Multiply by 0.9 to prevent systemsettings from going "full screen" and overlapping all other windows.
Comment Actions
I still object to enforce a minimum size. On my main system, I use a 4K screen, and having a file dialog span nearly the complete screen is irritating, and mostly unusable because I have to travel a lot to reach buttons.
What is wrong with offering a good default size, and saving the size in case the user changes it? Why is there a need to enforce the default size as the minimum size?
Comment Actions
That's a separate bug that's irrelevant to this issue. We should fix that. If you can test (I don't have a 4K screen), I can submit a patch.
What is wrong with offering a good default size, and saving the size in case the user changes it? Why is there a need to enforce the default size as the minimum size?
Can you outline the use case for having a large screen but wanting to resize the System Settings window to be so small that many KCMs become borderline unusable? We do save the user-specified size; this patch only prevents users with large screens from making the window so small that usability suffers.
| app/SettingsBase.cpp | ||
|---|---|---|
| 83 |
I know you were told to change to this, but don't do that. Qt will scale the size you give it here by the devicePixelRatio. This is scaling it twice which we don't want to do. We shouldn't need to be doing any custom high DPI code in window sizing. | |
Comment Actions
OMG, for whatever reason, I was assuming this is about the file dialog window size. You are working on too many things at once, Nathan :)
Comment Actions
And don't forget about poor, poor, pitiful me, who asked for a list of things to work on. :-)
| app/SettingsBase.cpp | ||
|---|---|---|
| 83 | So should I just run the "preferred size" (1024x760) through the boundedTo function for odd-sized screens? In other words, ditch the scale factor completely? As in: const QSize screenSize = (QGuiApplication::primaryScreen()->availableSize()*0.9); const QSize targetSize = QSize(1024, 700); return targetSize.boundedTo(screenSize); I thought I'd ask before updating the diff for the 19th time. :-) | |
| app/SettingsBase.cpp | ||
|---|---|---|
| 83 | Confirmed. This works fine for me, so you can just remove the factor part entirely. Sorry for sending you down the wrong path! | |
| app/SettingsBase.cpp | ||
|---|---|---|
| 108 | Do we actually need this? | |
| app/SettingsBase.cpp | ||
|---|---|---|
| 108 | It wasn't referenced anywhere else in the project. Probably a leftover from times long ago. | |
| app/SettingsBase.cpp | ||
|---|---|---|
| 107 | You've already added a setMinimumSize() call below (under the "// enforce minimum window size" comment); do we need this second one here? | |
Comment Actions
I've just noticed that it's calling setMinimumSize. Why? (especially, with such a big minimum size)
What about icon based view mode, i.e. the old settings layout? The new settings layout(with sidebar) is still usable with window sizes smaller than 1024x700.
Comment Actions
Is this issue still open? The size has been discussed a couple of times. Do I need to work up something else?
Comment Actions
Hmm, that's true.
The new settings layout(with sidebar) is still usable with window sizes smaller than 1024x700.
Many KCMs are not, though.