-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Update Extractor and add migration to remove SoundCloud Top 50 kiosk #12438
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
Conversation
93818ba
to
3c50f42
Compare
LGTM, however: |
3c50f42
to
128ead8
Compare
128ead8
to
95f2ab1
Compare
Yes, there are some kind of safe-guards, but they do not work as expected. I fixed that in b42f3ce. |
95f2ab1
to
b42f3ce
Compare
} catch (final ExtractionException e) { | ||
} catch (final Exception e) { | ||
ErrorUtil.showUiErrorSnackbar(context, "Getting fragment item", e); | ||
// TODO: show an error fragment instead |
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.
Not sure if this is ready yet (PR says ready for review and is no draft so I assume yes)
But this should be addressed before merging ;)
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 that this can be done later / on the refactor branch. I don't have time for that now, but if you want to implement this, fell free to do so ;)
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.
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 that this can be done later / on the refactor branch. I don't have time for that now, but if you want to implement this, fell free to do so ;)
Code is not an issue tracker, please open a issue for this ;)
Update: Nevermind Stypox left a comment 500ms before I submitted mine lol
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.
The message change approach may be generic, as we will have the same behavior with YouTube's changes.
Could we also make the dialog not dismissable by clicking outside but only on the OK button? As we show it only once, it could be missed by users who tapped by mistake. This is a bit of dark pattern UX though.
app/src/main/java/org/schabi/newpipe/settings/SettingMigrations.java
Outdated
Show resolved
Hide resolved
Do not crash if something unexpected happens.
b42f3ce
to
fe58ec8
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.
I implemented the error panel in BlankFragment
. The rest LGTM. Thanks!
What is it?
Description of the changes in your PR
This updates the extractor to the latest version. The
Top 50
kiosk has been discontinued by SoundCloud and was removed from the extractor in TeamNewPipe/NewPipeExtractor#1332. This PR adds a migration to remove the corresponding main page tab in case it was present. This PR also fixes the error handling when loading tabs to ensure the application does not crash in case an unexpected error occurs.Screenshots
Fixes the following issue(s)
Without this migration we'd get a crash at startup rendering the app unusable.
Stacktrace
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. You can find more info and a video demonstration on this wiki page.
Due diligence