Added additional debug info (WebView/Hardware) #8250

2tanayk · 2021-03-16T22:05:41Z

Pull Request template

Purpose / Description

Added additional and useful debug info -device manufacturer, model, hardware and webview user agent.

Fixes

Approach

Used the android os class Build. to extract manufacturer, model and hardware information
Used an anonymous instance of the Webview class which is new WebView(this).getSettings().getUserAgentString() to extract the webview user agent

How Has This Been Tested?

Tested on an actual android device which runs on Android 10(API 29)

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration (SDK version(s), emulator or physical, etc)

Checklist

Please, go through these checks before submitting the PR.

You have not changed whitespace unnecessarily (it makes diffs hard to read)
You have a descriptive commit message with a short title (first line, max 50 chars).
Your code follows the style of the project (e.g. never omit braces in if statements)
You have commented your code, particularly in hard-to-understand areas
You have performed a self-review of your own code
UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
UI Changes: You have tested your change using the Google Accessibility Scanner

codecov · 2021-03-16T22:15:28Z

Codecov Report

Merging #8250 (e8141a2) into master (5f44643) will increase coverage by 0.12%.
The diff coverage is 33.11%.

❗ Current head e8141a2 differs from pull request most recent head 8213854. Consider uploading reports for the commit 8213854 to get more accurate results

@@             Coverage Diff              @@
##             master    #8250      +/-   ##
============================================
+ Coverage     36.88%   37.00%   +0.12%     
- Complexity     3736     3788      +52     
============================================
  Files           347      355       +8     
  Lines         35676    35843     +167     
  Branches       4724     4744      +20     
============================================
+ Hits          13158    13263     +105     
- Misses        21010    21069      +59     
- Partials       1508     1511       +3

Impacted Files	Coverage Δ	Complexity Δ
...oid/src/main/java/com/ichi2/anki/AnkiActivity.java	`52.22% <0.00%> (-2.43%)`	`49.00 <0.00> (ø)`
...Droid/src/main/java/com/ichi2/anki/DeckPicker.java	`19.46% <0.00%> (-0.24%)`	`56.00 <0.00> (-1.00)`
.../main/java/com/ichi2/anki/FilteredDeckOptions.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
AnkiDroid/src/main/java/com/ichi2/anki/Info.java	`1.85% <0.00%> (-0.30%)`	`2.00 <0.00> (ø)`
...iDroid/src/main/java/com/ichi2/anki/MyAccount.java	`2.96% <0.00%> (ø)`	`2.00 <0.00> (ø)`
...roid/src/main/java/com/ichi2/anki/Preferences.java	`11.26% <0.00%> (-0.32%)`	`15.00 <0.00> (ø)`
...Droid/src/main/java/com/ichi2/anki/Statistics.java	`1.04% <0.00%> (+<0.01%)`	`2.00 <0.00> (ø)`
...main/java/com/ichi2/anki/StudyOptionsFragment.java	`0.88% <0.00%> (ø)`	`1.00 <0.00> (ø)`
.../com/ichi2/anki/dialogs/DeckPickerContextMenu.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...main/java/com/ichi2/anki/dialogs/ExportDialog.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
... and 69 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ead42c...8213854. Read the comment docs.

david-allison

Thanks! 2 minor changes, then good to go

AnkiDroid/src/main/java/com/ichi2/anki/Info.java

david-allison

Looks good to me, thanks!

2tanayk · 2021-03-17T06:47:53Z

Looks good to me, thanks!

some JUnit test is failing,why is that?

david-allison · 2021-03-17T08:32:53Z

Likely flakiness/nondeterminism in our unit tests - I haven't been through to fix them, and I think the migration from TravisCI to GitHub actions brought out a few issues

Arthur has a PR waiting which should fix the issue once and for all, and I haven't had the time to truly think through the design

2tanayk · 2021-03-18T14:49:51Z

Any updates regarding this ?Is everything fine?

mikehardy · 2021-03-18T15:47:13Z

I have updated the PR description to include Fixes #issuenumber vs just #issuenumber, adding the Fixes as we request in the PR template is important because that is how GitHub knows to related the PR and issue in a way that closes the issue and cross-links them more strongly.

mikehardy

one control flow issue, two spacing nits, excited to get this expanded info from users though!

mikehardy · 2021-03-18T15:50:27Z

AnkiDroid/src/main/java/com/ichi2/anki/Info.java

+            webviewUserAgent = getWebviewUserAgent();
+        } catch (Throwable e) {
+            AnkiDroidApp.sendExceptionReport(e, "Info::copyDebugInfo()", "some issue occured while extracting webview user agent");
+            return null;


why would we return in this case, without returning the rest of the information ?
None of the other try/catch blocks return and so in no other case will this method return null, meaning that now consumers have to null check it whereas before it was safe.

This catch block should not return, and definitely should not return null

mikehardy · 2021-03-18T15:51:45Z

AnkiDroid/src/main/java/com/ichi2/anki/Info.java

+                "Manufacturer = " + Build.MANUFACTURER + "\n\n" +
+                "Model = " + Build.MODEL + "\n\n" +
+                "Hardware = " + Build.HARDWARE+"\n\n" +
+                "Webview User Agent = "+ webviewUserAgent +


Suggested change

"Webview User Agent = "+ webviewUserAgent +

"Webview User Agent = " + webviewUserAgent +

spacing nit: https://github.com/ankidroid/Anki-Android/wiki/Code-style#white-space-should-be-used-in-the-following-cases -> "after and before operators"

mikehardy · 2021-03-18T15:52:01Z

AnkiDroid/src/main/java/com/ichi2/anki/Info.java

@@ -236,6 +247,9 @@ public String copyDebugInfo() {
        return debugInfo;
    }

+    private String getWebviewUserAgent() {
+        return new WebView(this).getSettings().getUserAgentString()+ "\n\n";


Suggested change

return new WebView(this).getSettings().getUserAgentString()+ "\n\n";

return new WebView(this).getSettings().getUserAgentString() + "\n\n";

https://github.com/ankidroid/Anki-Android/wiki/Code-style#white-space-should-be-used-in-the-following-cases

2tanayk · 2021-03-18T21:28:28Z

Will get it done as soon as possible sir✌️

2tanayk · 2021-03-19T08:41:39Z

done @mikehardy sir!

david-allison

As discussed on Discord - needs rebasing onto master and removing the merge commit

needs rebase

mikehardy · 2021-03-19T16:47:49Z

Yeah, sorry - the commit history is non-reviewable at the moment. Once cleaned up I can re-check

2tanayk · 2021-03-19T18:43:31Z

Yeah, sorry - the commit history is non-reviewable at the moment. Once cleaned up I can re-check

sir it is the same code as from the PR ,just except that due to the rebase it has other commits with it 😅,I can squash all the commits into one but would that help?

mikehardy · 2021-03-20T13:54:22Z

When I open the "Commits" tab on the PR I should see nothing but clean commits that implement your changes only, no unrelated commits

When I open the "Files Changed" tab on the PR I should see nothing but an easily readable diff containing nothing but the actual changes you needed to make to implement the improvement

There are many ways to do this with git, I don't have the time to lead you through them, but choose one that meets my expectations of a clean diff as described here, and I can review it

mikehardy · 2021-03-20T16:28:59Z

That is sometimes the best option, although I think there is value for you - as a person learning git - in figuring out how to actually fix this using git rebase. It won't be ...fun, but you will have much stronger control over git afterwards. Your choice

2tanayk · 2021-03-20T19:12:25Z

That is sometimes the best option, although I think there is value for you - as a person learning git - in figuring out how to actually fix this using git rebase. It won't be ...fun, but you will have much stronger control over git afterwards. Your choice

Yes I get that!

2tanayk · 2021-03-22T08:32:28Z

Now let me know if I've got it right this time :)

david-allison

Thanks! Tiny nitpick.

Almost there.

Could you squash the 3 commits into one

I feel the commit message would be better as:

Added additional debug info (WebView/Hardware)

Fixes #8234

This provides more context for the commit, the issue number isn't too relevant when looking for context in a long list of issues.

david-allison · 2021-03-22T08:30:34Z

AnkiDroid/src/main/java/com/ichi2/anki/Info.java


        String debugInfo = "AnkiDroid Version = " + VersionUtils.getPkgVersionName() + "\n\n" +
                "Android Version = " + Build.VERSION.RELEASE + "\n\n" +
+                "Manufacturer = " + Build.MANUFACTURER + "\n\n" +
+                "Model = " + Build.MODEL + "\n\n" +
+                "Hardware = " + Build.HARDWARE+"\n\n" +


Suggested change

"Hardware = " + Build.HARDWARE+"\n\n" +

"Hardware = " + Build.HARDWARE+ "\n\n" +

david-allison · 2021-03-22T08:32:04Z

AnkiDroid/src/main/java/com/ichi2/anki/Info.java

@@ -236,6 +246,9 @@ public String copyDebugInfo() {
        return debugInfo;
    }

+    private String getWebviewUserAgent() {
+        return new WebView(this).getSettings().getUserAgentString() + "\n\n";


Nitpick/Implementer's choice: code style

I feel that the try/catch would be better inside this method, this allows us to reduce code in the copyDebugInfo method, and move the method call inline to where it's used

Yeah thats what I really wanted to do but it gives a compile time error (Java doesn't risk return in non-void functions) and hence I will have to return something else (probably null) if the try block throws an exception which makes the code unsafe again.

I will squash the commits for now

You can return null, that's what we're doing implicitly here anyway

ok did that

david-allison

Last one!

One last thing!

Note that the PR title isn't the commit message title

You can change the commit message title via a reword operation in a rebase.

Alternately, Android Studio lets you do this with a UI:

AnkiDroid/src/main/java/com/ichi2/anki/Info.java

2tanayk · 2021-03-22T10:01:42Z

I think all is taken care of ,can you check @david-allison-1

Fixes ankidroid#8234

david-allison

Looks good to me, thanks!

mikehardy

good to go! Thanks

mikehardy · 2021-04-05T18:33:00Z

Hi there!

Just a friendly notice (one per person for PRs merged in March, regardless of PR quantity) that we try to process OpenCollective payments monthly - it's time for March 2021 submissions

If you are interested in compensation for this work, the process with details is here:

https://github.com/ankidroid/Anki-Android/wiki/OpenCollective-Payment-Process#how-to-get-paid

Thanks!

mikehardy · 2021-06-04T21:35:24Z

@2tanayk I was able to just immediately diagnose a user problem as an out-of-date webview based on this extra debugging information, and it fixed the user immediately when they updated. I love seeing things like this pay off! Thanks again

2tanayk · 2021-06-05T14:30:31Z

Thanks @mikehardy means a lot to me and my semester almost over,look to start contributing again!

david-allison requested changes Mar 16, 2021

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/Info.java Outdated Show resolved Hide resolved

AnkiDroid/src/main/java/com/ichi2/anki/Info.java Outdated Show resolved Hide resolved

david-allison added the Needs Author Reply Waiting for a reply from the original author label Mar 16, 2021

david-allison reviewed Mar 16, 2021

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/Info.java Outdated Show resolved Hide resolved

david-allison previously approved these changes Mar 16, 2021

View reviewed changes

david-allison added Needs Second Approval Has one approval, one more approval to merge and removed Needs Author Reply Waiting for a reply from the original author labels Mar 16, 2021

mikehardy added this to the 2.15 release milestone Mar 18, 2021

mikehardy requested changes Mar 18, 2021

View reviewed changes

david-allison self-requested a review March 19, 2021 08:58

david-allison reviewed Mar 19, 2021

View reviewed changes

2tanayk force-pushed the fixes-issue-8234 branch from 22338fe to e8141a2 Compare March 19, 2021 11:03

david-allison self-requested a review March 19, 2021 11:10

mikehardy added Needs Author Reply Waiting for a reply from the original author and removed Needs Second Approval Has one approval, one more approval to merge labels Mar 19, 2021

2tanayk force-pushed the fixes-issue-8234 branch from e8141a2 to 8213854 Compare March 22, 2021 08:24

david-allison requested changes Mar 22, 2021

View reviewed changes

2tanayk force-pushed the fixes-issue-8234 branch from 8213854 to e1f0735 Compare March 22, 2021 09:22

2tanayk changed the title ~~Fixes #8234 Additional debug info added~~ Added additional debug info (WebView/Hardware) Mar 22, 2021

david-allison reviewed Mar 22, 2021

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/Info.java Outdated Show resolved Hide resolved

AnkiDroid/src/main/java/com/ichi2/anki/Info.java Outdated Show resolved Hide resolved

2tanayk force-pushed the fixes-issue-8234 branch from 28a7588 to ed2fd93 Compare March 22, 2021 10:00

2tanayk added 3 commits March 22, 2021 20:58

Added additional debug info (WebView/Hardware)

d37e05d

Fixes ankidroid#8234

Added additional debug info (WebView/Hardware)

74c7d68

Fixes ankidroid#8234

Added additional debug info (WebView/Hardware)

710728d

Fixes ankidroid#8234

2tanayk force-pushed the fixes-issue-8234 branch from ed2fd93 to 710728d Compare March 22, 2021 15:29

david-allison removed the Needs Author Reply Waiting for a reply from the original author label Mar 24, 2021

david-allison approved these changes Mar 24, 2021

View reviewed changes

david-allison added the Needs Second Approval Has one approval, one more approval to merge label Mar 24, 2021

mikehardy approved these changes Mar 24, 2021

View reviewed changes

mikehardy merged commit 175d961 into ankidroid:master Mar 24, 2021

mikehardy removed the Needs Second Approval Has one approval, one more approval to merge label Mar 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added additional debug info (WebView/Hardware) #8250

Added additional debug info (WebView/Hardware) #8250

2tanayk commented Mar 16, 2021 •

edited by mikehardy

Loading

codecov bot commented Mar 16, 2021 •

edited

Loading

david-allison left a comment

david-allison left a comment

2tanayk commented Mar 17, 2021

david-allison commented Mar 17, 2021

2tanayk commented Mar 18, 2021

mikehardy commented Mar 18, 2021

mikehardy left a comment

mikehardy Mar 18, 2021

mikehardy Mar 18, 2021

mikehardy Mar 18, 2021

2tanayk commented Mar 18, 2021

2tanayk commented Mar 19, 2021

david-allison left a comment

mikehardy commented Mar 19, 2021

2tanayk commented Mar 19, 2021 •

edited

Loading

mikehardy commented Mar 20, 2021

mikehardy commented Mar 20, 2021

2tanayk commented Mar 20, 2021

2tanayk commented Mar 22, 2021

david-allison left a comment

david-allison Mar 22, 2021

david-allison Mar 22, 2021

2tanayk Mar 22, 2021 •

edited

Loading

2tanayk Mar 22, 2021

david-allison Mar 22, 2021

2tanayk Mar 22, 2021

david-allison left a comment •

edited

Loading

2tanayk commented Mar 22, 2021 •

edited

Loading

david-allison left a comment

mikehardy left a comment

mikehardy commented Apr 5, 2021

mikehardy commented Jun 4, 2021

2tanayk commented Jun 5, 2021

	"Webview User Agent = "+ webviewUserAgent +
	"Webview User Agent = " + webviewUserAgent +

	return new WebView(this).getSettings().getUserAgentString()+ "\n\n";
	return new WebView(this).getSettings().getUserAgentString() + "\n\n";

	"Hardware = " + Build.HARDWARE+"\n\n" +
	"Hardware = " + Build.HARDWARE+ "\n\n" +

Added additional debug info (WebView/Hardware) #8250

Added additional debug info (WebView/Hardware) #8250

Conversation

2tanayk commented Mar 16, 2021 • edited by mikehardy Loading

Pull Request template

Purpose / Description

Fixes

Approach

How Has This Been Tested?

Checklist

codecov bot commented Mar 16, 2021 • edited Loading

Codecov Report

david-allison left a comment

Choose a reason for hiding this comment

david-allison left a comment

Choose a reason for hiding this comment

2tanayk commented Mar 17, 2021

david-allison commented Mar 17, 2021

2tanayk commented Mar 18, 2021

mikehardy commented Mar 18, 2021

mikehardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2tanayk commented Mar 18, 2021

2tanayk commented Mar 19, 2021

david-allison left a comment

Choose a reason for hiding this comment

mikehardy commented Mar 19, 2021

2tanayk commented Mar 19, 2021 • edited Loading

mikehardy commented Mar 20, 2021

mikehardy commented Mar 20, 2021

2tanayk commented Mar 20, 2021

2tanayk commented Mar 22, 2021

david-allison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2tanayk Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-allison left a comment • edited Loading

Choose a reason for hiding this comment

2tanayk commented Mar 22, 2021 • edited Loading

david-allison left a comment

Choose a reason for hiding this comment

mikehardy left a comment

Choose a reason for hiding this comment

mikehardy commented Apr 5, 2021

mikehardy commented Jun 4, 2021

2tanayk commented Jun 5, 2021

2tanayk commented Mar 16, 2021 •

edited by mikehardy

Loading

codecov bot commented Mar 16, 2021 •

edited

Loading

2tanayk commented Mar 19, 2021 •

edited

Loading

2tanayk Mar 22, 2021 •

edited

Loading

david-allison left a comment •

edited

Loading

2tanayk commented Mar 22, 2021 •

edited

Loading