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

Upgrade some dependencies #3509

Merged
merged 6 commits into from
May 28, 2020
Merged

Upgrade some dependencies #3509

merged 6 commits into from
May 28, 2020

Conversation

wb9688
Copy link
Contributor

@wb9688 wb9688 commented May 1, 2020

What is it?

  • Bug fix (user facing)
  • Feature (user facing)
  • Code base improvement (dev facing)
  • Meta improvement to the project (dev facing)

Agreement

@wb9688 wb9688 marked this pull request as draft May 1, 2020 15:27
@wb9688 wb9688 marked this pull request as ready for review May 1, 2020 16:02
@Stypox
Copy link
Member

Stypox commented May 2, 2020

Have you tested on 4.4?

@wb9688
Copy link
Contributor Author

wb9688 commented May 2, 2020

@Stypox: Not thoroughly, but I was going to.

@wb9688
Copy link
Contributor Author

wb9688 commented May 3, 2020

Reading licenses is broken :/

Edit: I partially fixed that, however it shows nothing on my phone as soon as I add any stylesheet. It currently works on an Android 4.4 AVD though, which was crashing before, but I didn't test it in any other AVDs yet.

Edit 2: the issue on my phone doesn't happen when reverting compile/targetSdkVersion to 28. Ughhh…

Edit 3: it happens in an Android 10 AVD as well, so at least it's not some Samsung bullshit.

Edit 4: it apparently needs to be base64-encoded since Android 10, so I did that now.

@wb9688 wb9688 added this to the 0.19.4 milestone May 8, 2020
Stypox
Stypox previously approved these changes May 9, 2020
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.

The code seems correct to me.
I tested on AVD API 19 and AVD Android 10, I have hopefully gone through many of the processes that involve the library updates and everything seems to work just fine (@TobiGr is there a protocol that can be followed to fully test such app-wide changes?).

The new "Leaks" icon is adorable ;-)

@TobiGr
Copy link
Contributor

TobiGr commented May 9, 2020

is there a protocol that can be followed to fully test such app-wide changes?

@Stypox Test everything on KitKat, Lollipop, latest and a version (maybe Oreo) in between. With so many upgraded dependencies almost everything is somehow affected by this change.

@wb9688
Copy link
Contributor Author

wb9688 commented May 9, 2020

@TobiGr: I think he meant more like what is "everything"? NewPipe has so many features that you could easily forget one.

app/build.gradle Outdated Show resolved Hide resolved
@Stypox
Copy link
Member

Stypox commented May 9, 2020

Btw I also tested on my 7.0 phone and everything works fine

@B0pol
Copy link
Member

B0pol commented May 9, 2020

Tested on 9.0 phone and Android TV 7.1.2 and I haven't found an issue.

@wb9688
Copy link
Contributor Author

wb9688 commented May 9, 2020

I tested it on AVD API 19 and on my phone with Android 10. I'll rebase and do what @Stypox suggested.

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

@TobiGr
Copy link
Contributor

TobiGr commented May 18, 2020

On KitKat main page tabs are not loaded again when new tabs are added through the settings.
E.g. I added the history tab and moved it to the top of the list. Instead of seeing the history when going back, I see trending content.

@wb9688
Copy link
Contributor Author

wb9688 commented May 22, 2020

@TobiGr: I just rebased, but I can't reproduce your bug.

@Stypox
Copy link
Member

Stypox commented May 22, 2020

@TobiGr I think this has to do with #3312 and is unrelated from this PR.

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.

Thank you for searching for a related issue @Stypox!
I could not reproduce the problem today.

@wb9688 changes look good so far. I'd like to do a little more testing before merging.

app/build.gradle Outdated Show resolved Hide resolved
@Stypox Stypox modified the milestones: 0.19.4, 0.19.5 May 28, 2020
@TobiGr TobiGr merged commit 9d25c0b into TeamNewPipe:dev May 28, 2020
This was referenced May 29, 2020
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.

4 participants