-
Notifications
You must be signed in to change notification settings - Fork 201
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.
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
Adopt reduced motion settings in more places #1760
base: master
Are you sure you want to change the base?
Conversation
nishanthkarthik
commented
Jun 24, 2024
- Refactors theme names to be enums
- Disables ripple effect when reduced motion is enabled
- Disabled transitions when reduced motion is enabled
}; | ||
static Kind kindFromString(QStringView kind); | ||
|
||
static QPalette paletteFromTheme(Kind theme); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's your reason for making this an enum? I planned to allow loading other palettes from files in the future, which is why this is currently still a string. Does this have any benefits apart from reducing a few lines of string duplication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a commit for a custom plasma style that reads off Plasma APIs directly and bypasses the QQC style. As a pre-work, I made it an enum because I wanted warnings about unimplemented switch cases. Unfortunately the results weren't that much better than using the System
style. So, I removed the second commit and left the first refactor as-is.
I did not know about the palettes idea - I think we could still emulate sum types with a std::variant
compared to using a string. I'll remove this commit if you wish :)
@@ -335,6 +335,7 @@ Rectangle { | |||
y: messageInput.positionToRectangle(messageInput.completerTriggeredAt).y - height | |||
|
|||
enter: Transition { | |||
enabled: !Settings.reducedMotion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally we were told that fade effects are fine for reducedMotion, which is why we didn't disable those. Do you have a reason to do otherwise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I mechanically disabled all transitions when reducedMotion is enabled. Are there any specific instances where you'd like to keep these? My motivation for disabling transitions is poor performance - on nvidia-wayland, the transitions stutter sometimes on a fairly powerful gpu (1060).