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

Added a dark mode property on App. #2992

Merged
merged 4 commits into from
Nov 27, 2024
Merged

Conversation

rtpg
Copy link
Contributor

@rtpg rtpg commented Nov 25, 2024

Fixes #2841

This only adds support for Cocoa as that was the only platform I could get working today, but there are now entrypooints for others to add support directly.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@rtpg rtpg force-pushed the dark-mode branch 5 times, most recently from fefbee8 to 793bcd6 Compare November 25, 2024 05:24
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

The implementation looks great, and the testbed test is great (just needs to be moved to a different location).

The only piece that looks to be missing has been picked up by CI - there's a test coverage failure because there's no core test for this API. That means implementing the "dark mode" API for the dummy backend, and adding a unit test to the core test suite to exercise the core part of the API.

Comment on lines 264 to 265
def get_dark_mode_state(self):
raise NotImplementedError()
Copy link
Member

Choose a reason for hiding this comment

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

It would be more consistent to use self.factory.not_implemented("dark mode state"), and return None directly (and similar for the other not-implemented backends)

@@ -0,0 +1 @@
``toga.App`` now has a ``dark_mode`` read-only property, to read the user's OS-level dark mode preferences. Support is currently limited to Cocoa, anod other backends will use ``None`` to indicate that the dark mode preference cannot be detected.
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to fully document the feature in the changenote; we're just advertising that it exists. The API docs become the "full" documentation.

Suggested change
``toga.App`` now has a ``dark_mode`` read-only property, to read the user's OS-level dark mode preferences. Support is currently limited to Cocoa, anod other backends will use ``None`` to indicate that the dark mode preference cannot be detected.
Apps can now interrogate whether they are in dark mode on some platforms.

Comment on lines 399 to 402
try:
return self._impl.get_dark_mode_state()
except NotImplementedError:
return None
Copy link
Member

Choose a reason for hiding this comment

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

If the underlying dark mode implementations use factory.not_implemented(), this can be simplified to a direct self._impl.get_dark_mode_state()

def dark_mode(self) -> bool | None:
"""Whether the user has dark mode enabled in their environment (read-only).

Returns None if we cannot determine dark mode settings."""
Copy link
Member

Choose a reason for hiding this comment

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

We can use specific markup for the return value.

Suggested change
Returns None if we cannot determine dark mode settings."""
:returns: A boolean describing if the app is in dark mode; ``None`` if Toga
cannot determine if the app is in dark mode.
"""

from types import NoneType


async def test_dark_mode_state_read(app):
Copy link
Member

Choose a reason for hiding this comment

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

This should be part of test_app.py, not a standalone test file for a single property.

######################################################################

def get_dark_mode_state(self):
return None
Copy link
Member

Choose a reason for hiding this comment

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

Textual has the ability to differentiate between light and dark themes, so this should probably be handled the same as other platforms.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

One small markup tweak in a comment, but otherwise, this looks great - Thanks for the fix (and the skeleton of a fix for other platforms)!

core/src/toga/app.py Outdated Show resolved Hide resolved
core/src/toga/app.py Outdated Show resolved Hide resolved
core/src/toga/app.py Outdated Show resolved Hide resolved
@freakboy3742 freakboy3742 merged commit f40adfa into beeware:main Nov 27, 2024
40 checks passed
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.

Add property to evaluate if app is in dark mode
2 participants