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.

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

Fix settings app system theme preview #4931

Merged

Conversation

toaster
Copy link
Member

@toaster toaster commented Jun 13, 2024

Description:

The settings app always shows a dark preview for “system default” no matter what the system actually said.

This PR moves the app.defaultVariant() to internal/app.DefaultVariant() and thus gives the settings app access to it.

Note that someone need to check that this still compiles for all platforms.

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

With exporting this internally, the settings app can access it and
provide a more accurate preview.
@toaster toaster requested a review from andydotxyz June 13, 2024 16:31
@coveralls
Copy link

Coverage Status

coverage: 65.553% (-0.02%) from 65.57%
when pulling 81ecd12 on toaster:bugfix/settings_app_system_theme_preview
into 389162b on fyne-io:develop.

Jacalz
Jacalz previously approved these changes Jun 13, 2024
Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Nice :)

XDG files should not be included in mobile. For non-Android non-IOS
mobile builds (e.g., running tests with `-tags mobile`) the `app_other.go`
has to be included, to provide `OpenURL` and `SendNotification`.

The whole scheme has clearly evolved over time.
Might be useful to separate the different functionalities and get a more
clear scheme in the end.
@coveralls
Copy link

Coverage Status

coverage: 65.575% (+0.005%) from 65.57%
when pulling 7bc98c6 on toaster:bugfix/settings_app_system_theme_preview
into 389162b on fyne-io:develop.

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

On this version the system default preview always seems to be dark?

@andydotxyz
Copy link
Member

Compiles on Darwin, iOS, Android, WASM - but does not seem to get the preview right on my macOS machine

@toaster
Copy link
Member Author

toaster commented Jun 14, 2024

Compiles on Darwin, iOS, Android, WASM - but does not seem to get the preview right on my macOS machine

You are right, seems I’ve broken something in between.

@toaster
Copy link
Member Author

toaster commented Jun 14, 2024

Found and fixed.

@toaster toaster requested a review from andydotxyz June 14, 2024 13:22
@coveralls
Copy link

Coverage Status

coverage: 65.639% (+0.07%) from 65.57%
when pulling 630e3d5 on toaster:bugfix/settings_app_system_theme_preview
into 389162b on fyne-io:develop.

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Looks good thanks, I just got stuck on naming.

themeNameDark = "dark"
themeNameLight = "light"
themeNameSystemLabel = "system default"
themeNameSystemSettings = ""
Copy link
Member

Choose a reason for hiding this comment

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

I was a little confused by the naming - would themeNameSystem be OK? It feels like that matches the other keys, leaving the Label variant just used for label purposes?

@toaster toaster requested a review from andydotxyz June 14, 2024 14:40
@coveralls
Copy link

Coverage Status

coverage: 65.637% (+0.07%) from 65.57%
when pulling 50676be on toaster:bugfix/settings_app_system_theme_preview
into 389162b on fyne-io:develop.

@toaster toaster merged commit 1cbc2e3 into fyne-io:develop Jun 17, 2024
12 checks passed
@toaster toaster deleted the bugfix/settings_app_system_theme_preview branch June 17, 2024 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants