search

evristzam / ndt

Automatically exported from code.google.com/p/ndt
Other
0 stars 0 forks source link

Patch review for issues 62, 63, 64, and Commit Request #67

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
The included patch applies cleanly to the branch under branches/android.

Please review the patch an commit to the branches/android tree.  Once the patch 
is committed I will update the NDT app in the Google Play store.

$ svn checkout http://ndt.googlecode.com/svn/branches/android/ ndt-android
$ cd ndt-android
$ patch -p1 < ../update-1.1.patch 
patching file Android/AndroidManifest.xml
patching file Android/res/values/strings.xml
patching file Android/src/net/measurementlab/ndt/NdtService.java
patching file Android/src/net/measurementlab/ndt/ResultsActivity.java
patching file Android/src/net/measurementlab/ndt/SelectServerActivity.java
patching file Android/src/net/measurementlab/ndt/TestsActivity.java
patching file java/net/measurementlab/ndt/NdtTests.java

This patch makes the following changes:

 * Updates version string from 1.0b2 to 1.0b3
 * Adds all current M-Lab servers to SelectServerActivity.java
 * Sets Android Activities to screenOrientation="portrait" to prevent activity restarts that resulted in faults from automatic orientation changes.
 * Updates parseInt() calls in error handling code.
 * Corrects display of "Optimal receive buffer & % time not spent in receiver or sender limited state".
 * Corrects display of TEST SERVER IP address.  Previously the default was always displayed rather than the user-selected address.

REMAINING, IDENTIFIED ISSUES:
 * Feedback to user is ambiguous when an NDT server is not working correctly.
 * Some operations, such as 'back'-'back'-restart and opening a flip phone still cause tests to be interrupted.
 * Once a test fails, restarts do not correct the problem.

"Crash Errors" reported in Play-store, developer console.
This patch FIXES:
Integer format faults:
NumberFormatException 51 reports 2 reports/week
NumberFormatException 111 reports 0 reports/week

Resume Activity faults:
IllegalArgumentException 98 reports 2 reports/week
NullPointerException 1 reports 0 reports/week
NullPointerException 3 reports 0 reports/week
NullPointerException 27 reports

This patch does NOT ADDRESS:
MissingResourceException 5 reports 0 reports/week

I could not identify nor reproduce this error:
java.util.MissingResourceException: Can't find resource for bundle 
'java.util.PropertyResourceBundle', key 'meta'
at java.util.ResourceBundle.missingResourceException(ResourceBundle.java:237)
at java.util.ResourceBundle.getObject(ResourceBundle.java:440)
at java.util.ResourceBundle.getString(ResourceBundle.java:456)
at net.measurementlab.ndt.NdtTests.dottcp(NdtTests.java:1026)
at net.measurementlab.ndt.NdtTests.run(NdtTests.java:179)
at java.lang.Thread.run(Thread.java:856)

Original issue reported on code.google.com by mlab.tea...@gmail.com on 7 Nov 2012 at 5:24

Attachments:

GoogleCodeExporter commented 9 years ago 
Line numbers refer to patch line numbers rather than code line numbers.

46/47: Don't reverse the check here. Having the constant first is a simple way 
to protect against the = vs == typo.

86: worth logging when intent is null? Actually, it might be better to just 
return if intent == null and not even try to set any text views.

266: indent changed
273: indent changed
282: indent changed
289: indent changed
298: indent
305: indent
314: indent
321: indent
330: indent
337: indent
346: indent
353: indent
362: indent
378: indent
385: indent
etc etc etc :)

Original comment by domi...@google.com on 7 Nov 2012 at 5:43

GoogleCodeExporter commented 9 years ago 
Thank you. I've attached a new patch that addresses your of your comments.

Original comment by mlab.tea...@gmail.com on 7 Nov 2012 at 6:39

Attachments:

GoogleCodeExporter commented 9 years ago 
Still some indentation issues, but there was a fair amount of inconsistent 
indentation prior to the patch. If there is agreement on style for this code, 
I'd be happy to do a human linting pass and clean up the indentation in a 
future patch.

Original comment by awar...@google.com on 7 Nov 2012 at 8:38

GoogleCodeExporter commented 9 years ago 
Fixed in https://code.google.com/p/ndt/source/detail?r=793

Original comment by dominic@measurementlab.net on 21 Dec 2012 at 6:27