release: use 24.11rc1 (locally, add SYNC_SERVER_MESSAGE handling) #439
Conversation
| @@ -8,7 +8,6 @@ use std::env; | |||
| use std::env::consts::OS; | |||
| use std::fs; | |||
| use std::io::{self, Write}; | |||
| use std::ops::Deref; | |||
There was a problem hiding this comment.
unused import likely from earlier changes and dev iterations on typescript binding listing, I just threw this in there both "for free" and to confuse you about why it's in there. Thus this comment.
| @@ -19,7 +19,7 @@ android.useAndroidX=true | |||
| android.enableJetifier=false | |||
|
|
|||
| GROUP=io.github.david-allison | |||
| VERSION_NAME=0.1.46-anki24.10rc2 | |||
| VERSION_NAME=0.1.47-anki24.11rc1 | |||
There was a problem hiding this comment.
bumped local number since this won't be cross-compatible with prior versions of anki etc as there were local changes
There was a problem hiding this comment.
mvn central status seems green
There was a problem hiding this comment.
Got a headache, review may not be fully thought through.
The question is "what we want to do with the exception?"
I feel we want to special-case the 'revalidate' at the very least:
- We want the dialog to have a button to open up AnkiWeb
- We want the user's email as a parameter to the exception
So we want to change ServerMessage(error) into an exception builder, in the same way that BackendDbException.fromDbError works
ExceptionTest should include the new exception
|
Server messages are arbitrary text. If you want to handle a specific sub-case in a non-fragile way, it's going to require reworking the sync protocol to return a specific flag or error code instead of the message. |
|
Based on current 2 server messages like this - reconfirm email + agree to Ts&Cs - I think (@dae please correct if wrong) a server message could be handled by just showing it to the user? Maybe with an optional "open ankiweb.net directly" button since that appears to be the typical need? And then don't log it anywhere of course :-) In that case, just having it cased here so we can do that seems sufficient? |
|
Yep, they're intended to be directly shown to the user and not parsed by the client, and there's more than 2, e.g: By keeping them as strings, it allows new error cases to be introduced without a client upgrade. |
|
That makes sense, thanks. So KISS principle is just show them to the user, and that means that we don't need anything fancier in this repo than the typing here, which is what I believe this PR does I still like the idea of an "open ankiweb" type UI element but the wording will need to be carefully considered since it needs to contain the idea "hey, this may help but also it may not be relevant at all". Tough wording challenge but a minor point really |
mikehardy
force-pushed
the
24.11rc1-sync-server-message
branch
from
1701b38 to
e73cfcd
Compare
Sorry! skipped past that - just missed it Just re-pushed with the addition of that exception type to the test - did I do that correctly? I'm not sure what exactly that exercises to be honest 🤔 |
Adopts the new upstream version, with the locally-interesting change being the split-out handling of new SYNC_SERVER_MESSAGE
I...think I did that correctly? Extra eyes + strict review keenly desired