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

Remove silence in podcast episodes #2961

Merged
merged 4 commits into from Jan 15, 2019

Conversation

@HaBaLeS
Copy link
Contributor

commented Jan 5, 2019

Closes #2253

Updated ExoPlayer to latest Release
Added Switch to Settings -> Playback to enabled skipping of Silence. (Only if ExoPlayer is selected as MediaPlayer)

Did a small Refactoring so that the SetSpeed is now SetPlaybackParameters so both parameters Speed and SkipSilence can be transported.

HaBaLeS added 2 commits Jan 5, 2019
upstream
In order to support the feature to skip silence audio ExoPlayer must be
updated. Lateste avaiable Version is 2.9.3
@HaBaLeS HaBaLeS force-pushed the HaBaLeS:AP2253 branch from 59947ec to 7dd8e26 Jan 5, 2019
@orionlee

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2019

FYI, it'd be helpful if you add " Closes #2253 " in pull request description so that the associated issue can be closed automatically.

@ByteHamster

This comment has been minimized.

Copy link
Member

commented Jan 5, 2019

I think this setting should be in the audio settings dialog. The main settings screen is already so long.

(by the way, down mixing to stereo and volume control do not work with ExoPlayer)

@HaBaLeS

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2019

I will have a look at the Audio Settings Dialog, but as it is a ExoPlayer exclusive Feature users have to work with the settings anyway

@ByteHamster

This comment has been minimized.

Copy link
Member

commented Jan 5, 2019

We plan to remove the other players as soon as Exoplayer has all features of the other players.

@HaBaLeS

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2019

Added the control also to the audio_controls.xml and wired it up. I did a quick check for Downmix and L/R control of audio in ExoPlayer, it does not seem to be supported. At least not on public API right away. I would recommend to remove all other players asap, as all the abstraction layers add a lot of unnecessary complexity. If that means dropping some features, that's fine.
@ByteHamster

This comment has been minimized.

Copy link
Member

commented Jan 5, 2019

Added the control also to the audio_controls.xml and wired it up.

Nice, thanks! I suggest to remove the preference from the preferences screen. The other options like stereo downmix and playback speed also do not have a second place where they can be toggled.

If that means dropping some features, that's fine.

That's fine for me too, but I am quite sure that removing the features will make many users angry :P

@HaBaLeS HaBaLeS force-pushed the HaBaLeS:AP2253 branch from 0b8bb0d to 84766e8 Jan 5, 2019
@HaBaLeS

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2019

Reverted changes in preferences screen

@ByteHamster ByteHamster changed the title Remove silence in podcast episodes #2253 Remove silence in podcast episodes Jan 5, 2019
@ByteHamster

This comment has been minimized.

Copy link
Member

commented Jan 5, 2019

Thanks a lot, that was fast 👍

Please correct the code style so it follows the rest of the project (it's mainly about spaces, also in the xml and when defining methods)

if (!UserPreferences.useExoplayer()) {
  ^                                 ^
HaBaLeS added 2 commits Jan 5, 2019
SetSpeed was change to SetPlaybackParams which contain speed and a switch to skip silence. For Players that fo not support this the call is ignored or only SetSpeed is used. It is only working if ExoPlayeris used

Default is OFF
Add new Checkbox to dialog. Enable only if ExoPlayer is selected.

Selection directly changes player behavior
@HaBaLeS HaBaLeS force-pushed the HaBaLeS:AP2253 branch from 84766e8 to 73c8635 Jan 5, 2019
@HaBaLeS

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2019

Did another Update.

Maybe you want to include a CodeStyleGuide in the project or use a fixed style in AndroidStudio like this https://github.com/grandcentrix/AndroidCodeStyle -- maybe it is just me, but AntennaPod does not really have a visible CodeStyle thw whole project is following. If you e.g. look at if( and if ( :

{23:04}~/projects/open_source/AntennaPod:AP2253 ✗ ➭ ag -Q "if (" *  | wc -l
2740
{23:04}~/projects/open_source/AntennaPod:AP2253 ✗ ➭ ag -Q "if(" * | wc -l 
458

i only see a trend, but not a common code-style.

@ByteHamster

This comment has been minimized.

Copy link
Member

commented Jan 5, 2019

Yeah, you are right that it's not really consistent, unfortunately. The default Android Studio style uses spaces and that's what I want to (slowly) enforce throughout the project.

@ByteHamster

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

Thanks!

@ByteHamster ByteHamster merged commit 3cad182 into AntennaPod:develop Jan 15, 2019
1 check passed
1 check passed
ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.