Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 Walkthrough<hidden_range_assignment><range_id>range_3106daf57ab6</range_id><cohort_id>cohort_main</cohort_id><layer_id>layer_apps</layer_id></hidden_range_assignment> Mergeability Score: ⚪ Minimal · up to The PR adds a system top bar and settings app without any supplied evidence of an actionable merge-blocking risk; it is merge-ready after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 174 |
| Duplication | 6 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
applications/launcher/controller.cpp (1)
41-58: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBlocking startup wait can stall the launcher for up to 30 seconds.
This polls D-Bus synchronously with
nanosleepin the constructor, before the event loop starts. Iftarnishis slow to register, the launcher blocks for up to 30s with no feedback beforeqFatal. ConsiderQDBusServiceWatcherfor an event-driven wait instead of busy-polling.♻️ Sketch of an event-driven alternative
- qDebug() << "Waiting for tarnish to start up"; - int retries = 30; - while (!bus.interface()->registeredServiceNames().value().contains( - OXIDE_SERVICE - )) { - if (!retries--) { - qFatal("Timed out waiting for %s to register on D-Bus", OXIDE_SERVICE); - } - struct timespec args{ - .tv_sec = 1, - .tv_nsec = 0, - }, - res; - nanosleep(&args, &res); - } + if (!bus.interface()->registeredServiceNames().value().contains(OXIDE_SERVICE)) { + QDBusServiceWatcher watcher(OXIDE_SERVICE, bus, QDBusServiceWatcher::WatchForRegistration); + QEventLoop loop; + QTimer::singleShot(30000, &loop, &QEventLoop::quit); + QObject::connect(&watcher, &QDBusServiceWatcher::serviceRegistered, &loop, &QEventLoop::quit); + loop.exec(); + if (!bus.interface()->registeredServiceNames().value().contains(OXIDE_SERVICE)) { + qFatal("Timed out waiting for %s to register on D-Bus", OXIDE_SERVICE); + } + }🤖 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 `@applications/launcher/controller.cpp` around lines 41 - 58, Replace the synchronous polling loop in the launcher constructor with an event-driven QDBusServiceWatcher for OXIDE_SERVICE. Start the watcher when the service is not yet registered, continue startup when its registration signal arrives, and retain a clear timeout/fatal path without blocking the event loop or using nanosleep.
🧹 Nitpick comments (1)
applications/launcher/controller.cpp (1)
276-401: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated root-guard +
findChild("batteryLevel")lookup across 6 handlers.Extracting a small
QObject* Controller::batteryUi()helper (returnroot ? root->findChild<QObject*>("batteryLevel") : nullptr) would remove the repeated pattern inbatteryAlert,batteryLevelChanged,batteryStateChanged,batteryTemperatureChanged,batteryWarning, andchargerStateChanged.♻️ Example helper extraction
+QObject* Controller::batteryUi() { + return root != nullptr ? root->findChild<QObject*>("batteryLevel") : nullptr; +} + void Controller::batteryAlert() { - if (root == nullptr) { - return; - } - QObject* ui = root->findChild<QObject*>("batteryLevel"); + QObject* ui = batteryUi(); if (ui) { ui->setProperty("alert", true); } }🤖 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 `@applications/launcher/controller.cpp` around lines 276 - 401, Extract a Controller::batteryUi() helper that returns the batteryLevel QObject when root is available, otherwise nullptr. Replace the repeated root null checks and findChild lookups in batteryAlert, batteryLevelChanged, batteryStateChanged, batteryTemperatureChanged, batteryWarning, and chargerStateChanged with this helper while preserving each handler’s existing property updates and early-return 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 `@applications/launcher/appitem.cpp`:
- Around line 99-103: The icon URL construction in AppItem::update and
AppItem::onIconChanged must encode local paths correctly. Replace the "file:" +
path construction in both sites with
QUrl::fromLocalFile(path).toString(QUrl::FullyEncoded), preserving the existing
fallback icon behavior; affected sites are applications/launcher/appitem.cpp
lines 99-103 and applications/launcher/appitem.h lines 66-70.
---
Outside diff comments:
In `@applications/launcher/controller.cpp`:
- Around line 41-58: Replace the synchronous polling loop in the launcher
constructor with an event-driven QDBusServiceWatcher for OXIDE_SERVICE. Start
the watcher when the service is not yet registered, continue startup when its
registration signal arrives, and retain a clear timeout/fatal path without
blocking the event loop or using nanosleep.
---
Nitpick comments:
In `@applications/launcher/controller.cpp`:
- Around line 276-401: Extract a Controller::batteryUi() helper that returns the
batteryLevel QObject when root is available, otherwise nullptr. Replace the
repeated root null checks and findChild lookups in batteryAlert,
batteryLevelChanged, batteryStateChanged, batteryTemperatureChanged,
batteryWarning, and chargerStateChanged with this helper while preserving each
handler’s existing property updates and early-return 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 05eb204a-31b3-41a0-930e-dbfafdd5637c
📒 Files selected for processing (6)
applications/launcher/appitem.cppapplications/launcher/appitem.happlications/launcher/applistmodel.happlications/launcher/controller.cppapplications/launcher/controller.happlications/launcher/main.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- applications/launcher/controller.h
| setProperty( | ||
| "imgFile", | ||
| (!imgFile.isEmpty() && QFile(imgFile).exists()) ? "file:" + imgFile | ||
| : "qrc:/img/icon.png" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate files:"
git ls-files | rg '(^|/)appitem\.(cpp|h)$|^.*\.qml$|^.*\.qrc$' | sed -n '1,120p'
echo
echo "Inspect appitem.cpp appitem.h around referenced lines:"
for f in applications/launcher/appitem.cpp applications/launcher/appitem.h; do
if [ -f "$f" ]; then
echo "--- $fwc -l $f"
wc -l "$f"
echo "--- $f first 160 lines"
sed -n '1,160p' "$f" | nl -ba | sed -n '80,110p;60,75p'
fi
done
echo
echo "Search for icon/imgFile usage:"
rg -n "imgFile|onIconChanged|QUrl|toLocalFile|fromLocalFile|qrc:/img/icon|icon.png" applications/launcher -SRepository: Eeems-Org/oxide
Length of output: 2384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in applications/launcher/appitem.cpp applications/launcher/appitem.h; do
echo "--- lines $f"
wc -l "$f"
echo "--- appitem.cpp lines 1-140"
sed -n '1,140p' "$f" | nl -ba
echo "--- appitem.h lines 1-120"
sed -n '1,120p' "$f" | nl -ba
done
echo "--- imgFile/icon usages in launcher files"
rg -n "imgFile|onIconChanged|QUrl|toLocalFile|fromLocalFile|qrc:/img/icon|icon.png|Image" applications/launcher -S
echo "--- relevant widgets.qml image section"
sed -n '1,180p' applications/launcher/widgets/AppItem.qml | nl -ba | sed -n '1,140p'
echo "--- URL-significant character decoding behavior relevant to concatenated file paths"
python3 - <<'PY'
import urllib.parse, pathlib
samples = [
"/home/user/Apps/My App/app.png",
"/home/user/Apps/app%2Fname.png",
"/home/user/Apps/app#name.png",
"/home/user/Apps/app space&icon.png",
]
for p in samples:
concatenated = "file:" + p
# What a strict URL parser would decode from the path component before interpreting q#fragments
# URL path decoding is necessary to see whether spaces/#/% affect parsing.
u = connected = f"file:{p}"
pathcomp = urllib.parse.urlparse(u).path
decoded = urllib.parse.unquote(pathcomp)
print({"input": p, "concatenated": concatenated, "parsed_path": pathcomp, "decoded": decoded, "matches_input": (decoded == p)})
PYRepository: Eeems-Org/oxide
Length of output: 303
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in applications/launcher/appitem.cpp applications/launcher/appitem.h; do
echo "--- file: $f"
wc -l "$f"
echo "--- appitem.cpp lines 1-140"
awk '{printf "%5d\t%s\n", NR, $0}' "$f" | sed -n '1,140p'
done
echo "--- imgFile/icon usages in launcher files"
rg -n "imgFile|onIconChanged|QUrl|fromLocalFile|qrc:/img/icon|icon.png|Image" applications/launcher -S || true
echo "--- launcher AppItem.qml relevant sections"
awk '{printf "%5d\t%s\n", NR, $0}' applications/launcher/widgets/AppItem.qml | sed -n '1,160p'Repository: Eeems-Org/oxide
Length of output: 13093
Encode both icon file paths before building the QUrl.
"file:" + path is not a valid QUrl::fromLocalFile replacement; it leaves spaces unencoded and treats characters like # as fragment separators. Use QUrl::fromLocalFile(path).toString(QUrl::FullyEncoded) in both AppItem::update and AppItem::onIconChanged.
📍 Affects 2 files
applications/launcher/appitem.cpp#L99-L103(this comment)applications/launcher/appitem.h#L66-L70
🤖 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 `@applications/launcher/appitem.cpp` around lines 99 - 103, The icon URL
construction in AppItem::update and AppItem::onIconChanged must encode local
paths correctly. Replace the "file:" + path construction in both sites with
QUrl::fromLocalFile(path).toString(QUrl::FullyEncoded), preserving the existing
fallback icon behavior; affected sites are applications/launcher/appitem.cpp
lines 99-103 and applications/launcher/appitem.h lines 66-70.
469022d to
c78ce45
Compare
1 participant
Summary by CodeRabbit