D25006: Make sure -dpi value is valid

[Gabriel Craciunescu]

gcraciunescu marked 2 inline comments as done.
gcraciunescu added a comment.


  In D25006#605578 , @filipf wrote:
  
  > David's suggestion to just turn `dpiValue` into an int sounds sensible to 
me. @gcraciunescu do you want to finish this patch?
  
  
  I think you guys need to agree on something first. I don' mind if someone 
else commits a fix for that.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: gcraciunescu, filipf, #plasma, ngraham, davidedmundson
Cc: davidedmundson, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25006: Make sure -dpi value is valid

[Filip Fila]

filipf added a comment.


  David's suggestion to just turn `dpiValue` into an int sounds sensible to me. 
@gcraciunescu do you want to finish this patch?

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: gcraciunescu, filipf, #plasma, ngraham, davidedmundson
Cc: davidedmundson, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25006: Make sure -dpi value is valid

[Gabriel Craciunescu]

gcraciunescu marked 2 inline comments as done.
gcraciunescu added a comment.


  
  
  > Though I'm not sure I fully understand why we have this sort of wrong value 
in the config anyway?
  
  The problem coming from force font DPI code so far I see.
  Now the min/max & steps to scale down/up are fixed in the UI but any reset to 
default
  enable/disable a DPI value there will write a forceFontDPI=0 in the config 
file which seems to mean disabled.
  I am not sure why we need to write a 0 on reset/disable since default has 
nothing also.
  
  However, even if we fix there we may still want to be sure the value is a 
number and >= 24, at least in my opinion.
  People do crazy stuff even writing manually or with scripts to configuration 
files.
  
  > Also do we want
  > 
  >   } else {
  >   args[QStringLiteral("kde_settings.conf/X11/ServerArguments")] = 
QString();
  >   }
  > 
  > 
  > so that a broken DPI wipes the old config entry out? Not sure.
  
  Not sure about that either but maybe a good idea.

INLINE COMMENTS

> ngraham wrote in advancedconfig.cpp:193
> You can combine both of these conditions into the same if statement.

I thought I should follow the coding style in the file which does
all the comparing in at least 2 lines.

> ngraham wrote in advancedconfig.cpp:195
> `>=` has a different meaning when comparing strings vs comparing integers. 
> You need to first parse or cast `dpiValue` to an `int`, then compare it to 
> the number 24.

Right,  I keep forgetting that =) Not doing much in Qt these days.

Also, QStringLiteral(X) needs to be .toInt() so I guess I drop that and use 
plain number.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: gcraciunescu, filipf, #plasma, ngraham, davidedmundson
Cc: davidedmundson, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25006: Make sure -dpi value is valid

[David Edmundson]

davidedmundson added a comment.


  Turning a number into a string and back again isn't ideal
  
  I would suggest:
  
int dpiValue = dpiConfigGroup.readEntry("forceFontDPI", -1);


if (dpiValue  > 24) {
 const QString dpiArgument = QStringLiteral("-dpi ") + 
QString::number(dpiValue);
args[QStringLiteral("kde_settings.conf/X11/ServerArguments")] = 
dpiArgument;
}
  
  Though I'm not sure I fully understand why we have this sort of wrong value 
in the config anyway?
  
  Also do we want
  
} else {
args[QStringLiteral("kde_settings.conf/X11/ServerArguments")] = 
QString();
}
  
  so that a broken DPI wipes the old config entry out? Not sure.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: gcraciunescu, filipf, #plasma, ngraham, davidedmundson
Cc: davidedmundson, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25006: Make sure -dpi value is valid

[Filip Fila]

filipf added a comment.


  Friendly ping.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: gcraciunescu, filipf, #plasma, ngraham, davidedmundson
Cc: davidedmundson, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25006: Make sure -dpi value is valid

[Filip Fila]

filipf accepted this revision.
filipf added a comment.


  Tested the patch and it no longer accepts strings or values under 24.
  
  Thanks @gcraciunescu
  
  This should be landed on the 5.17 branch, but let's wait and see if 
@davidedmundson has any additional comments.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: gcraciunescu, filipf, #plasma, ngraham, davidedmundson
Cc: davidedmundson, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25006: Make sure -dpi value is valid

[Nathaniel Graham]

ngraham added a reviewer: davidedmundson.
ngraham accepted this revision.
ngraham added a subscriber: davidedmundson.
ngraham added a comment.
This revision is now accepted and ready to land.


  @davidedmundson, does this look good to you?

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

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

To: gcraciunescu, filipf, #plasma, ngraham, davidedmundson
Cc: davidedmundson, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25006: Make sure -dpi value is valid

[Gabriel Craciunescu]

gcraciunescu updated this revision to Diff 68898.
gcraciunescu retitled this revision from "Checking dpi value being empty is not 
enough" to "Make sure -dpi value is valid".
gcraciunescu edited the summary of this revision.
gcraciunescu added a comment.


  Addressed changes requested by ngraham.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25006?vs=68882&id=68898

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

AFFECTED FILES
  src/advancedconfig.cpp

To: gcraciunescu, filipf, #plasma, ngraham
Cc: ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart