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

Color scheme change notifications #10

Closed
wants to merge 10 commits into from

Conversation

edfloreshz
Copy link
Collaborator

@edfloreshz edfloreshz commented Jan 17, 2022

Implements notify function, which takes a sender and sends the current mode when it changes.

  • Linux
    • Remove loop and replace for distro-specific code.
  • Windows
    • Implement notify
  • macOS
    • Implement notify

Resolves #3

@edfloreshz edfloreshz added the enhancement New feature or request label Jan 17, 2022
@edfloreshz edfloreshz self-assigned this Jan 17, 2022
@edfloreshz
Copy link
Collaborator Author

edfloreshz commented Jan 17, 2022

I would appreciate some help testing this @Be-ing 😸, works on my machine.

I used this tool for testing.

src/lib.rs Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/freedesktop/detect.rs Outdated Show resolved Hide resolved
src/freedesktop/detect.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@edfloreshz edfloreshz mentioned this pull request Jan 17, 2022
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@@ -0,0 +1,8 @@
#[tokio::main]
async fn main() -> anyhow::Result<()> {
dark_light::notify(&change_color_scheme).await
Copy link
Collaborator

Choose a reason for hiding this comment

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

This API of an async function running a synchronous callback in an infinite loop feels strange. I haven't seen a future that is expected to never resolve and keep running an infinite loop. I would expect a future from a library to actually return a value. Would it be better for the library to have an async function that returns a Mode value? That way, it would be the application's responsibility to await the future in a loop. I'm not sure about this. @frewsxcv what do you think?

Copy link
Collaborator

@Be-ing Be-ing Jan 18, 2022

Choose a reason for hiding this comment

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

Thinking about this again, why not both? The library could provide an async function which returns a Mode and the implementation of this notify function could be refactored to use it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated the code to use tokio channels instead of a callback.

@edfloreshz
Copy link
Collaborator Author

@bilelmoussaoui I'm getting an error when calling color_scheme and read on color-scheme key.

Portal request failed: org.freedesktop.zbus.Error: no description

Using ashpd 0.2.0-beta-1

The key is present in my system and works if I call it on version 0.1.0.

@bilelmoussaoui
Copy link

I will investigate, please open an issue against ashpd

@edfloreshz
Copy link
Collaborator Author

I've been thinking it might be best to ask for a Sender instead of a Fn to support async workflows.

What do you think @frewsxcv?

@frewsxcv
Copy link
Owner

I've been thinking it might be best to ask for a Sender instead of a Fn to support async workflows.

What do you think @frewsxcv?

Can you explain more what that would look like? Would Sender be a trait?

@edfloreshz
Copy link
Collaborator Author

Yeah, we ask for a Sender<Mode> and every time the library detects a change in the color scheme it does something like tx.send(mode) and then the user handles it with its respective Receiver<Mode>.

@frewsxcv
Copy link
Owner

@edfloreshz Yep! That sounds good to me

@edfloreshz edfloreshz changed the title Callback functions Color scheme change notifications Dec 26, 2022
Implemented callback functions for freedesktop and non-freedesktop.
Partitioned freedesktop into a module.

New module structure

Implemented new module structure to macOS and Windows

Fix for doc test

Ensured that all possible cases are being handled within `Mode` match.
`notify` is now an async function.
Modified example to use `tokio`
Renamed `Mode::rgb` to `Mode::from_rgb`
`freedesktop_watch` now uses `ashpd` beta.
Changed signature for `notify` function in macOS and Windows files.
- Removed Settings struct as it was not necessary.
- Removed `Settings` parameter from `freedesktop_watch`.
- `get_freedesktop_color_scheme` matches `ColorScheme` instead of `u32`
@edfloreshz
Copy link
Collaborator Author

I'm currently facing an issue with ashpd, I'm unable to fetch the color-scheme key value.

I created an issue to investigate before continuing with this PR

When the settings proxy is unable to init, legacy is used as fallback.
@edfloreshz edfloreshz marked this pull request as ready for review December 30, 2022 15:12
@edfloreshz
Copy link
Collaborator Author

I marked this PR as ready for review, don't merge until the issue with ashpd is resolved, this code can still function using the fallback.

- Upgraded to the master branch of ashpd
- Moved the thread spawning to `notify`
- Made the async methods return `anyhow::Result<T>`
Comment on lines +21 to +28
while let Ok(color_scheme) = proxy.receive_color_scheme_changed().await {
let mode = match color_scheme {
ColorScheme::NoPreference => Mode::Default,
ColorScheme::PreferDark => Mode::Dark,
ColorScheme::PreferLight => Mode::Light,
};
tx.send(mode).await?;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bilelmoussaoui I'm unsure if this is the correct way of using the receive_color_scheme_changed method.

@edfloreshz edfloreshz closed this Dec 30, 2022
@edfloreshz edfloreshz deleted the callback branch December 30, 2022 20:02
@edfloreshz
Copy link
Collaborator Author

PR moved to #26

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add new function that takes a callback which gets run whenever dark/light mode changes
6 participants