v1.21.0: SMS push delivery, runcommand output streaming, timer-free MPRIS idle - #47
Merged
Merged
Conversation
The watcher dispatch was an inline switch on sig.Name inside the read loop, so no test could reach it: the switch needed a live *dbus.Conn and a real signal channel. Comparing bare member names against it therefore went unnoticed while silently dropping every MPRIS signal. Extract a pure classifySignal so the routing contract is testable, and cover both spellings. godbus fills Signal.Name as "<interface>.<member>"; match rules given to AddMatchSignal use the bare member. Asserting the bare names route nowhere is what pins the regression shut.
The watchdog existed only to re-arm the poller when a D-Bus signal was missed, which is what the unmatchable signal switch guaranteed. With routing fixed it never fired: across ~20 minutes and ten play/pause cycles every arm came from the signal path and the watchdog logged nothing, so the ticker it was papering over was demonstrably redundant. It was not free. While any MPRIS player was tracked but paused, a tracked-but-idle desktop paid one GetAll per 10s. Removing it restores the plain "zero timers at idle" invariant for paused players, not just untracked ones. Also drop the now-redundant stop call in removePlayer, which duplicated what syncPlayingPollerLocked already does for an empty player list, and record the two signal-layer gotchas (qualified vs bare member names, WithMatchInterface/WithMatchMember) in ARCHITECTURE.md.
The poller is the only remaining MPRIS timer, so capture what it costs and where the headroom is before someone else re-litigates it. Notes that seeking is signal-driven and not polled, that the tick is pure drift correction on top of the phone's posAnchorMs extrapolation, and ranks the three ways to cut it. Also fixes two measurement-method traps hit while taking these numbers: dbus-monitor counts round-trips rather than CPU, and a /proc awk match on voluntary_ctxt_switches also matches nonvoluntary_ctxt_switches.
The phone suppresses every SMS push until the desktop asks once, setting haveMessagesBeenRequested. Nothing here ever asked, so the phone stayed silent and clients had to re-issue a request to see anything new. Arm on connect instead, which is what upstream KDE Connect does. Arming is a one-way door: no packet clears the phone's flag, so an armed phone pushes for the rest of its app's lifetime. That shapes everything else. Notifications therefore carry two independent gates -- the arm time, which drops the per-thread history burst the phone sends in reply to the ask, and the armed check, which keeps a transient `kcd watch` from turning into a permanent notifier. The residual stream after clients leave parses a small body and does nothing. With always_arm off, a bus subscriber-change hook makes the subscription itself the opt-in, so the phone is only asked while somebody is watching. Arming is idempotent per connection, since watch reconnects and would otherwise re-trigger a full burst each time, and the sends run in a goroutine because bus hooks must not block on a 10s write timeout. Also stop logging message bodies. They were written to the journal at debug level, and journals get collected and shipped off-box. log.Observe exists so tests outside internal/log can assert on log content; depguard keeps zap imports in that package, so they cannot build an observing core themselves.
`kcd watch --events=sms.incoming` already filtered correctly but fell
through to the default renderer and printed only "[device] sms.incoming",
with no sender or body. That made the hint the sms subcommands print
("use kcd watch --events sms.incoming to see results") a dead end for
anyone not passing --json.
Adds a case alongside the ten other rendered event types. No new command,
no new IPC, and no change to any existing subcommand's behaviour.
Sixteen event types fell through to the default "[device] type" line, so telephony, volume, connectivity, contacts, ping, battery thresholds, MMS attachments and the device lifecycle events were all unreadable without --json. The docs even showed a rendered telephony line the code never produced. Extract the formatting into formatEvent so it is testable at all, and pin all thirteen pre-existing formats to their exact current output -- the extraction is a pure refactor, and a test failure there means a line a script may already parse has moved. Text mode is for people, so the new formats are prose rather than field dumps: "incoming call: Bob (+1555)" instead of "telephony.ringing: ...". Wording matches the desktop notification the same event raises. Volume keeps the stream name instead of dropping it, sink lists render as "Speaker 42%, Headset (muted)" rather than Go slice syntax, and zero counts are omitted from contacts and the charging flag from battery thresholds. Payloads arrive as untyped JSON, so decodePayload round-trips the ones published as structs to reuse formatConnectivity, which the dedicated `kcd connectivity` command already uses and tests. state.snapshot is left on the bare line on purpose: the IPC layer writes it straight to the socket carrying a full device and plugin dump, which would flood the terminal. ring.received, device.removed and device.disconnected carry no payload.
The [███░] (3/4) form was noisy for what is a four-level reading, and the raw "(3/4)" repeated what the bar already showed. A four-position dot bar reads at a glance and maxes out cleanly, since the reported range is 0-4. Level 0 now reads "No service" rather than an empty bar, which is the difference between a registered SIM with no signal and no SIM at all. An unknown or absent network type reads "Cellular" instead of "CELL". The network label is padded to eight columns so the bars line up across SIMs. The subscription id is still shown verbatim rather than renumbered from 1: the key is a real subId, so renumbering would mislabel a phone that reports a sparse set.
Found by hk check --all: four indented blank lines inside the mermaid state-diagram block and one trailing space in a shell comment. The mermaid diagram renders identically with genuinely empty lines.
hk.pkl existed but only ever made `hk check` work. The installed git hook invokes `hk run pre-commit`, so with no step by that name the hook was a no-op. Top-level steps are what create the implicit check, fix and pre-commit hooks, so declaring them properly turns the hook on. The default set mirrors what CI already enforces, using only tools that are installed, so a clean checkout passes. The shell and GitHub Actions linters the repo would benefit from are behind a `shell` profile: none are installed and CI does not run them, so making them default gates would fail every fresh clone for rules nothing currently enforces. Same for go_vuln_check under `slow`. fail_fast is off so one missing tool reports alongside the rest instead of aborting the run. mise.toml pins golangci-lint to 2.11 to match golangci-lint-action in ci.yml, and go to 1.25 to match go.mod. A local linter ahead of CI means local checks pass while CI fails.
The daemon asked for a connectivity report on every connect, but the phone's ConnectivityReportPlugin declares supportedPacketTypes as empty and its onPacketReceived returns false unconditionally, so the packet could never be processed. The phone already pushes a report whenever its signal state changes. Deleting it removes one packet per connection. The test pins the silence: without it, a deliberate no-op and a forgotten request look identical, and the request gets re-added by someone who assumes it works. This also corrects two doc claims that reports are "requested fresh on every connect" -- the CLI route reads the cache and never requests.
Command output was delivered as a kdeconnect.notification, which the phone
ignores unless the user enables its ReceiveNotificationsPlugin. It is
disabled by default, so output was effectively invisible.
The phone also implements kdeconnect.runcommand.output, which renders into
an in-app card. Emit that instead: commandStarted, batched commandOutput
while the command runs, then commandFinished.
Three properties of the phone's handler constrain the shape. It seeds a
display row from commandStarted and keys later packets to that id, so the
order is mandatory and a finished packet for an unknown id only flips the
spinner. It reads the id with getInt, so the id must fit a 32-bit int --
the previous notification id was a nanosecond timestamp. And it iterates
stdout and stderr without a null check, so both keys are sent on every
batch even when one stream was silent.
Execution now uses separate pipes so the two streams stay distinguishable,
flushed every 250ms and drained eagerly so a burst collapses into one
packet rather than one per line. Output is capped at 2000 lines because
the phone keeps every line in an unbounded list; the truncation is reported
rather than silent. Scanner sends block, so a slow consumer applies
backpressure instead of discarding output.
Handle{"stop":true} now cancels a running execution, which the phone's
stop button already offers, and disconnect cancels too. The notification
fallback stays for anyone who has not enabled the output card.
Two things found while preparing v1.21.0. Cancelling a command could hang instead of stopping it. exec.CommandContext kills only the direct child, so when the shell forks rather than execs, the orphan holds the write end of the pipes open: the scanners never see EOF, Wait is never reached, and the plugin's WaitGroup never drains. The local /bin/sh execs so this passed here, while CI's runner forks and hung for the full timeout. Closing our read ends when the context ends makes it deterministic whatever the shell does. The new test uses a command that forks a child on purpose, and fails without the fix. SMS push is now opt-in rather than on. An armed phone cannot be un-armed, so arming it on every user's behalf is a standing privacy commitment nobody asked for, and the feature wants more soak time before defaulting. The mechanism is unchanged: subscribing to sms.incoming still arms the push, and [sms] always_arm = true restores asking on every connect.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Candidate for v1.21.0. 12 non-merge commits on top of v1.20.0.
User-visible changes
SMS now arrives by push. The daemon asked the phone for a conversation
list on demand and clients had to re-request to see anything new. The phone
suppresses all SMS push until the desktop asks once, and nothing ever did,
so
kdeconnect.sms.messagesnever arrived unprompted. The daemon now armspush on connect, so a client only has to subscribe to
sms.incoming.Arming is a one-way door: no packet clears the phone's flag, so an armed
phone pushes for the rest of its app's lifetime. That is why notifications
carry two independent gates — the arm time, which drops the per-thread
history burst the phone sends in reply to the ask, and the armed check,
which keeps a transient
kcd watchfrom becoming a permanent notifier.New
[sms] always_arm, off by default. An armed phone cannot be un-armed,so asking on every user's behalf is a standing privacy commitment nobody asked
for; the feature wants more soak time before defaulting. Subscribing to
sms.incomingstill arms the push, andalways_arm = truerestores upstreamKDE Connect's ask-on-every-connect.
MPRIS is timer-free at idle. The 10s watchdog that re-armed the
position poller on missed signals is gone. It existed only because signal
routing was broken, which #46 fixed. Measured after: 0
GetAll/min and 0context switches while paused, down from ~6 reads/min. The poller is now
armed purely by D-Bus signals and self-stops on a confirmed pause.
Command output now shows in the phone's output card. It was delivered
as a desktop notification, which the phone ignores unless the user enables
its ReceiveNotificationsPlugin — disabled by default, so output was
effectively invisible. Now streamed via
kdeconnect.runcommand.output.{"stop": true}from the phone cancels a running execution, which its stopbutton already offered. The notification fallback stays.
kcd watchrenders every event type. Sixteen types fell through to abare
[device] typeline, so telephony, volume, connectivity, contacts,ping, battery thresholds, MMS attachments and the device lifecycle were all
unreadable without
--json. Docs also showed a rendered telephony line thecode never produced. Text mode is prose for people now;
--jsonremainsthe stable contract.
kcd connectivityrenders a dot bar instead of[███░] (3/4), andNo serviceat level 0.Fix found during release prep
Cancelling a command could hang instead of stopping it.
exec.CommandContextkills only the direct child, so when the shell forks rather than execs, the
orphan holds the pipes open, the scanners never see EOF, and the plugin's
WaitGroup never drains. The local
/bin/shexecs so it passed here; CI'srunner forks and hung for the full timeout. The pipes are now closed on
cancel, and the new test forks a child deliberately.
Other
kdeconnect.connectivity_report.requestthe phone cannotreceive: its plugin declares no incoming packet types and rejects every
packet. Corrects two docs that claimed reports are requested on connect.
at debug level, and journals get collected and shipped off-box.
Known gaps
telephony push, not on the SMS push path, so blocked senders can still
arrive. Documented in
kcd.example.tomland the guide.kdeconnect.telephonyevent: "sms"packet is stillignored on purpose. It only reaches a desktop when KDE Connect is the
phone's default SMS app, and handling it would create a second source for
an existing event. See
docs/ARCHITECTURE.md.kcd watchcannot follow arunning command.
Verification
All five CI jobs run clean locally against the pinned toolchain
(go 1.25.14, golangci-lint 2.11.4), including the four integration tests,
which are behind
-tags integrationand had not been run on this branchbefore. Runcommand output and SMS push were both confirmed against a real
phone.