Feature/linux cli #356
Feature/linux cli #356
Conversation
Add command-line interface to the Linux app: - --help, --version: standard CLI options - --status, -s: show device status, battery levels - --json, -j: JSON output for scripting - --waybar, -w: Waybar custom module format - --set-noise-mode: control noise cancellation mode - --set-conversational-awareness: toggle conversational awareness - --set-adaptive-level: set adaptive noise level CLI commands communicate with running instance via IPC socket. Refactored CLI code into separate cli.cpp/cli.h for cleaner main.cpp.
- Fix segfault when calling .first() on empty rootObjects() or topLevelWindows() lists in tray menu handlers - Add Fusion as fallback Qt Quick Controls style to prevent QML load failures when platform theme modules (e.g., kvantum) are not installed - Override QT_STYLE_OVERRIDE=kvantum to Fusion before QApplication init The Fusion style is built into Qt and available on all platforms (X11, Wayland, KDE, GNOME, etc.), ensuring the app works regardless of the user's theme configuration.
WalkthroughAdds a Linux CLI layer and related IPC helpers, integrates CLI into startup to reuse or start instances, exposes three status-reporting methods (JSON, text, Waybar), and defines a compile-time LIBREPODS_VERSION macro for C++ code. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant App as main() / Startup
participant IPC as QLocalSocket / Server
participant GUI as Running GUI Instance
User->>App: launch with CLI args
App->>CLI: handleCLICommands(app)
CLI->>IPC: isInstanceRunning()
alt instance running
CLI->>IPC: sendIpcCommand(command, timeout)
IPC-->>CLI: response (status / ack / error)
CLI-->>User: print formatted output / exit
else no instance
CLI-->>App: return -1 (continue to GUI)
App->>App: cleanup stale socket, start IPC server
App->>GUI: initialize GUI and event loop
GUI-->>User: GUI started (no CLI response)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
linux/cli.cpp (1)
33-39: Centralize the local‑server name and align timeouts
isInstanceRunning()andsendIpcCommand()(and main.cpp) all hard‑code"app_server"and slightly different connect timeouts. Consider a shared constant (e.g.constexpr auto kIpcServerName = "app_server";) and, if possible, a single place defining the connection timeout, to avoid subtle drift if this ever changes.Also applies to: 41-61
linux/cli.h (1)
3-9: Avoidusing namespacein a public header
using namespace AirpodsTrayApp::Enums;in a header pulls all of that namespace’s symbols into any translation unit that includescli.h, which can lead to surprising name clashes.Prefer one of:
- Qualify the type in the declarations, e.g.
AirpodsTrayApp::Enums::NoiseControlMode.- Or add a narrow alias inside the
CLInamespace, e.g.using NoiseControlMode = AirpodsTrayApp::Enums::NoiseControlMode;, and use that in the signatures.This keeps the CLI API clean without leaking the entire enums namespace to callers.
Also applies to: 12-25
linux/main.cpp (1)
1128-1133: Optionally include<QFile>explicitly where it’s usedYou use
QFile stale("/tmp/app_server");in both the startup and shutdown cleanup paths. IfQFileis not already pulled in transitively via another header, it would be safer to add an explicit#include <QFile>near the top of this file to keep the dependency clear and avoid relying on indirect includes.Also applies to: 1270-1274
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
linux/CMakeLists.txt(2 hunks)linux/cli.cpp(1 hunks)linux/cli.h(1 hunks)linux/main.cpp(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
linux/cli.h (2)
linux/cli.cpp (10)
noiseControlModeName(14-22)noiseControlModeName(14-14)parseNoiseControlMode(24-31)parseNoiseControlMode(24-24)isInstanceRunning(33-39)isInstanceRunning(33-33)sendIpcCommand(41-61)sendIpcCommand(41-41)handleCLICommands(63-234)handleCLICommands(63-63)linux/main.cpp (7)
mode(291-301)mode(291-291)mode(302-310)mode(302-302)mode(1063-1063)name(1068-1068)app(1104-1104)
linux/cli.cpp (2)
linux/main.cpp (5)
mode(291-301)mode(291-291)mode(302-310)mode(302-302)mode(1063-1063)linux/deviceinfo.hpp (1)
static_cast(58-58)
🪛 Clang (14.0.6)
linux/cli.h
[error] 3-3: 'QString' file not found
(clang-diagnostic-error)
🔇 Additional comments (5)
linux/cli.cpp (1)
14-31: Noise-control mode helpers look consistent and cover common aliasesThe string/enum mapping (including short forms like
nc,tr, numeric 0–3) looks consistent and should work well with the IPC integer protocol. Just keep the mapping in sync with any future changes toNoiseControlModeordering.linux/CMakeLists.txt (1)
7-9: Version macro + CLI sources wiring looks correctPassing
${PROJECT_VERSION}viaLIBREPODS_VERSIONand addingcli.cpp/cli.hto thelibrepodstarget cleanly exposes the version to the CLI and ensures the new code is built with the main binary.Also applies to: 17-47
linux/main.cpp (3)
15-17: Status helpers for JSON/text/Waybar are well factored and null‑safeThe new
getStatusJson(),getStatusText(), andgetStatusWaybar()methods cleanly encapsulate status reporting for CLI/IPC, with appropriate checks for connection state and nullBattery/EarDetectionpointers. They should be safe to call regardless of connection and give consistent output across formats.Also applies to: 32-32, 141-180, 182-208, 210-242
548-556: Tray and menu handlers now guard against missing UI and avoid crashesThe added checks for
QGuiApplication::topLevelWindows().isEmpty()andparent->rootObjects().isEmpty()before accessing windows/root objects will prevent the crashes seen when tray actions are used before QML is loaded. Falling back toloadMainModule()in those cases is a sensible behavior.Also applies to: 564-575, 576-586
1099-1103: Theme fallback and CLI entry integration look reasonableSwitching from
QT_STYLE_OVERRIDE=kvantumtoFusionand forcingQT_QUICK_CONTROLS_STYLE=Fusionshould mitigate theme-plugin crashes on systems with partial Kvantum setups, and the earlyCLI::handleCLICommands(app)call cleanly short‑circuits into CLI mode when relevant options are present while still allowing--debug/--hideto be picked up for GUI runs.Also applies to: 1104-1110, 1134-1140
| parser.process(app); | ||
|
|
||
| bool wantsStatus = parser.isSet(statusOption); | ||
| bool wantsJson = parser.isSet(jsonOption); | ||
| bool wantsWaybar = parser.isSet(waybarOption); | ||
| QString noiseMode = parser.value(setNoiseModeOption); | ||
| QString caState = parser.value(setCAOption); | ||
| QString adaptiveLevel = parser.value(setAdaptiveLevelOption); | ||
|
|
||
| // Check if this is a CLI command | ||
| bool hasStatusQuery = wantsStatus || wantsWaybar; | ||
| bool hasControlCommand = !noiseMode.isEmpty() || !caState.isEmpty() || !adaptiveLevel.isEmpty(); | ||
| bool isCLICommand = hasStatusQuery || hasControlCommand; | ||
|
|
||
| if (!isCLICommand) { | ||
| // Not a CLI command, return -1 to indicate GUI should start | ||
| return -1; | ||
| } | ||
|
|
||
| // Handle CLI commands | ||
| QTextStream out(stdout); | ||
| QTextStream err(stderr); | ||
|
|
||
| if (!isInstanceRunning()) { | ||
| err << "Error: LibrePods is not running. Start the application first.\n"; | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
Waybar disconnected fallback is unreachable when the app isn’t running
Right now handleCLICommands() returns early if !isInstanceRunning() (Lines 135–138), so the --waybar branch (Lines 141–153) never executes in that case and the “disconnected” JSON fallback is not emitted. For Waybar, it’s usually preferable to always print valid JSON even if the app isn’t running.
Consider restructuring to handle wantsWaybar before the instance check, e.g.:
- If
wantsWaybar, callsendIpcCommand("cli:status:waybar")directly and, on empty response, print the disconnected JSON and exit 0, regardless ofisInstanceRunning(). - For other CLI commands, keep the existing
isInstanceRunning()guard and error exit.
This keeps Waybar output robust while still surfacing a clear error for the other CLI commands.
Also applies to: 140-168, 170-231
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
linux/main.cpp (1)
1172-1258: IPC CLI handler is much stronger;cli:set-adaptive-levelstill reports success when no change is possibleNice improvements here:
- Attaching
deleteLateronQLocalSocket::disconnectedfixes the previous per‑request socket leak.- CLI handlers now validate numeric inputs and check that AirPods are connected before attempting writes, which is a solid step up.
One remaining behavioral edge case (echoing the earlier review about IPC set‑* responses):
cli:set-adaptive-levelreturns"OK"whenever the level parses andareAirpodsConnected()is true, butAirPodsTrayApp::setAdaptiveNoiseLevelsilently no‑ops whenm_deviceInfo->adaptiveModeActive()is false or when the current level already matches. From a CLI/IPC client’s perspective, this makes it impossible to distinguish “applied” from “ignored”.Consider having
setAdaptiveNoiseLevelreturn aboolindicating whether a change was actually applied (and/or whetheradaptiveModeActive()is true) and using that in the CLI handler to reply with"OK"vs"Error: adaptive mode inactive / no change applied". This would fully address the earlier concern about accurate IPC feedback for set‑operations.
🧹 Nitpick comments (3)
linux/main.cpp (3)
141-242: Status helpers are well‑structured; consider minor reuse/encoding tweaksThe three helpers (
getStatusJson,getStatusText,getStatusWaybar) pull consistent data fromDeviceInfo/Battery/EarDetectionand handle nulls safely, which is good. Two optional improvements:
- Explicitly wrap JSON output as
QString::fromUtf8(doc.toJson(QJsonDocument::Compact))to make UTF‑8 encoding explicit.- Extract the common battery/tooltip construction into a small helper to avoid duplication between JSON and Waybar outputs if fields evolve later.
550-555: Tray/menu reopen guards correctly prevent crashes when QML isn’t loadedThe new checks for
topLevelWindows().isEmpty()andparent->rootObjects().isEmpty()before accessing windows/root objects should prevent the tray “Open” / “Settings” actions from crashing when the engine hasn’t loaded yet. As a tiny defensive improvement, you could add aQ_ASSERT(parent)or an early‑returnif (!parent) return;in these slots to make the non‑null assumption explicit.Also applies to: 566-569, 578-581
1099-1140: Fusion fallback and early CLI handling look good; one redundant removeServer callUsing Fusion when
QT_STYLE_OVERRIDE=kvantumplus forcingQT_QUICK_CONTROLS_STYLE=Fusionshould address missing‑theme crashes on misconfigured systems, and handling CLI commands before GUI startup is a clean separation. You now callQLocalServer::removeServer("app_server")both beforeisInstanceRunning/reopen (Lines 1128–1132) and again just beforeserver.listen(Line 1160); keeping only one of these calls would simplify the flow without changing behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
linux/main.cpp(9 hunks)
🔇 Additional comments (1)
linux/main.cpp (1)
15-17: Includes for JSON, text streaming, and CLI are appropriateAdding
QJsonDocument,QJsonObject,QTextStream, andcli.hcleanly supports the new status/CLI features without unnecessary dependencies. No issues here.Also applies to: 32-32
- Add deleteLater cleanup for IPC sockets to prevent memory leaks - Validate noise mode input (0-3 range check) - Validate conversational awareness state (must be "0" or "1") - Validate adaptive level range (0-100)
meerzulee
force-pushed
the
feature/linux-cli
branch
from
b3e6524 to
bc25f08
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
linux/main.cpp (3)
576-586: Early return afterloadMainModule()may not open the Settings page.When
rootObjects().isEmpty()is true and the user clicks "Settings" in the tray menu,loadMainModule()is called but the method returns immediately without invoking the"reopen"method with"settings"argument. This means the app will show its default view rather than navigating to settings.Consider deferring the
reopencall after the module loads:void onOpenSettings() { if (parent->rootObjects().isEmpty()) { loadMainModule(); - return; + // Wait for QML to load, then navigate to settings + QTimer::singleShot(100, this, [this]() { + if (!parent->rootObjects().isEmpty()) { + QObject *rootObject = parent->rootObjects().first(); + if (rootObject) { + QMetaObject::invokeMethod(rootObject, "reopen", Q_ARG(QVariant, "settings")); + } + } + }); + return; } QObject *rootObject = parent->rootObjects().first();The same pattern applies to
onOpenApp()at lines 564-574 if the default view isn't the "app" view.
1157-1160: RedundantQLocalServer::removeServer("app_server")call.
removeServer("app_server")is called at line 1129 during stale socket cleanup, and again at line 1160 beforeserver.listen(). The second call is redundant since any stale socket was already removed.engine.addImageProvider("qrcode", new QRCodeImageProvider()); trayApp->loadMainModule(); QLocalServer server; - QLocalServer::removeServer("app_server"); if (!server.listen("app_server"))
1193-1204: Hardcoded noise mode range may become stale if enum changes.The validation
mode > 3hardcodes the maximum valid mode value. IfNoiseControlModeenum is extended, this check could silently reject valid modes or accept invalid ones.Consider using the enum's max value for consistency with
setNoiseControlModeInt():else if (msg.startsWith("cli:set-noise-mode:")) { QString modeStr = msg.mid(QString("cli:set-noise-mode:").length()); bool ok; int mode = modeStr.toInt(&ok); - if (!ok || mode < 0 || mode > 3) { + if (!ok || mode < 0 || mode > static_cast<int>(NoiseControlMode::Adaptive)) { response = "Error: Invalid noise mode";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
linux/main.cpp(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
linux/main.cpp (1)
linux/cli.cpp (7)
noiseControlModeName(14-22)noiseControlModeName(14-14)out(132-132)handleCLICommands(63-234)handleCLICommands(63-63)isInstanceRunning(33-39)isInstanceRunning(33-33)
🔇 Additional comments (7)
linux/main.cpp (7)
15-17: LGTM!The new includes are appropriate for JSON serialization (
QJsonDocument,QJsonObject), text stream handling (QTextStream), and CLI integration (cli.h).Also applies to: 32-32
141-180: LGTM!The
getStatusJson()implementation properly guards against null pointers form_deviceInfo,Battery, andEarDetectionobjects. The JSON structure is well-organized and provides comprehensive status information.
182-208: LGTM!The
getStatusText()method provides a clean human-readable format with proper null checks and early return for disconnected state.
210-242: LGTM!The
getStatusWaybar()method provides well-structured Waybar output with appropriate fallbacks for null battery objects and includes useful tooltip information with charging indicators.
1099-1102: Fusion style applied unconditionally to Quick Controls.Line 1102 sets
QT_QUICK_CONTROLS_STYLEto"Fusion"regardless of whether the kvantum condition is met at line 1100. This means all users get Fusion style for Quick Controls, even if they have a complete theme setup.Is this intentional? If the goal is only to fix kvantum-specific issues, you may want to wrap line 1102 inside the condition:
if (qgetenv("QT_STYLE_OVERRIDE").toLower() == "kvantum") + { qputenv("QT_STYLE_OVERRIDE", "Fusion"); - qputenv("QT_QUICK_CONTROLS_STYLE", "Fusion"); + qputenv("QT_QUICK_CONTROLS_STYLE", "Fusion"); + }Or if Fusion for Quick Controls is intentional for broader compatibility, the current code is fine.
1134-1139: LGTM!The
--debugand--hideflags are properly parsed and propagated. SettingquitOnLastWindowClosed(false)ensures the app continues running in the system tray when windows are closed.
1172-1174: Socket lifetime properly managed.The
deleteLaterconnection ondisconnectedsignal ensures sockets are cleaned up after clients disconnect, addressing the memory leak concern from previous reviews.
1 participant
Linux: Add CLI support and fix Qt theme compatibility
Features
CLI Interface
Query status and control AirPods from the command line while the app is running in the background.
Waybar integration example:
Bug Fixes
Qt Theme Compatibility
The app now works on X11, Wayland, KDE, GNOME, and other desktop environments regardless of theme configuration.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
✏️ Tip: You can customize this high-level summary in your review settings.