Skip to content

Creating an event for network errors#28

Merged
juniorxsound merged 5 commits intomasterfrom
uploading-failure-handling
Aug 13, 2018
Merged

Creating an event for network errors#28
juniorxsound merged 5 commits intomasterfrom
uploading-failure-handling

Conversation

@juniorxsound
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👌 0fa27ea

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

public delegate void UploadAction(string status, float progress);
public event UploadAction OnUploadProgress;

public delegate void RequestAction(string response);
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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