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.

Sign up for GitHub

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

Jump to bottom

Add an Open in browser button on error panel #9180

Merged
merged 6 commits into from
Oct 27, 2022
Merged

Add an Open in browser button on error panel #9180

merged 6 commits into from
Oct 27, 2022

Conversation

YonghaoDeng
Copy link
Contributor

@YonghaoDeng YonghaoDeng commented Oct 23, 2022 •
edited by Stypox
Loading

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

With the addition of the open_in_browser button design, the The user can watch the video in case the new pipe itself is faulty.

Before/After Screenshots/Screen Record

Before After
Before After

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

AudricV
AudricV previously requested changes Oct 23, 2022
View reviewed changes
Copy link
Member

@AudricV AudricV left a comment

Choose a reason for hiding this comment

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

Thank you for this improvement! Here are changes that you need to apply in order to have this pull request merged.

app/src/main/java/org/schabi/newpipe/error/ErrorInfo.kt Outdated Show resolved Hide resolved
app/src/main/java/org/schabi/newpipe/error/ErrorPanelHelper.kt Outdated Show resolved Hide resolved
app/src/main/java/org/schabi/newpipe/error/ErrorPanelHelper.kt Outdated Show resolved Hide resolved
app/src/main/java/org/schabi/newpipe/error/ErrorPanelHelper.kt Outdated Show resolved Hide resolved
app/src/main/res/layout/error_panel.xml Outdated Show resolved Hide resolved
app/src/main/java/org/schabi/newpipe/error/ErrorPanelHelper.kt Outdated Show resolved Hide resolved
app/src/main/java/org/schabi/newpipe/error/ErrorPanelHelper.kt Outdated Show resolved Hide resolved
app/src/main/res/layout/error_panel.xml Show resolved Hide resolved
app/src/main/res/layout/error_panel.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@AudricV AudricV added feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface labels Oct 23, 2022
@AudricV AudricV changed the title add a open in browser button Add an Open in browser button on error panel Oct 23, 2022
@YonghaoDeng
Copy link
Contributor Author

I have modified the code according to your reply, you can review them now @AudricV

@opusforlife2
Copy link
Collaborator

What will the button do, BTW? Will it directly open the default browser selected in the OS? Or show a list of apps that can handle the link type?

@YonghaoDeng
Copy link
Contributor Author

it will directly open the default browser selected in the OS

@opusforlife2
Copy link
Collaborator

I think it would be better to show the list of apps. The user may want to open it in an app other than the default browser, like a media player.

@YonghaoDeng
Copy link
Contributor Author

But I think the video needs to be viewed online at the YouTube website and the requirement is to create an open in browser button, so I think it should be redirected to the browser, don't you think so?

@opusforlife2
Copy link
Collaborator

It's perfectly fine to open it in the default browser. Nothing wrong with that behaviour.

I'm just providing a suggestion for extra functionality. If a user wants to open it in a different browser, or in a media app, they should be able to. They can always select "Always open" so that their selected choice becomes the default way to open.

@YonghaoDeng
Copy link
Contributor Author

show the list of apps

Hmmmmm I see what you mean but I'm afraid I won't be able to do well with this extra feature. As I am an Anu student, this is an example of PR practice in one of my classes and the deadline for this assignment is this weekend

@opusforlife2
Copy link
Collaborator

No problems at all. 👍

Stypox
Stypox requested changes Oct 25, 2022
View reviewed changes
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Thank you! Looks almost good

settings.gradle Outdated Show resolved Hide resolved
app/src/main/res/layout/error_panel.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/error_panel.xml Outdated Show resolved Hide resolved
@AudricV AudricV removed their request for review October 25, 2022 19:13
@YonghaoDeng
Copy link
Contributor Author

I added the function as you said @Stypox

Stypox
Stypox requested changes Oct 26, 2022
View reviewed changes
Copy link
Member

@Stypox Stypox left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just a few more little changes

app/src/main/res/layout/error_panel.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/error_panel.xml Show resolved Hide resolved
app/src/main/java/org/schabi/newpipe/error/ErrorPanelHelper.kt Outdated Show resolved Hide resolved
@YonghaoDeng
Copy link
Contributor Author

OK，I modified the code as you said @Stypox

Stypox
Stypox approved these changes Oct 27, 2022
View reviewed changes
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you!

@Stypox Stypox merged commit a22162f into TeamNewPipe:dev Oct 27, 2022
This was referenced Nov 4, 2022
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AudricV AudricV AudricV left review comments

@Stypox Stypox Stypox approved these changes

Assignees
No one assigned
Labels
feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a 'Open in Browser' button to the broken video page
4 participants
@YonghaoDeng @opusforlife2 @Stypox @AudricV