Enlarge default window size. Use (adjusted) calculation in `SizeHint` to determine minimum window width.
ClosedPublic

Authored by sharvey on Apr 16 2018, 4:51 PM.

Details

Summary

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

Test Plan
  • 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

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
zzag added a subscriber: zzag.Apr 16 2018, 5:07 PM

What if sizeHint() returns a size which is bigger than screen size? Please use QSize::boundedTo.

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

ngraham requested changes to this revision.Apr 16 2018, 5:27 PM
ngraham added inline comments.
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.

This revision now requires changes to proceed.Apr 16 2018, 5:27 PM
sharvey updated this revision to Diff 32314.Apr 16 2018, 5:38 PM
  • 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()

sharvey marked an inline comment as done.Apr 16 2018, 5:39 PM
sharvey added inline comments.
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.

sharvey retitled this revision from 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()` to Enlarge default window size. Use (adjusted) calculation in `SizeHint` to determine minimum window width..Apr 16 2018, 5:42 PM
sharvey edited the summary of this revision. (Show Details)
sharvey edited the test plan for this revision. (Show Details)
sharvey edited the summary of this revision. (Show Details)
sharvey edited the summary of this revision. (Show Details)
sharvey marked an inline comment as done.Apr 16 2018, 5:45 PM
In D12252#247508, @zzag wrote:

What if sizeHint() returns a size which is bigger than screen size? Please use QSize::boundedTo.

Can that happen, if we're basing sizeHint off the screen's physical DPI?

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

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:

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?

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?

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?

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.

sharvey updated this revision to Diff 32323.Apr 16 2018, 7:34 PM
  • Adjust Y scale to 700
sharvey marked an inline comment as done.Apr 16 2018, 8:10 PM

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.

sharvey updated this revision to Diff 32336.Apr 16 2018, 9:58 PM
  • Use devicePixelRatio for base size instead of physicalDotsPerInch
ngraham added inline comments.Apr 16 2018, 10:09 PM
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...

sharvey updated this revision to Diff 32337.Apr 16 2018, 10:15 PM
  • Remove qBound and division by 96 for pixel ratio

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.

sharvey marked an inline comment as done.Apr 16 2018, 10:23 PM
ngraham added a comment.EditedApr 16 2018, 10:30 PM

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.

User-chosen window sizes should always be saved. The only alternative is to make the window non-resizable in the first place.

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.

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.

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

sharvey edited the summary of this revision. (Show Details)Apr 16 2018, 10:37 PM
sharvey edited the test plan for this revision. (Show Details)
ngraham accepted this revision.Apr 16 2018, 10:43 PM

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.

This revision is now accepted and ready to land.Apr 16 2018, 10:43 PM
sharvey updated this revision to Diff 32341.Apr 16 2018, 10:46 PM
  • Tweak size to 1024x768

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.

68 pixels added.

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.

68 pixels added.

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.

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

zzag added a comment.Apr 16 2018, 11:10 PM

These defaults are really big, especially height. What about folks with 1366x768 screen resolution? http://gs.statcounter.com/screen-resolution-stats/desktop/worldwide

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.

In D12252#247757, @zzag wrote:

These defaults are really big, especially height. What about folks with 1366x768 screen resolution? http://gs.statcounter.com/screen-resolution-stats/desktop/worldwide

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.

sharvey updated this revision to Diff 32343.Apr 16 2018, 11:19 PM
  • Revert back to 700 for Y scale
zzag added inline comments.Apr 16 2018, 11:31 PM
app/SettingsBase.cpp
83

You forgot const.

Also, you could multiply whole QSize by factor so Qt will round width and height. ;-)

sharvey updated this revision to Diff 32351.Apr 17 2018, 12:49 AM
  • Add const, revise scale factoring for Qt rounding
sharvey marked an inline comment as done.Apr 17 2018, 12:50 AM
cfeck added a subscriber: cfeck.Apr 17 2018, 12:51 AM
cfeck added inline comments.
app/SettingsBase.cpp
131

Why? I use some systems in portrait mode, 768x1024 pixels.

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.

sharvey updated this revision to Diff 32368.Apr 17 2018, 10:21 AM
  • 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.

sharvey edited the test plan for this revision. (Show Details)Apr 17 2018, 10:25 AM
sharvey edited the test plan for this revision. (Show Details)Apr 17 2018, 10:33 AM
ngraham accepted this revision.Apr 17 2018, 1:22 PM
cfeck added a comment.Apr 17 2018, 8:30 PM

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?

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.

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.

davidedmundson requested changes to this revision.Apr 17 2018, 9:03 PM
davidedmundson added a subscriber: davidedmundson.
davidedmundson added inline comments.
app/SettingsBase.cpp
83

qreal factor = qBound(1.0, QGuiApplication::primaryScreen()->devicePixelRatio()/96., 3.0);

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.

This revision now requires changes to proceed.Apr 17 2018, 9:03 PM
cfeck added a comment.Apr 17 2018, 9:47 PM

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

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

Hehe, if you want to review some of those, they're right here, ready and waiting!

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

And don't forget about poor, poor, pitiful me, who asked for a list of things to work on. :-)

sharvey added inline comments.Apr 17 2018, 10:13 PM
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. :-)

ngraham added inline comments.Apr 22 2018, 1:15 PM
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!

sharvey updated this revision to Diff 32814.Apr 22 2018, 2:56 PM
  • Remove scale factor entirely; set minimum size
sharvey marked 4 inline comments as done.Apr 22 2018, 2:58 PM

Ping! @davidedmundson or anyone else, any remaining objections to this?

ngraham added inline comments.Apr 26 2018, 5:44 PM
app/SettingsBase.cpp
108

Do we actually need this?

sharvey updated this revision to Diff 33208.Apr 27 2018, 3:39 PM
  • Remove old, unused call to KConfig "MainWindow"
sharvey marked an inline comment as done.Apr 27 2018, 3:40 PM
sharvey added inline comments.
app/SettingsBase.cpp
108

It wasn't referenced anywhere else in the project. Probably a leftover from times long ago.

ngraham added inline comments.Apr 27 2018, 7:48 PM
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?

zzag added a comment.Apr 27 2018, 8:16 PM

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.

sharvey updated this revision to Diff 33667.May 5 2018, 12:45 AM
sharvey marked an inline comment as done.
  • Remove redundant size setting
sharvey marked 2 inline comments as done.May 5 2018, 12:46 AM
In D12252#254928, @zzag wrote:

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.

Is this issue still open? The size has been discussed a couple of times. Do I need to work up something else?

In D12252#254928, @zzag wrote:

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?

Hmm, that's true.

The new settings layout(with sidebar) is still usable with window sizes smaller than 1024x700.

Many KCMs are not, though.

davidedmundson accepted this revision.May 5 2018, 4:07 AM
This revision is now accepted and ready to land.May 5 2018, 4:07 AM
hein accepted this revision.May 9 2018, 8:56 AM
This revision was automatically updated to reflect the committed changes.