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

Added Native Player Gesture for Volume & Brightness #133

Merged
merged 1 commit into from
Sep 27, 2020
Merged

Added Native Player Gesture for Volume & Brightness #133

merged 1 commit into from
Sep 27, 2020

Conversation

yashx
Copy link
Contributor

@yashx yashx commented Sep 18, 2020

closes #13

How it looks

gesture2

@nielsvanvelzen
Copy link
Member

Can we add an option to disable this behavior?

if(counter == -1f){
counter = windowLayoutParams.screenBrightness
if(counter < 0f)
counter = android.provider.Settings.System.getFloat(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Maxr1998 @nielsvanvelzen This line has a few problems. According to the docs, it should give value between 0 and 255 but on my phone (which has MIUI) it is goes over 255.
Worst case scenario, it jumps to full brightness first time the gesture is used.
I can disable it but imo the utility it offers is worth dealing with the bug.

@yashx
Copy link
Contributor Author

yashx commented Sep 18, 2020

Can we add an option to disable this behavior?

Should I do this in this PR or send another one?

@dkanada
Copy link
Member

dkanada commented Sep 18, 2020

Within this one should be fine.

@yashx
Copy link
Contributor Author

yashx commented Sep 18, 2020

@nielsvanvelzen Added option to disable it in "Client Settings"

@Maxr1998 Maxr1998 added enhancement New feature or request exoplayer Related to the ExoPlayer integration labels Sep 19, 2020
Copy link
Member

@Maxr1998 Maxr1998 left a comment

Choose a reason for hiding this comment

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

Code looks mostly good, I have a few comments about naming and formatting stuff however. I'd generally use a shorter name for the gesture feature.
Also, I'm not a fan of that counter attribute.. I'm not sure how you could do it differently though, I think I'll have to play with the code myself once the other mentioned issues are fixed.

app/src/main/java/org/jellyfin/mobile/AppPreferences.kt Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/mobile/AppPreferences.kt Outdated Show resolved Hide resolved
app/src/main/res/drawable/ic_brightness_white_84dp.xml Outdated Show resolved Hide resolved
app/src/main/res/drawable/ic_volume_white_84dp.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/activity_player.xml Outdated Show resolved Hide resolved
@yashx
Copy link
Contributor Author

yashx commented Sep 21, 2020

@Maxr1998 While brightness gesture can be implemented without the counter. The problem is with the volume gesture. I can also set volume by using the distance between e1 and e2 in onScroll but I would need to store the initial volume level when the gesture was started.
So I decided to go with the counter approach so that all gestures (brightness, volume and horizontal seek(future PR)) can have a similar implementation.

@Maxr1998
Copy link
Member

I see, then there's probably no way around that. Could you still make the counter variable a bit more descriptive then, and add some documentation/comments on how you're using it?

@yashx
Copy link
Contributor Author

yashx commented Sep 21, 2020

Yes I will do that and all the requested changes in a few hours.

@yashx yashx requested a review from Maxr1998 September 21, 2020 04:26
@Maxr1998
Copy link
Member

@yashx did some more tweaks and code style improvements, please let me know if it's ok for you, and if you @nielsvanvelzen also don't have any comments anymore, I'll squash and merge it.

@Maxr1998 Maxr1998 merged commit 2f52b9d into jellyfin:master Sep 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request exoplayer Related to the ExoPlayer integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add gesture controls for volume and brightness
4 participants