Improve Android "install failed" messages - #2949
Conversation
|
|
|
Looking for feedback on the error messages. Happy to change them if needed. Also, since both messages suggest uninstalling as the remedy, should there be a note warning that this wipes the app's data? @mhsmith raised the same concern back in the original thread, just in the context of an automatic uninstall/reinstall flow (#1362 (comment)), but it seems just as relevant here since someone could copy-paste the command without realizing the consequence. |
|
Another note I'd like to mention is the adb command in these messages may not resolve if it's not on the user's PATH, still working on that fix. |
|
Updated the error message to use Briefcase's managed adb path instead of assuming adb is already on the user's PATH. @freakboy3742 or @mhsmith whenever you get a chance, this is ready for review. |
mhsmith
changed the title
|
@moondial-pal Apologies for the delay in reviewing this - we should be able to get to this in the next couple of days. |
| computer). To resolve this, run the following command to uninstall the | ||
| existing app, then try again: | ||
|
|
||
| $ "{self.tools.android_sdk.adb_path}" -s {self.device} """ |
There was a problem hiding this comment.
Windows users might not recognize "$" as a prompt, and think it's part of the command line. Let's just have no prompt at all.
| $ "{self.tools.android_sdk.adb_path}" -s {self.device} """ | |
| "{self.tools.android_sdk.adb_path}" -s {self.device} """ |
There was a problem hiding this comment.
Also, this repeated command line should be factored out into a variable. The variable can also include the "To resolve this" sentence, along with the data loss warning you suggested.
| Unable to install APK {apk_path} on {self.device} | ||
|
|
||
| The app currently installed on the device was signed with a different key | ||
| than this APK (often because it was previously installed from a different |
There was a problem hiding this comment.
Better to focus on the user's current situation rather than making generalizations.
| than this APK (often because it was previously installed from a different | |
| than this APK (maybe because it was installed from a different |
| raise BriefcaseCommandError( | ||
| f"""\ | ||
| Unable to install APK {apk_path} on {self.device} |
There was a problem hiding this comment.
We recently added some helper methods to avoid this kind of confusing indentation (#2559). However, in this case we don't need the asterisk box to call attention to the message, because it's the last thing printed before Briefcase exits.
So I think we should make _dedent_and_wrap a public function, and make its width argument default to 80 like the other functions. Then it can be used here to wrap the message, similarly to this example. It looks like it already avoids wrapping further-indented lines, such as the command line here.
| ) from e | ||
| else: | ||
| raise BriefcaseCommandError( | ||
| f"Unable to install APK {apk_path} on {self.device}" |
There was a problem hiding this comment.
The 3 copies of this message should be factored out into a variable.
|
|
||
| with pytest.raises(BriefcaseCommandError) as exc_info: | ||
| adb.install_apk("example.apk", "com.example.helloworld") | ||
| assert "uninstall com.example.helloworld" in str(exc_info.value) |
There was a problem hiding this comment.
We don't need to include the full error text, but we should at least include enough words to check that the two different errors produce two different messages.
Yes, that would be a good idea. |
|
Thanks @mhsmith I'll get to work cleaning this up and addressing the feedback. |
3 participants
The two known failure markers
UPDATE_INCOMPATIBLEandVERSION_DOWNGRADEnow present the user with an informative error message indicating the probable cause and include a suggested route for resolution.Previously, both failures produced the same generic 'Unable to install APK' message, giving users no indication of what went wrong or how to fix it.
Refs #1362
PR Checklist:
Assisted-by: Claude Sonnet