-
-
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
Fix download dialog selector layout #7516
Fix download dialog selector layout #7516
Conversation
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.
Code looks on the first impression good.
👍 for the tests
However I would appreciate some (there is currently zero) documentation like JavaDoc or comments.
363c37a
to
065f9df
Compare
Done. |
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
However I'm unable to get a situation where the icon is visible in the first place.
@mauriciocolli
Do you know a situation/video where that's the case?
* @return if there are any video-only streams with no secondary stream associated with them. | ||
* @see #hasVideoOnlyWithNoSecondaryStream | ||
*/ | ||
private boolean checkHasVideoOnlyWithNoSecondaryStream() { |
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 rename that method to checkHasAnyVideoOnlyStreamWithNoSecondaryStream
065f9df
to
99d6238
Compare
Since there is no feedback for over 2-3 months now (seriously guys stop doing fire-and-forget PRs 😾), I fixed it myself: |
Kudos, SonarCloud Quality Gate passed! |
What is it?
Description of the changes in your PR
Some logic was missing in the adapter for making the no-audio icon
GONE
instead ofINVISIBLE
. I fixed it and added some tests as well.That reveals another problem: some icons are not themed properly, I will fix that in another pull request.
Fixes the following issue(s)
Due diligence