Skip to content

Conversation

@javedh-dev
Copy link

@javedh-dev javedh-dev commented Dec 25, 2019

Hi all,

I have updated the target SDK version to 29 and a few other dependency versions to its latest.
I have also added a sleep timer similar to Spotify in background player.

Please review the changes and merge.

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 your contribution! The sleep timer is a frequently requested feature. That's why, I'd like to ship it with a good UI and implementation. To achieve this, I'd like to discuss some changes.

Clean UI and UX

Users should be able to select the exact time when to put NP to sleep. This means, there should be no predefined values, but a number picker. When a timer is running and the timer button was pressed, the remaining time should be displayed.
At all: Do you think a minute picker is enough or do we also need to offer a hours/seconds picker?

Stop timer

Please add an option to stop the timer. Every time bomb has a breaker, so should our sleep timer :)
To achieve this, please make sure to only create one timer and modify it when a time is selected or a stop button was pressed. Otherwise we might end up with multiple timers when a user wants to modify the remaining time. This could cause unexpected results e.g. when one tries to extend the remaining time but instead a new timer is created having no affect because the player would be stopped earlier by the old timer.

Clean code

Make all used strings translatable using the strings.xml file. Please ensure that your changes run on all supported Android versions. KitKat (Android 4.4) handles vector drawables differently than later versions. Using them can result in a crash on those devices,

Caution when updating dependencies, Gradle etc.

Please update dependencies in a separate PR when they do not directly affect your changes. Dependency updates need to be tested and verified cautiously, because they can brake functionality. Moreover, you bumped the target SDK, although there are some problems with level 29 api devices which have not been resolved. The build fails, too, because you did not update the specs for Travis in .travis.yml simultaneously. .

@TobiGr TobiGr added feature request Issue is related to a feature in the app player Issues related to any player (main, popup and background) labels Dec 26, 2019
@javedh-dev
Copy link
Author

Thanks Tobias, really appreciate your quick reply. I'll make sure these changes are implemented and I'll also look into the dependency upgrades. I'll get back to you soon.

@javedh-dev
Copy link
Author

Hi,
Can you please review these new changes and let me know if they are fine.

@javedh-dev javedh-dev requested a review from TobiGr December 28, 2019 08:38
@TobiGr TobiGr added this to the 0.18.2 milestone Jan 13, 2020
@TobiGr TobiGr changed the title Updated some dependencies and Sleep timer. Sleep timer Jan 22, 2020
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.

I am sorry for the delay.

@javedh-dev javedh-dev requested a review from TobiGr February 2, 2020 10:15
@javedh-dev
Copy link
Author

Hi @TobiGr ,
Thanks for replying and reviewing. I have submitted the requested changes. Can you please verify once.
Thanks

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.

Thanks for the quick reply!

@TobiGr TobiGr requested a review from Stypox February 2, 2020 11:49
@javedh-dev
Copy link
Author

@javed-hussain can you give NewPipe Sleep Timer apk based on latest changes?

Hi @gillhash,

I can provide you the APK with latest changes merged. I have added one more change recently -
Automatic conversion of m4a audio format to mp3 on youtube audio download. That might have increased the size of apk a bit due to adding external library.
Please let me know if that change is fine with you. I am using the same build but haven't tested on all devices but working perfectly fine for me.

Thanks
Javed Hussain

@ShareASmile
Copy link
Collaborator

ShareASmile commented Feb 23, 2020

@javed-hussain thanks for responding.

Yes, that would be great. Please upload the apk. #3043 PR (my fav) has been merged with dev branch today. Would that be included or your apk is built on latest 0.18.4

@javedh-dev
Copy link
Author

@javed-hussain thanks for responding.

Yes, that would be great. Please upload the apk. #3043 PR (my fav) has been merged with dev branch today. Would that be included or your apk is built on latest 0.18.4

I'll merge the build with the latest dev and then generate the apk. So that should be included.

Thanks.

@Stypox
Copy link
Member

Stypox commented Feb 23, 2020

Please remove mp3 conversion from this PR, it is completely unrelated to the sleep timer.
Also, as discussed in #507, we currently don't want to implement conversion (go discuss there if new alternatives to ffmpeg have arisen)

@ShareASmile
Copy link
Collaborator

@javed-hussain
On a rethought: you can give apk you use based on latest 0.18.4 changes. Because debug apk will be more big in size & i can do without "unhook speed fix for the time being.

i m using Samsung J2 Pro Android 6.0.1.
Many Thanks, Love your work ❤

@javedh-dev
Copy link
Author

Please remove mp3 conversion from this PR, it is completely unrelated to the sleep timer.
Also, as discussed in #507, we currently don't want to implement conversion (go discuss there if new alternatives to ffmpeg have arisen)

Sure @Stypox . That changes are not related to this PR. They are for my personal use. I wanted that change so I generated the build with that. I'll remove those changes at the time of merging with main branch

@javedh-dev
Copy link
Author

@javed-hussain
On a rethought: you can give apk you use based on latest 0.18.4 changes. Because debug apk will be more big in size & i can do without "unhook speed fix for the time being.

i m using Samsung J2 Pro Android 6.0.1.
Many Thanks, Love your work ❤
Hi @gillhash,

Thanks for the appreciation. If you want you can download the build from below link and This is a release build not a debug one.
(https://github.com/javed-hussain/NewPipe/releases/tag/v0.18.4)

Thanks
Javed

@ShareASmile
Copy link
Collaborator

@javed-hussain
Given apk is not installing on my Samsung SM-J210F Galaxy J2Pro Android 6.0.1. Api level 23.

@javedh-dev
Copy link
Author

@gillhash did you try uninstalling the previous version.

@ShareASmile
Copy link
Collaborator

Hi @javed-hussain would you Please remove mp3 conversion from this PR?

..as testing phase is starting of PRs in this Milestone.

@javedh-dev
Copy link
Author

Hi @javed-hussain would you Please remove mp3 conversion from this PR?

..as testing phase is starting of PRs in this Milestone.

Sure @gillhash I'll do that.

@TobiGr TobiGr removed this from the 0.19.0 milestone Mar 14, 2020
@gkeegan gkeegan mentioned this pull request Apr 3, 2020
@panther7
Copy link

+1 for sleep timer, thanks

@TobiGr TobiGr mentioned this pull request May 31, 2020
@RickyM7
Copy link
Contributor

RickyM7 commented Dec 7, 2020

Any progress on this feature? This is something really useful and would be amazing if it were added to NewPipe.

@javedh-dev
Copy link
Author

Any progress on this feature? This is something really useful and would be amazing if it were added to NewPipe.

Hi @RickyM7 ,
Got stuck with some other work. Will try to merge with latest changes and update this PR.

@RickyM7
Copy link
Contributor

RickyM7 commented Dec 7, 2020

@javedh-dev Thank you.

@metzger100
Copy link

Any progress on this feature? This is something really useful and would be amazing if it were added to NewPipe.

Hi @RickyM7 ,
Got stuck with some other work. Will try to merge with latest changes and update this PR.

That would be great yes. The Sleep timer is a feature I am still waiting for. Would be awesome if you could add it.

@litetex litetex marked this pull request as draft October 1, 2021 17:49
@litetex
Copy link
Member

litetex commented Oct 1, 2021

Closing this for now:

  • no progress since >9 months
  • extremely old code-base (2 years - >4k commits) it's nearly impossible that all changes now work correctly even after rebasing
  • no GitHub actions build
  • massive merge conflicts
  • massive changes requested

Feel free to reopen it when there is progress again.

@litetex litetex closed this Oct 1, 2021
@tsiflimagas
Copy link
Contributor

This is such a useful feature to have! I hope it'll eventually be picked up again.

@litetex
Copy link
Member

litetex commented Oct 1, 2021

Was closed here: #2830

@litetex litetex mentioned this pull request Oct 1, 2021
@tsiflimagas
Copy link
Contributor

Was closed here: #2830

I checked the app linked there and is a very nice solution, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature request Issue is related to a feature in the app player Issues related to any player (main, popup and background)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants