core+qt: add setConnectHandler/setDisconnectHandler to IBackend - #39
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Yaraslaut
added a commit
that referenced
this pull request
Aug 3, 2026
… no-ops QtWebSocketBackend overrides both hooks added for #29, so the base IBackend "store-and-ignore" bodies (backend.hpp:273,288) were never exercised, tripping the codecov/patch gate at 0% on PR #39. LocalBackend does not override either, so a direct call through it hits the real default implementation. Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
setReconnectHandler deliberately fires only on the second and later connects -- it exists so Bridge can re-register handlers after a drop, and skipping the initial connect is correct for that purpose. There was no notification for the first successful connect (waitForConnected() answers this but blocks, unusable on a browser/WASM main thread) or for a disconnect at all (a client learned the socket dropped only indirectly, when a later action failed) -- both gaps a connection-state UI needs closed. Add setConnectHandler (fires on every successful connect, first included) and setDisconnectHandler (fires whenever the transport drops, before any reconnect is scheduled -- so an observer sees the disconnected state even when a retry follows immediately) to IBackend itself, with the same no-op-default pattern setReconnectHandler already established, rather than only on QtWebSocketBackend: connection state is a property of any transport-backed backend, and a UI observing it shouldn't have to downcast to a concrete type. A purely local backend has no meaningful connection state, so the base-class hook is simply inert for it -- matching precedent exactly, since setReconnectHandler already works this way. QtWebSocketBackend's connected/disconnected QWebSocket signal slots invoke the new handlers (if installed) at the same points they already invoke _reconnectHandler/schedule a reconnect. Purely additive: setReconnectHandler keeps its current semantics and every existing embedder is unaffected. Closes #29 Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
… no-ops QtWebSocketBackend overrides both hooks added for #29, so the base IBackend "store-and-ignore" bodies (backend.hpp:273,288) were never exercised, tripping the codecov/patch gate at 0% on PR #39. LocalBackend does not override either, so a direct call through it hits the real default implementation. Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
Yaraslaut
force-pushed
the
feature/29-connect-disconnect-notifications
branch
from
August 4, 2026 16:51
766c651 to
a5fed12
Compare
Two gaps flagged during code review and left as "worth noting" rather than fixed at the time: 1. setConnectHandler/setDisconnectHandler/setReconnectHandler's documented nullptr-to-clear behavior had zero test coverage. Add a test that installs all three, drives one full disconnect/reconnect cycle to confirm each fires, clears all three via nullptr, drives a second cycle, and confirms none of the counters move. 2. The class doc comment's claim that the disconnect handler "fires before any reconnect is scheduled" was unverified -- the existing test sidesteps it via reconnectEnabled = false. Add a test where the disconnect handler itself observes reconnectCount == 0 at the moment it runs, then confirms the reconnect handler does eventually fire once a fresh server comes up on the same port. Verified locally: all 4 [issue29]-tagged cases pass across 3 repeated runs (timing-sensitive reconnect assertions), full Qt suite (57 cases) passes. Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
Yaraslaut
added a commit
that referenced
this pull request
Aug 7, 2026
ExportBookmarks entity-escapes rec.url via escapeHtml() (necessary -- HREF="..." is attribute-quoted, so a literal '&', '<', '>', or '"' in a URL must be escaped to keep the output well-formed). But parseNetscapeChunk() never decoded the HREF value back on import, only the title -- so a URL containing '&' (an extremely common case in real query strings, e.g. https://example.com/search?a=1&b=2) got corrupted on any export/reimport cycle: the literal text "&b=2" ended up baked into the reimported URL instead of decoding back to "&b=2". Fix: apply the same decodeEntities() call already used for the title to the parsed HREF value, making import symmetric with export (option 1 from the two documented alternatives). This is simpler than making export stop escaping the URL, and it fixes the header's other disclosed out-of-scope case (a URL containing a literal quote) for free, since the quote now round-trips through " instead of breaking the attribute boundary. Also drop a dead, confusing "';" branch in decodeEntities that could only match a stray extra ';' in the input and produced the same output as the "'" branch beneath it anyway. Add a regression test proving the round trip: a bookmark with URL https://example.com/search?a=1&b=2 survives ExportBookmarks -> ImportBookmarks unchanged, plus a direct parseNetscapeChunk test for HREF entity-decoding. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HffixfknmXdAjnM5hSGRBb
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.
Summary
setReconnectHandlerdeliberately fires only on the second and later connects — correct for its purpose (Bridgere-registering handlers after a drop), but it leaves no notification for the first successful connect (waitForConnected()answers this but blocks — unusable on a browser/WASM main thread) or for a disconnect at all (previously only discoverable indirectly, via a later failed action).setConnectHandler(fires on every successful connect, first included) andsetDisconnectHandler(fires whenever the transport drops, before any reconnect is scheduled, so an observer sees the disconnected state even when a retry follows immediately).IBackendor only onQtWebSocketBackend? — and leaned toward the interface, citingsetReconnectHandler's exact existing precedent (already onIBackendwith a no-op default). Followed that lean directly, since the precedent is exact: connection state is a property of any transport-backed backend, a UI observing it shouldn't have to downcast to a concrete type, and a purely local backend simply never invokes either (inert, zero behavior change).QtWebSocketBackend'sconnected/disconnectedsignal slots invoke the new handlers (if installed) at the same points they already invoke_reconnectHandler/schedule a reconnect. Purely additive —setReconnectHandlerkeeps its current semantics and every existing embedder is unaffected.Test plan
tests/qt/test_qt_websocket.cpp: new end-to-end tests against a real Qt WebSocket server —setConnectHandlerfires exactly once on the first connect whilesetReconnectHandlerdoes not;setDisconnectHandlerfires when the server is torn down mid-session (reconnect disabled to isolate the assertion)../build/tests/morph_tests— all 811 test cases / 8284 assertions pass, confirming theIBackendinterface addition is a no-op for every backend that doesn't override it.Closes #29
🤖 Generated with Claude Code