add dynamic wallpaper timer#349
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds a GeoClue2-based latitude helper, a monthly wallpaper selection script that adjusts for hemisphere, a user-level systemd oneshot service and daily timer to run the script, and a user setup hook that enables the timer. ChangesDynamic Wallpaper System
Sequence DiagramsequenceDiagram
participant Timer as systemd Timer
participant Service as bluefin-dynamic-wallpaper Service
participant Wallpaper as bluefin-dynamic-wallpaper Script
participant Geoclue as get-geoclue-latitude Script
participant gsettings
participant GeoClue2 as GeoClue2 D-Bus
Timer->>Service: Trigger at boot + daily
Service->>Wallpaper: Execute script
Wallpaper->>gsettings: Read picture-uri, picture-uri-dark
gsettings-->>Wallpaper: Current URIs
Wallpaper->>Geoclue: Request latitude
Geoclue->>GeoClue2: CreateClient, Set props, Start, Poll
GeoClue2-->>Geoclue: Location, Latitude/Longitude
Geoclue-->>Wallpaper: Latitude (or error/default)
Wallpaper->>Wallpaper: Compute month/hemisphere, select file
Wallpaper->>gsettings: Set picture-uri, picture-uri-dark
gsettings-->>Wallpaper: Success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
joshyorko
changed the title
dosubot
Bot
added
size:L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@system_files/shared/usr/libexec/get-geoclue-latitude`:
- Around line 100-107: The direct gdbus call that populates LAT_OUTPUT should
preserve the helper’s AccessDenied exit-code contract by either using the
existing run_gdbus wrapper or by detecting the DBus AccessDenied error text and
exiting with code 2; replace the inline gdbus invocation that sets LAT_OUTPUT
(and the analogous longitude block that sets LONG_OUTPUT) with a call to
run_gdbus --system --dest org.freedesktop.GeoClue2 --object-path
"$LOCATION_PATH" --method org.freedesktop.DBus.Properties.Get
org.freedesktop.GeoClue2.Location Latitude (and Longitude) so run_gdbus can map
org.freedesktop.DBus.Error.AccessDenied to exit code 2, or if you must keep the
inline call, check LAT_OUTPUT for "org.freedesktop.DBus.Error.AccessDenied" and
exit 2 instead of 1.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a8ea5fcc-c2b3-4fb0-ae7e-d2b0b9e781cb
📒 Files selected for processing (5)
system_files/shared/usr/lib/systemd/user/bluefin-dynamic-wallpaper.servicesystem_files/shared/usr/lib/systemd/user/bluefin-dynamic-wallpaper.timersystem_files/shared/usr/libexec/bluefin-dynamic-wallpapersystem_files/shared/usr/libexec/get-geoclue-latitudesystem_files/shared/usr/share/ublue-os/user-setup.hooks.d/20-dynamic-wallpaper.sh
hanthor
left a comment
hanthor
left a comment
There was a problem hiding this comment.
Well-structured port of the dynamic wallpaper work from ublue-os/bluefin — systemd timer + service pair, GeoClue latitude helper with clean fallback, and user-setup hook to enable it for new users. CI passes. Approved.
There was a problem hiding this comment.
No way Aurora wants this. So its in the wrong shared folder folder
See https://github.com/projectbluefin/common#directory-structure
I guess we should codify this in codeowners and as a PR template |
joshyorko
force-pushed
the
codex/dynamic-wallpaper-common
branch
from
3b520e8 to
8ef7787
Compare
understood, done |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
system_files/bluefin/usr/share/ublue-os/user-setup.hooks.d/20-dynamic-wallpaper.sh (1)
9-9: 💤 Low value
set -xlooks like a leftover debug artifact.
version-scriptalready disables tracing (set +x) on success, and re-enabling it here traces thesystemctlinvocation into the user session journal on every setup run. If this isn't intentional debugging, drop it.🧹 Proposed cleanup
-set -x - echo "Enabling dynamic wallpaper timer" systemctl --user enable --now bluefin-dynamic-wallpaper.timer🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@system_files/bluefin/usr/share/ublue-os/user-setup.hooks.d/20-dynamic-wallpaper.sh` at line 9, Remove the leftover tracing invocation "set -x" so the hook does not re-enable shell tracing and thereby avoid leaking the subsequent systemctl invocation into the user session journal; locate the "set -x" token in the 20-dynamic-wallpaper.sh hook and delete it (or replace with a no-op/comment) so tracing remains disabled by the existing version-script "set +x" behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@system_files/bluefin/usr/libexec/get-geoclue-latitude`:
- Around line 60-77: The Set and Start gdbus calls (invoked via run_gdbus for
org.freedesktop.GeoClue2.Client DesktopId, RequestedAccuracyLevel and
Client.Start against "$CLIENT_PATH") are printing their results to stdout and
leaking "()" into the script output; silence those calls by discarding their
stdout/stderr (e.g. redirect their output to /dev/null) so only the
latitude/longitude read commands produce stdout for LATITUDE; keep the calls and
arguments (run_gdbus, DesktopId, RequestedAccuracyLevel, Client.Start,
"$CLIENT_PATH") unchanged except for adding the redirection.
- Around line 108-120: The sed regex used to extract LATITUDE and LONGITUDE from
LAT_OUTPUT/LON_OUTPUT is too strict (expects "<double 52.52>") and fails when
gdbus prints "<52.52>"; update the extraction used for LATITUDE and LONGITUDE
(the sed invocation lines) to accept either form (make the "double " prefix
optional) so both "<52.52>" and "<double 52.52>" parse correctly; apply the same
tolerant pattern change to both the LATITUDE and LONGITUDE assignments that
follow run_gdbus.
---
Nitpick comments:
In
`@system_files/bluefin/usr/share/ublue-os/user-setup.hooks.d/20-dynamic-wallpaper.sh`:
- Line 9: Remove the leftover tracing invocation "set -x" so the hook does not
re-enable shell tracing and thereby avoid leaking the subsequent systemctl
invocation into the user session journal; locate the "set -x" token in the
20-dynamic-wallpaper.sh hook and delete it (or replace with a no-op/comment) so
tracing remains disabled by the existing version-script "set +x" behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 035d39a6-0f98-4e43-a2a0-b7cf3b56b26b
📒 Files selected for processing (5)
system_files/bluefin/usr/lib/systemd/user/bluefin-dynamic-wallpaper.servicesystem_files/bluefin/usr/lib/systemd/user/bluefin-dynamic-wallpaper.timersystem_files/bluefin/usr/libexec/bluefin-dynamic-wallpapersystem_files/bluefin/usr/libexec/get-geoclue-latitudesystem_files/bluefin/usr/share/ublue-os/user-setup.hooks.d/20-dynamic-wallpaper.sh
hanthor
left a comment
hanthor
left a comment
There was a problem hiding this comment.
Thanks for the contribution, @joshyorko! Building on @inffy's feedback — the files themselves are correctly placed under system_files/bluefin/, which is good. However, the concern is about the setup hook: system_files/bluefin/usr/share/ublue-os/user-setup.hooks.d/20-dynamic-wallpaper.sh.
Per the repository directory structure, content in system_files/shared/ is consumed by all downstream images including Aurora. Aurora would not want a Bluefin-specific dynamic wallpaper timer enabled for their users.
The systemd service/timer units and scripts are correctly scoped to system_files/bluefin/. Please make sure the user-setup hook is also exclusively under system_files/bluefin/ and is not referenced from or copied into the shared/ layer. If the hook currently lives in a shared hooks directory (e.g. system_files/shared/usr/share/ublue-os/user-setup.hooks.d/), it should be moved to the bluefin/-specific path so that Aurora users are not affected.
Once that is confirmed/corrected, the content looks like a great addition for Bluefin specifically!
hanthor
left a comment
hanthor
left a comment
There was a problem hiding this comment.
Thanks for the contribution, @joshyorko! Building on @inffy's feedback: the systemd service, timer units, and scripts are correctly placed under system_files/bluefin/ - that part is right. The key concern is that the user-setup hook (20-dynamic-wallpaper.sh) must also live exclusively under system_files/bluefin/ and not be included in the system_files/shared/ layer. Per the repo directory structure, content in shared/ is consumed by all downstream images including Aurora, and Aurora would not want a Bluefin-specific dynamic wallpaper timer enabled for their users. As long as all 5 files remain under system_files/bluefin/ (which the current diff shows they do), the directory placement is correct. Please double-check that nothing in the Containerfile or build process copies this hook into the shared layer. Once confirmed, the feature looks like a solid addition for Bluefin!
🤖 Copilot Test ReportBranch: Test Results
Files Changed
Service Review
What testing this PR needs beyond static analysisThis PR ships systemd units and a shell script — functional testing requires a running system. Ghost/titan lab test is recommended: build with |
joshyorko
force-pushed
the
codex/dynamic-wallpaper-common
branch
from
8ef7787 to
a81ff9f
Compare
hanthor
left a comment
hanthor
left a comment
There was a problem hiding this comment.
LGTM! Excellent port of the shared-layer dynamic wallpaper work from bluefin to common. Verified all shellchecks and validation checks are successfully passing. Ready to merge.
5 participants
Summary
Why
This ports the applicable shared-layer work from ublue-os/bluefin#3890 into
projectbluefin/common, where shared Bluefin workload behavior belongs. That keeps the feature in one layer for downstream Bluefin variants instead of landing it only inublue-os/bluefin.Scope note
The approved Bluefin PR also removed the old build-time month rewrite from
ublue-os/bluefinatbuild_files/base/05-override-install.sh. That file does not exist inprojectbluefin/common, so this PR cannot remove that line directly. If that rewrite still runs in Bluefin after this common change lands, it should be removed in a small follow-up PR againstublue-os/bluefin.Testing
shellcheck system_files/shared/usr/libexec/bluefin-dynamic-wallpaper system_files/shared/usr/libexec/get-geoclue-latitude system_files/shared/usr/share/ublue-os/user-setup.hooks.d/20-dynamic-wallpaper.shbash -n system_files/shared/usr/libexec/bluefin-dynamic-wallpaper system_files/shared/usr/libexec/get-geoclue-latitude system_files/shared/usr/share/ublue-os/user-setup.hooks.d/20-dynamic-wallpaper.shgit diff --check HEAD~1..HEADsystemd-analyze verifywith a temporary host-validExecStartrelocationgsettingstest confirmed southern hemisphere maps May to November and updates both light/dark wallpaper URIsgsettingstest confirmed custom dark wallpaper is not overwrittengdbustests confirmed valid latitude, AccessDenied exit 2, and(0.0, 0.0)rejectionjust checkwas also run. It fails on existing formatting drift in untouched Justfiles undersystem_files/bluefin/usr/share/ublue-os/just/; this PR does not touchJustfileor*.just, so that path-filtered workflow should not run for this change.Refs ublue-os/bluefin#3890
Summary by CodeRabbit
New Features
Chores