Skip to content
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

Creating an event for network errors #28

Merged
merged 5 commits into from
Aug 13, 2018

Conversation

juniorxsound
Copy link
Contributor

The VimeoAPI now implements OnNetworkError event which listens to request.isNetworkError and prints an error to the console when a network error prevents you from publishing to Vimeo

Tested on macOS / Unity v2018.1.6f1

@juniorxsound juniorxsound requested a review from caseypugh August 8, 2018 19:33
private void NetworkError(string response)
{
if (OnUploadFail != null) {
OnUploadFail("It seems like you are not connected to the internet, or having connection problems which disables the uploading of the video.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message is technically not accurate. Lots of vimeo api calls are in the Publisher class. Doesnt mean it failed while uploading.

Maybe simply write:

It seems like you are not connected to the internet or are having connection problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌 0fa27ea

for (int i = 0; i < json["invalid_parameters"].Count; i++) {
// TODO use .Value
if (json["invalid_parameters"][i]["field"].ToString() == "\"privacy.download\"") {
Debug.LogError("You must upgrade your Vimeo account in order to disable downloads on your video. https://vimeo.com/upgrade");
if (OnUploadFail != null) {
OnUploadFail("You must upgrade your Vimeo account in order to access this privacy feature. https://vimeo.com/upgrade");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If OnUploadFail is not implemented, then the user will never see these messages, right? Need a way to log these out regardless of the event. I see you are logging errors in VimeoRecorder.cs but what about VimeoPlayer.cs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would make sense to have a virtual method in the VimeoAPI class that both the publisher and player can override but it will handle the logging for everyone, in the meantime I am just straight up logging it in the VimeoPlayer -> 0fa27ea

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that sounds good

@@ -12,6 +12,9 @@ public class VimeoPublisher : MonoBehaviour
public delegate void UploadAction(string status, float progress);
public event UploadAction OnUploadProgress;

public delegate void RequestAction(string response);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe name this error_message instead of response. response implies some sort of api json response

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#29

private void NetworkError(string error_message)
{
if (OnUploadFail != null) {
OnUploadFail("It seems like you are not connected to the internet or are having connection problems.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename this to OnNetworkError or better yet switch to the event type delegation (#29)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in this commit 056dc50

else {
JSONNode json = JSON.Parse(request.downloadHandler.text);
Debug.LogError("[VimeoApi] " + request.responseCode + " " + json["error"]);
if (OnError != null && OnNetworkError != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decouple this so that you can bind into either event if you want.

Copy link
Contributor Author

@juniorxsound juniorxsound Aug 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in this commit 056dc50

@juniorxsound juniorxsound merged commit f242628 into master Aug 13, 2018
@juniorxsound juniorxsound deleted the uploading-failure-handling branch August 13, 2018 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants