-
-
Notifications
You must be signed in to change notification settings - Fork 3.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
Show toast when no updates are available. #8668
Show toast when no updates are available. #8668
Conversation
Wouldn't this show every time the app is launched? That would be pretty annoying, I think. |
I agree with @TacoTheDank, it makes little sense to me to show a notification if there is no update available, plus it would be quite irritating. What problem are you trying to solve by this PR @Isira-Seneviratne? |
The idea was to show some feedback if no update was available, since at the moment nothing is shown if the app is up-to-date. |
I think that would be more suited for a update check button/page perhaps. |
I would not show a notification, but rather a toast. A toast saying "Checking for updates..." is already shown when the "check for updates" button is pressed (settings > updates). |
@TobiGr Yeah, I'll make that change. |
4ea9012
to
b8d95ad
Compare
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.
Looks almost good to me
@@ -32,7 +32,7 @@ private void checkNewVersionNow() { | |||
// Reset the expire time. This is necessary to check for an update immediately. | |||
defaultPreferences.edit() | |||
.putLong(getString(R.string.update_expiry_key), 0).apply(); | |||
NewVersionWorker.enqueueNewVersionCheckingWork(getContext()); | |||
NewVersionWorker.enqueueNewVersionCheckingWork(requireContext(), true); |
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.
I think you should use the MANUAL flag inside NewVersionWorker.checkNewVersion
to determine whether to ignore the expiry time, instead of resetting the expiry time here. Also, since only one statement would be left here then, I would remove checkNewVersionNow()
and inline the work enqueueing.
2d66f2b
to
6ca5b41
Compare
if (isManual) { | ||
PreferenceManager.getDefaultSharedPreferences(context).edit { | ||
putLong(context.getString(R.string.update_expiry_key), 0) | ||
} | ||
} |
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.
This is not what I meant: remove this and just add an if (!manual)
around these lines
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.
Is the change okay?
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.
I get the following error, probably the wrong thread or context are used?
java.lang.NullPointerException: Can't toast on a thread that has not called Looper.prepare()
at ...
at org.schabi.newpipe.NewVersionWorker.compareAppVersionAndShowNotification(NewVersionWorker.kt:49)
@@ -42,26 +44,30 @@ class NewVersionWorker( | |||
versionCode: Int | |||
) { | |||
if (BuildConfig.VERSION_CODE >= versionCode) { | |||
if (inputData.getBoolean(IS_MANUAL, false)) { | |||
// Show toast stating that the app is up-to-date if the update check was manual. | |||
Toast.makeText(applicationContext, R.string.app_update_unavailable_toast, Toast.LENGTH_SHORT).show() |
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.
No lines longer than 100 characters... ktlint seems to be misconfigured, it should detect these problems
Toast.makeText(applicationContext, R.string.app_update_unavailable_toast, Toast.LENGTH_SHORT).show() | |
Toast.makeText(applicationContext, R.string.app_update_unavailable_toast, | |
Toast.LENGTH_SHORT).show() |
@@ -16,25 +16,17 @@ public class UpdateSettingsFragment extends BasePreferenceFragment { | |||
.apply(); | |||
|
|||
if (checkForUpdates) { | |||
checkNewVersionNow(); | |||
NewVersionWorker.enqueueNewVersionCheckingWork(requireContext(), true); |
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.
I would make this false
intead of true
, since the user did not directly ask for an update check, he just enabled them. So if the last check was done a while ago (or never), then it's ok to check again, but if it has been done minutes earlier since the user just disabled and enabled twice in a row, it should not be done. Also, it's better to consider it automatic since we don't want a toast to be shown to the user in this case, as he does not expect feedback I think.
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.
I'd keep this true
because the user enabled the check manually, in other words wants NewPipe to check for new versions. Reporting back, that we did not find any new version this time is ok. It shows that the app does something as requested.
Yeah, it seems like the fix is to use a By the way, how can I check for updates in the debug build? |
7621517
to
012e9ef
Compare
I would assume that debug builds are for debugging, and that update checking isn't needed for them. |
Right, my bad. I'll check with a release build. It's been a while since I made changes to the update process. |
012e9ef
to
f05fd3e
Compare
Kudos, SonarCloud Quality Gate passed! |
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.
LGTM. Thank you
btw. a rebase is required. I just pulled the latest translations from weblate. To the maintainer merging this PR: please make sure to sync weblate once this PR is merged |
f05fd3e
to
99a4e7b
Compare
Kudos, SonarCloud Quality Gate passed! |
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.
Thanks!
99a4e7b
to
e2670ac
Compare
Co-authored-by: Stypox <stypox@pm.me>
e2670ac
to
6b210e1
Compare
I merged weblate into dev, then rebased this PR onto dev, then I'm merging it. |
Kudos, SonarCloud Quality Gate passed! |
What is it?
Description of the changes in your PR
Before/After Screenshots/Screen Record
Fixes the following issue(s)
APK testing
The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.
Due diligence