We need to make sure the -dpi value is something valid.
Right now we can pass everything as value to -dpi, including 0,1 or any string.
Check for the lowest possible value set from force font DPI settings
Details
- Reviewers
filipf ngraham davidedmundson - Group Reviewers
Plasma
Diff Detail
- Repository
- R123 SDDM Configuration Panel (KCM)
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Also this patch's title could use some changes. Please rewrite it in the form of a statement that explains the change or fix, not the problem.
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.
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.
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.
src/advancedconfig.cpp | ||
---|---|---|
193 | I thought I should follow the coding style in the file which does | |
195 | 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. |
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.