-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added error handling in Spreadsheet component #3324
base: master
Are you sure you want to change the base?
Conversation
Can one of the admins verify this patch? |
try { | ||
Log.d(LOG_TAG, errorMessage); | ||
activity.runOnUiThread(new Runnable() { | ||
@Override | ||
public void run() { | ||
try { | ||
if (!EventDispatcher.dispatchEvent(thisInstance, "ErrorOccurred", errorMessage)) { | ||
// Dispatch to screen if the event handler does not exist. | ||
form.dispatchErrorOccurredEvent(Spreadsheet.this, "ErrorOccurred", | ||
ErrorMessages.ERROR_SPREADSHEET_ERROR, errorMessage); | ||
} | ||
} catch (Exception e) { | ||
Log.e(LOG_TAG, "Error in ErrorOccurred event dispatch", e); | ||
} | ||
} | ||
} | ||
}); | ||
}); | ||
} catch (Exception e) { | ||
Log.e(LOG_TAG, "Fatal error in ErrorOccurred handler", e); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exception(s) are you trying to protect against here? runOnUiThread
doesn't document any thrown exceptions (runtime or otherwise) so there doesn't seem to be any value in the outer try-catch. Do you have a specific project file that would trigger an exception here?
@@ -2091,4 +2127,4 @@ private static Object sanitizeObject(Object o) { | |||
return o.toString(); | |||
} | |||
} | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please restore the newline at the end of the file.
@@ -1491,9 +1500,18 @@ public void run() { | |||
} | |||
}); | |||
} | |||
catch (Exception e) { | |||
catch (GoogleJsonResponseException e) { | |||
e.printStackTrace(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As part of this change, I would recommend we move to using the standard Android Log class rather than using e.printStackTrace()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Tonyhrule This block of changed code is still using e.printStackTrace().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, now I have replaced all instances of e.printStackTrace()
with the Android Log class.
Thank you for the comments! I have removed the unnecessary outer try-catch block around |
Spreadsheet Component Crash on Google Sheets
General items:
ant tests
passes on my machine (skipped as per maintainer's request)If your code changes how something works on the device (i.e., it affects the companion):
ucr
ucr
as the baseFurther, if you've changed the blocks language or another user-facing designer/blocks API (added a SimpleProperty, etc.):
For all other changes:
master
master
as the baseWhat does this PR accomplish?
This PR attempts to fix a bug in the App Inventor Spreadsheets component where pushing data to Google Sheets would crash the entire app. The PR adds error handling in all write operations.
Changes Made
Added exception handling for:
Error messages with categorization:
ErrorOccurred handler: