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

Silence CheckForNewAppVersionTask #2423

Merged
merged 9 commits into from
Jul 7, 2019
Merged

Silence CheckForNewAppVersionTask #2423

merged 9 commits into from
Jul 7, 2019

Conversation

Redirion
Copy link
Member

@Redirion Redirion commented Jun 25, 2019

Closes #2421
Closes #2197

This is a very simple change to not annoy users when the internet connection isn't stable. The task will run again on another occasion.

Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Thanks! Can you please print the exception to the log when debug is enabled?

@TobiGr
Copy link
Contributor

TobiGr commented Jun 25, 2019

Can you please do the same in line 119 to close #2197 as well?

@Stypox
Copy link
Member

Stypox commented Jun 25, 2019

I think a Toast or a Snackbar should be shown in these cases, just to let the user know. Maybe with Snackbar/Toast.LENGTH_SHORT to make it disappear after a short amount of time.

@Redirion
Copy link
Member Author

like this @Stypox ?

@Stypox
Copy link
Member

Stypox commented Jun 25, 2019

Yes, that's not intrusive

@TobiGr
Copy link
Contributor

TobiGr commented Jun 25, 2019

Good, idea @Stypox. I think in this case it might be better to not show Can not connect to the server, but something like (Connection) error during app update check. Showing Can not connect to the server without any context which is visible to the user might cause irritations.
btw. Shouldn't it be Cannot connect to the server?

@Stypox
Copy link
Member

Stypox commented Jun 25, 2019

Both "can not" and "cannot" are accepted by english, afaik

Connection error during app update check is short and explanatory, +1 👍

@phatloot123
Copy link

phatloot123 commented Jun 26, 2019

In my opinion the user should not be notified about this. This is just an app update check. It probably happens often so if it fails every now and then it's not a big deal. imo it should be hidden from the user since the user is usually not aware of this anyway so when they see an error it will just confuse them since no action on the users side is needed.

edit: for example the user might be reading some comment and this is when his reception changes in this moment the update check fails but the user is busy with reading comments and the reception might be better already when he is done reading, no need to notify him about a failed background update check.

@theScrabi
Copy link
Member

Well we should still get notifications when something failes, however if its a simple SSL issue because the host could not be reached I would not send a notification at all just as you said.

@theScrabi theScrabi added the feature request Issue is related to a feature in the app label Jun 26, 2019
@Redirion
Copy link
Member Author

okay, I have removed the Toast messages again.

Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Thanks

@TobiGr TobiGr merged commit ad79a71 into TeamNewPipe:dev Jul 7, 2019
This was referenced Jul 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issue is related to a feature in the app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

app update API fail due to bad reception Update check crashes on startup
5 participants