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

Use AndroidX Media compat in AudioReactor #5065

Merged
merged 1 commit into from
Dec 31, 2020
Merged

Use AndroidX Media compat in AudioReactor #5065

merged 1 commit into from
Dec 31, 2020

Conversation

TacoTheDank
Copy link
Member

@TacoTheDank TacoTheDank commented Dec 2, 2020

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

  • Updates AndroidX Media from a transitive version of 1.0.1 to a declared version of 1.2.1 (changelog)
  • Simplifies some methods, as the compat classes use the appropriate code automatically

APK testing

app-debug-androidx-media-audioreactor.zip

Due diligence

@Redirion
Copy link
Member

Redirion commented Dec 3, 2020

you can update to 1.2.1 :)

@TacoTheDank
Copy link
Member Author

TacoTheDank commented Dec 4, 2020

@Redirion 👍

Edit: I updated it and the change is visible on the branch, but it's not updating on the PR? Is GitHub acting up or something?

@TacoTheDank
Copy link
Member Author

Ok there we go. That was weird 🤔

@triallax
Copy link
Contributor

triallax commented Dec 7, 2020

Now that I take a look at Gradle dependencies, I noticed that none of the AndroidX dependencies have entries in the licenses page. Is this intentional?

@TacoTheDank
Copy link
Member Author

TacoTheDank commented Dec 7, 2020

@mhmdanas Coincidentally, I made a PR dealing with updating the displayed licenses around the same time as this one, which addresses this issue.

Stypox
Stypox previously approved these changes Dec 15, 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.

Looks good to me, thank you :-D
@Redirion could you also approve, if you think it's ok?

@Redirion
Copy link
Member

    public int getMaxVolume() {
        return audioManager.getStreamMaxVolume(STREAM_TYPE);
    }

Could be replaced with:

    public int getMaxVolume() {
        return AudioManagerCompat.getStreamMaxVolume(audioManager, STREAM_TYPE);
    }

@TacoTheDank
Copy link
Member Author

TacoTheDank commented Dec 16, 2020

@Redirion Completely missed that! Thank you :)

Redirion
Redirion previously approved these changes Dec 17, 2020
Copy link
Member

@Redirion Redirion left a comment

Choose a reason for hiding this comment

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

@Stypox looks ready to merge

@TacoTheDank
Copy link
Member Author

@Stypox Sorry for another mention, but idk if this was missed (repo activity looks mighty hectic right now). This should be all good to merge :)

Redirion
Redirion previously approved these changes Dec 30, 2020
@Redirion
Copy link
Member

I would like to merge this, but due to the "2 vulnerabilities" it isn't possible. @theScrabi can you check why this is blocking? I don't have access to snyk.

@TacoTheDank
Copy link
Member Author

TacoTheDank commented Dec 30, 2020

Some of the other PRs from around the time this one was made have also faced the same issue 🤔

@opusforlife2
Copy link
Collaborator

but due to the "2 vulnerabilities" it isn't possible

That used to be the case earlier. Now you can see that it doesn't have the "Required" label, which build-and-test does. So the Snyk check isn't blocking the merge, the build-and-test Action is.

Why it isn't either running or finished is a mystery. The Checks tab should show a job, and there are currently none.

@TacoTheDank
Copy link
Member Author

TacoTheDank commented Dec 30, 2020

@opusforlife2 Do you think it would regenerate the checks if I repushed the commit? 🤔 I'll give it a try.

@TacoTheDank
Copy link
Member Author

@opusforlife2 Do you think it would regenerate the checks if I repushed the commit? 🤔 I'll give it a try.

It worked :)

@Redirion Redirion merged commit 8193a0d into TeamNewPipe:dev Dec 31, 2020
@TacoTheDank TacoTheDank deleted the androidx-media-audioreactor branch January 1, 2021 00:24
@opusforlife2
Copy link
Collaborator

Yay!

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.

5 participants