Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(Settings): remove UI scaling option panel #14741

Merged
merged 1 commit into from
May 14, 2024
Merged

feat(Settings): remove UI scaling option panel #14741

merged 1 commit into from
May 14, 2024

Conversation

caybro
Copy link
Member

@caybro caybro commented May 13, 2024

What does the PR do

  • as discussed with Design, do not allow the user to select the UI scale option from Settings as it causes many problems; instead rely on the OS/Qt HighDPI support to handle the scaling for us
  • note however, for advanced users, it's still possible to specify the UI scale manually by exporting the standard QT_SCALE_FACTOR prior to starting the app
  • in the longterm, we will come with our own scaling solution at the QML level, independent from the OS

Fixes #14137

Affected areas

Settings/Appearance

Screenshot of functionality (including design for comparison)

  • I've checked the design and this PR matches it

image

@caybro
feat(Settings): remove UI scaling option panel
e8b9e58 
- as discussed with Design, do not allow the user to select the UI scale
option from Settings as it causes many problems; instead rely on the
OS/Qt HighDPI support to handle the scaling for us
- note however, for advanced users, it's still possible to specify the
UI scale manually by exporting the standard `QT_SCALE_FACTOR` prior to
starting the app
- in the longterm, we will come with our own scaling solution at the QML
level, independent from the OS

Fixes #14137
@status-im-auto
Copy link
Member

status-im-auto commented May 13, 2024 •
edited
Loading

Jenkins Builds

❔ Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ e8b9e58 #1 2024-05-13 18:12:03 ~8 min macos/aarch64 🍎dmg
✔️ e8b9e58 #1 2024-05-13 18:14:19 ~10 min tests/nim 📄log
✔️ e8b9e58 #1 2024-05-13 18:14:44 ~10 min macos/x86_64 🍎dmg
✔️ e8b9e58 #1 2024-05-13 18:21:29 ~17 min tests/ui 📄log
✔️ e8b9e58 #1 2024-05-13 18:26:39 ~22 min linux/x86_64 📦tgz
✔️ e8b9e58 #1 2024-05-13 18:28:53 ~24 min windows/x86_64 💿exe

alexjba
alexjba approved these changes May 13, 2024
View reviewed changes
Copy link
Contributor

@alexjba alexjba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Parveshdhull Parveshdhull mentioned this pull request May 14, 2024
Closed
@caybro caybro requested a review from Seitseman May 14, 2024 09:35
Seitseman
Seitseman approved these changes May 14, 2024
View reviewed changes
Copy link
Contributor

@Seitseman Seitseman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

micieslak
micieslak approved these changes May 14, 2024
View reviewed changes
Copy link
Member

@micieslak micieslak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general, one small doubt to be considered

vendor/DOtherSide/lib/src/DOtherSide.cpp
@@ -193,12 +179,12 @@ void dos_qtwebview_initialize()

void dos_qguiapplication_try_enable_threaded_renderer()
{
if(QSysInfo::kernelType() == "darwin" && QT_VERSION < QT_VERSION_CHECK(6, 0, 0))
if(QSysInfo::kernelType() == QLatin1String("darwin") && QT_VERSION < QT_VERSION_CHECK(6, 0, 0))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrapping raw listerals into QLatin1String makes sense only in case of QT_NO_CAST_FROM_ASCII defined. Otherwise there is no benefit of that, so I would keep it simple as it was. (https://doc.qt.io/qt-5/qlatin1string.html#details)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, see https://github.com/KDE/clazy/blob/master/docs/checks/README-qstring-allocations.md, points 6 and 7; the crucial part here is that QString has an overloaded operator== for QLatin1String: https://doc.qt.io/qt-5/qstring.html#operator-eq-eq

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it makes no sense to me bc QString has also overloaded operator== for const char*, no conversion is needed. Is clazy complaining about that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it does complain... I wonder why then 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it complains, just wrap it up as you proposed to suppress it, I'm fine with that :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bool QString::operator==(const char *other) const
The other const char pointer is converted to a QString using the fromUtf8() function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mystery solved :)

jrainville
jrainville approved these changes May 14, 2024
View reviewed changes
Copy link
Member

@jrainville jrainville left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, though I don't know C++ that well.

I tested the branch too and it works fine.

RIP my beloved 125% zoom 😢 🙏

@caybro
Copy link
Member Author

caybro commented May 14, 2024

Code looks good, though I don't know C++ that well.

I tested the branch too and it works fine.

RIP my beloved 125% zoom 😢 🙏

You can still use it; just make sure that you either export or that QT_SCALE_FACTOR is in your runtime env vars

@caybro caybro merged commit 8b8af40 into master May 14, 2024
8 checks passed
@caybro caybro deleted the fix/remove-ui-scaling-settings-panel branch May 14, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@micieslak micieslak micieslak approved these changes

@Seitseman Seitseman Seitseman approved these changes

@jrainville jrainville jrainville approved these changes

@alexjba alexjba alexjba approved these changes

@noeliaSD noeliaSD Awaiting requested review from noeliaSD

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

App is able to be resized more than expected (much smaller)
6 participants
@caybro @status-im-auto @Seitseman @jrainville @micieslak @alexjba