fix: restore "don't show again", and put Sharing in the drawer - #87
Merged
Merged
Conversation
Two things that should not ship as they are. **"Don't show again" never survived a restart.** ClientSettingsResponse names the field clientId while the server sends client_id, and Moshi does not convert snake_case. Being non-null with no default, the missing field failed the WHOLE response, so every read of client settings threw - and both callers wrap the call in runCatching, so it failed silently. The write worked throughout: the dismissal was saved server-side and simply never read back, which is why the announcement returned on the next launch and why the bug looked like a persistence failure rather than a parse one. A contract test now decodes the shape the server actually sends, through the app's own Moshi rather than a bare one, since this type is handled by KotlinJsonAdapterFactory and a plain builder cannot read it at all. Sweeping the rest of the models for the same mistake found only FrontUpdate, whose hand-written adapter writes the snake_case names itself, and the watch's models, which are local storage with their own serialisation. **Sharing was reachable only through Settings.** It is where you go to take something down, which is not a thing to go hunting through settings for, and web has had it as a top-level destination all along. It joins the drawer with the rest of the system, and can be pinned. The entry appears when the instance serves public profiles, or when it does not but this account still has grants on file - grants outlive the instance switch, and hiding the entry then would leave somebody published with no way to reach the button that unpublishes them. Availability is re-asked when the drawer opens, so publishing a first link does not need a relaunch to show the entry. The Settings > System row stays, since a second way in costs nothing. A pinned Sharing slot on a server that has since turned sharing off keeps its place, dimmed, and says so when tapped, rather than vanishing from somebody's bar because of a setting they do not control.
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
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.
Two bugs found in 1.4.0 before it went to Play. 1.4.0 is already tagged and its GitHub release is published with signed
openAPKs, so these want a 1.4.1 rather than a re-cut: anyone who has installedsheaf-1.4.0-open.apkholds versionCode 104000, and re-issuing different bytes under that same code leaves them with no upgrade path."Don't show again" never survived a restart
ClientSettingsResponsenames the fieldclientId; the server sendsclient_id; Moshi does not convert snake_case. Non-null with no default, so the missing field failed the whole response, and both callers wrap the call inrunCatching- so every read of client settings threw, silently, for the life of the feature.The write path was fine throughout. The dismissal reached the server and was stored; it just could never be read back, which is why this looked like a persistence bug rather than a parse one.
A contract test now decodes the shape the server actually sends. It uses the app's own Moshi rather than a bare one, because this type is handled by
KotlinJsonAdapterFactoryand a plain builder cannot read it at all - which is also why the existing model tests never caught it.I swept every model for the same mistake. The only other camelCase-without-
@Jsonproperties are onFrontUpdate, whose hand-written adapter writes the snake_case names itself, and on the watch's local-storage models, which never touch the wire.Anything dismissed permanently before this stays dismissed - the data was always there.
Sharing was reachable only through Settings
It's where you go to take something down, which is not a thing to go hunting through Settings for, and web has had it as a top-level destination all along. It now joins the drawer with the rest of the system, and can be pinned to the bottom bar.