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

[Scott Harvey]

This revision was automatically updated to reflect the committed changes.
Closed by commit R124:19712a1d22b2: Enlarge default window size. Use (adjusted) 
calculation in `SizeHint` to… (authored by sharvey).

REPOSITORY
  R124 System Settings

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12252?vs=33667=33892

REVISION DETAIL
  https://phabricator.kde.org/D12252

AFFECTED FILES
  app/SettingsBase.cpp

To: sharvey, ngraham, mart, davidedmundson, hein, #plasma
Cc: davidedmundson, cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart, davidedmundson
Cc: davidedmundson, cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Scott Harvey]

sharvey added a comment.


  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?

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart, davidedmundson
Cc: davidedmundson, cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Scott Harvey]

sharvey updated this revision to Diff 33667.
sharvey marked an inline comment as done.
sharvey added a comment.


  - Remove redundant size setting

REPOSITORY
  R124 System Settings

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12252?vs=33208=33667

BRANCH
  arcpatch-D12252

REVISION DETAIL
  https://phabricator.kde.org/D12252

AFFECTED FILES
  app/SettingsBase.cpp

To: sharvey, ngraham, mart, davidedmundson
Cc: davidedmundson, cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Vlad Zagorodniy]

zzag added a comment.


  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.

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart, davidedmundson
Cc: davidedmundson, cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> SettingsBase.cpp:107
>  
> +setMinimumSize(SettingsBase::sizeHint());
> +

You've already added a `setMinimumSize()` call below (under the "// enforce 
minimum window size" comment); do we need this second one here?

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart, davidedmundson
Cc: davidedmundson, cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Scott Harvey]

sharvey marked an inline comment as done.
sharvey added inline comments.

INLINE COMMENTS

> ngraham wrote in SettingsBase.cpp:108
> Do we actually need this?

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

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart, davidedmundson
Cc: davidedmundson, cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Scott Harvey]

sharvey updated this revision to Diff 33208.
sharvey added a comment.


  - Remove old, unused call to KConfig "MainWindow"

REPOSITORY
  R124 System Settings

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12252?vs=32814=33208

BRANCH
  enlarge-default-size (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12252

AFFECTED FILES
  app/SettingsBase.cpp

To: sharvey, ngraham, mart, davidedmundson
Cc: davidedmundson, cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> SettingsBase.cpp:108
> +
> +KConfigGroup mainWindowCfg = 
> KSharedConfig::openConfig()->group("MainWindow");
> +

Do we actually need this?

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart, davidedmundson
Cc: davidedmundson, cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart, davidedmundson
Cc: davidedmundson, cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  @davidedmundson?

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart, davidedmundson
Cc: davidedmundson, cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Scott Harvey]

sharvey updated this revision to Diff 32814.
sharvey added a comment.


  - Remove scale factor entirely; set minimum size

REPOSITORY
  R124 System Settings

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12252?vs=32368=32814

BRANCH
  enlarge-default-size (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12252

AFFECTED FILES
  app/SettingsBase.cpp

To: sharvey, ngraham, mart, davidedmundson
Cc: davidedmundson, cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> sharvey wrote in SettingsBase.cpp:82
> 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.  :-)

Confirmed. This works fine for me, so you can just remove the `factor` part 
entirely. Sorry for sending you down the wrong path!

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart, davidedmundson
Cc: davidedmundson, cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Scott Harvey]

sharvey added inline comments.

INLINE COMMENTS

> davidedmundson wrote in SettingsBase.cpp:82
> > 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.

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

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart, davidedmundson
Cc: davidedmundson, cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Scott Harvey]

sharvey added a comment.


  In D12252#248478 , @cfeck wrote:
  
  > 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. :-)

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart, davidedmundson
Cc: davidedmundson, cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added a comment.


  In D12252#248478 , @cfeck wrote:
  
  > 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!
  
  - https://phabricator.kde.org/D12149
  - https://phabricator.kde.org/D12077
  - https://phabricator.kde.org/D12215
  - https://phabricator.kde.org/D12218
  - https://phabricator.kde.org/D12227
  - https://phabricator.kde.org/D12240

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart, davidedmundson
Cc: davidedmundson, cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Christoph Feck]

cfeck added a comment.


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

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart, davidedmundson
Cc: davidedmundson, cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[David Edmundson]

davidedmundson requested changes to this revision.
davidedmundson added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> zzag wrote in SettingsBase.cpp:82
> You forgot `const`.
> 
> Also, you could multiply whole QSize by `factor` so Qt will round width and 
> height. ;-)

> 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.

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart, davidedmundson
Cc: davidedmundson, cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added a comment.


  In D12252#248458 , @cfeck wrote:
  
  > 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.

REPOSITORY
  R124 System Settings

BRANCH
  enlarge-default-size (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart
Cc: cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Christoph Feck]

cfeck added a comment.


  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?

REPOSITORY
  R124 System Settings

BRANCH
  enlarge-default-size (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart
Cc: cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Scott Harvey]

sharvey updated this revision to Diff 32368.
sharvey added a comment.


  - 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.

REPOSITORY
  R124 System Settings

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12252?vs=32351=32368

BRANCH
  enlarge-default-size (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12252

AFFECTED FILES
  app/SettingsBase.cpp

To: sharvey, ngraham, mart
Cc: cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R124 System Settings

BRANCH
  enlarge-default-size (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart
Cc: cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> SettingsBase.cpp:128
> +
> +// enforce minimum window size
> +setMinimumSize(SettingsBase::sizeHint());

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

REPOSITORY
  R124 System Settings

BRANCH
  enlarge-default-size (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart
Cc: cfeck, zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

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

[Scott Harvey]

sharvey updated this revision to Diff 32351.
sharvey added a comment.


  - Add `const`, revise scale factoring for Qt rounding

REPOSITORY
  R124 System Settings

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12252?vs=32343=32351

BRANCH
  enlarge-default-size (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12252

AFFECTED FILES
  app/SettingsBase.cpp

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> SettingsBase.cpp:82
> +// calculate base window size to an appropriate size
> +qreal factor = QGuiApplication::primaryScreen()->devicePixelRatio();
> +return QSize(1024*factor, 768*factor);

You forgot `const`.

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

REPOSITORY
  R124 System Settings

BRANCH
  enlarge-default-size (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Scott Harvey]

sharvey updated this revision to Diff 32343.
sharvey added a comment.


  - Revert back to 700 for Y scale

REPOSITORY
  R124 System Settings

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12252?vs=32341=32343

BRANCH
  enlarge-default-size (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12252

AFFECTED FILES
  app/SettingsBase.cpp

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R124 System Settings

BRANCH
  enlarge-default-size (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R124 System Settings

BRANCH
  enlarge-default-size (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Vlad Zagorodniy]

zzag added a comment.


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

REPOSITORY
  R124 System Settings

BRANCH
  enlarge-default-size (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Scott Harvey]

sharvey added a comment.


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

REPOSITORY
  R124 System Settings

BRANCH
  enlarge-default-size (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added a comment.


  In D12252#247750 , @sharvey wrote:
  
  > In D12252#247745 , @ngraham 
wrote:
  >
  > > 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.

REPOSITORY
  R124 System Settings

BRANCH
  enlarge-default-size (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Scott Harvey]

sharvey added a comment.


  In D12252#247745 , @ngraham wrote:
  
  > 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.

REPOSITORY
  R124 System Settings

BRANCH
  enlarge-default-size (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Scott Harvey]

sharvey updated this revision to Diff 32341.
sharvey added a comment.


  - Tweak size to 1024x768

REPOSITORY
  R124 System Settings

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12252?vs=32337=32341

BRANCH
  enlarge-default-size (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12252

AFFECTED FILES
  app/SettingsBase.cpp

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  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.

REPOSITORY
  R124 System Settings

BRANCH
  enlarge-default-size (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Scott Harvey]

sharvey added a comment.


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

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added a comment.


  In D12252#247719 , @sharvey wrote:
  
  > 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.

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Scott Harvey]

sharvey added a comment.


  > 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.

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Scott Harvey]

sharvey updated this revision to Diff 32337.
sharvey added a comment.


  - Remove qBound and division by 96 for pixel ratio

REPOSITORY
  R124 System Settings

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12252?vs=32336=32337

BRANCH
  enlarge-default-size (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12252

AFFECTED FILES
  app/SettingsBase.cpp

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> SettingsBase.cpp:82
> +// calculate base window size to an appropriate size
> +qreal factor = qBound(1.0, 
> QGuiApplication::primaryScreen()->devicePixelRatio()/96., 3.0);
> +return QSize(1024*factor, 700*factor);

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...

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Scott Harvey]

sharvey updated this revision to Diff 32336.
sharvey added a comment.


  - Use `devicePixelRatio` for base size instead of `physicalDotsPerInch`

REPOSITORY
  R124 System Settings

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12252?vs=32323=32336

BRANCH
  enlarge-default-size (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12252

AFFECTED FILES
  app/SettingsBase.cpp

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Scott Harvey]

sharvey updated this revision to Diff 32323.
sharvey added a comment.


  - Adjust Y scale to 700

REPOSITORY
  R124 System Settings

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12252?vs=32314=32323

BRANCH
  enlarge-default-size (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12252

AFFECTED FILES
  app/SettingsBase.cpp

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Scott Harvey]

sharvey added a comment.


  In D12252#247567 , @ngraham wrote:
  
  > 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.
  
  F5809984: Screenshot_20180416_142747.png 

  
  I'll leave it at 700 rather than hack away at some weird workaround.

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added a comment.


  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?

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Scott Harvey]

sharvey added a comment.


  In D12252#247563 , @ngraham wrote:
  
  > 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?
  F5809937: Screenshot_20180416_130509.png 


REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Nathaniel Graham]

ngraham added a comment.


  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

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

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

[Scott Harvey]

sharvey marked an inline comment as done.
sharvey added a comment.


  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?

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D12252

To: sharvey, ngraham, mart
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart