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

Add App Preferences for Dark Mode and Locale Override #2756

Merged
merged 28 commits into from
Nov 26, 2023
Merged

Conversation

danyeaw
Copy link
Member

@danyeaw danyeaw commented Oct 8, 2023

This PR creates a new Preferences Window to allow users to override the system dark mode settings and also override the language to English.

Fixes #2586 and #2115

  • Preferences window with dark mode override
  • Save and load Gio.Settings
  • Solution for overriding locale
  • Refactor settings in to its own module
  • Add tests

image
image

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bug fix
  • Feature
  • Chore (refactoring, formatting, local variables, other cleanup)
  • Documentation content changes

What is the current behavior?

Gaphor doesn't support any app wide preferences

Issue Number: N/A

What is the new behavior?

Gaphor has a minimum amount of preferences

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@github-actions github-actions bot added the python Pull requests that update Python code label Oct 8, 2023
@danyeaw
Copy link
Member Author

danyeaw commented Oct 8, 2023

@amolenaar I could use some architecture advice, should Settings be a separate service? It started out as something that loaded with the help module, but now we need to check the setting while loading i18n to see if we have Use English set.

@amolenaar
Copy link
Member

@amolenaar I could use some architecture advice, should Settings be a separate service? It started out as something that loaded with the help module, but now we need to check the setting while loading i18n to see if we have Use English set.

That is a very good question 😄. Let's see:

  • i18n current does not work as a service.
  • Changing will require a restart, or at least a reload of the application, since the language in all UI elements needs to change.

Maybe start with a module that takes a global config from a ~/.config/gaphor/settings file? The module provides a few functions that load the global settings. This should allow the i18n module to obtain the preferred language.

It would for sure be nice to have it as an (application) service, but then the i18n code needs to be reworked too.

UI-wise I would drop the sub-text ("Change the color scheme..."), and instead make the text "Override Dark Mode" more descriptive.

@github-actions github-actions bot added the packaging Update to packaging aspects label Oct 8, 2023
@danyeaw
Copy link
Member Author

danyeaw commented Oct 9, 2023

@amolenaar Thanks!

I made the changes to UI to remove the subtitles and try to make the descriptions more clear:
image

I created a new settings module. Gio.Settings takes care of saving and loading settings based on the schema, so I think creating a separate config file may not be needed. Do you think sharing the Settings class instance with i18n and help is OK?

@amolenaar
Copy link
Member

Do you think sharing the Settings class instance with i18n and help is OK?

I think there's not really any other way :).

This does not have any impact for cases where we use Gaphor as a library, right? Like our documentation generation.

@danyeaw
Copy link
Member Author

danyeaw commented Oct 9, 2023

Should I look at putting an exception in for our pytest-archon rules?

@danyeaw danyeaw marked this pull request as ready for review October 10, 2023 02:09
@danyeaw
Copy link
Member Author

danyeaw commented Oct 10, 2023

Never mind, I added settings as a Gaphor core module which should make archon happy.

A couple of other updates needed: I think we need an Adw Banner to popup when the language settings are changed:
image

I had a run at this and couldn't get it to work with the Adw Preferences Page, but I'll have another go at it.

The nice Adw SwitchRow looks like it is only supported with libadwaita 1.4. We may need a fallback using a Gtk.Switch if isn't available or don't use it at all. It is really cool though 😄

@amolenaar
Copy link
Member

amolenaar commented Oct 10, 2023

The nice Adw SwitchRow looks like it is only supported with libadwaita 1.4. We may need a fallback using a Gtk.Switch if isn't available or don't use it at all. It is really cool though 😄

The reason we are on an older GTK/libadwaita version is mainly due to our build server (Ubuntu), which is stuck on an older version. We have #2320 waiting to be merged for the same reason. I suggest we make the build work with the latest GTK/libadwaita version

  • Maybe we can find a PPA with up-to-date versions of GTK?
  • We can also use a container image (Fedora?) an build there.
  • We can exclude tests based on the GTK/libadwaita version used. This may be a good idea anyway. They're tested on Windows and macOS.
  • We can even compile GTK/libadwaita from source if we have to. I think there's even more things we can do,
  • I think I forgot some options, still.

What do you think?

@danyeaw
Copy link
Member Author

danyeaw commented Oct 10, 2023

Hi @amolenaar, ya, I like your idea to either find a PPA with updated versions of GTK or use a container image. Let me start to investigate this!

@danyeaw
Copy link
Member Author

danyeaw commented Oct 25, 2023

Linux is working great now. Neither Windows or macOS have libadwaita 1.4 yet due to AppStream compatibility. I'll work on some yak shaving to see if I can't get that going.

@danyeaw danyeaw force-pushed the preferences branch 2 times, most recently from 8e33cd8 to 29db2ec Compare November 11, 2023 23:58
@danyeaw
Copy link
Member Author

danyeaw commented Nov 12, 2023

I noticed that we are getting a Cairo error in Windows from the last Gvsbuild update, let's hold off merging this until I investigate

@danyeaw danyeaw added feature A new feature and removed python Pull requests that update Python code packaging Update to packaging aspects labels Nov 19, 2023
Copy link
Member

@amolenaar amolenaar 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. I think there are a few minor improvements to be made.

It may be worth to see if the settings module code can be moved into the UI module - so that settings made in the (ui) application are not applied when using (for example) a jupyter notebook.

.github/workflows/full-build.yml Outdated Show resolved Hide resolved
gaphor/settings.py Show resolved Hide resolved
gaphor/settings.py Outdated Show resolved Hide resolved
gaphor/settings.py Show resolved Hide resolved
gaphor/settings.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added python Pull requests that update Python code packaging Update to packaging aspects labels Nov 23, 2023
@danyeaw
Copy link
Member Author

danyeaw commented Nov 24, 2023

It looks like gsettings schema doesn't default searching in the users .local directory on Windows. We could override it there using the GSETTINGS_SCHEMA_DIR environmental variable. We could also copy it to the Gvsbuild share directory which it would search by default.

Copy link
Member

@amolenaar amolenaar left a comment

Choose a reason for hiding this comment

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

LGTM

@danyeaw danyeaw removed python Pull requests that update Python code packaging Update to packaging aspects documentation labels Nov 26, 2023
@danyeaw danyeaw merged commit ac519c5 into main Nov 26, 2023
20 checks passed
@danyeaw danyeaw deleted the preferences branch November 26, 2023 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to set the UI language of Gaphor
2 participants