Skip to content

Add playlist tab to main page #3506

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.

Sign up for GitHub

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

Jump to bottom
Merged
Stypox merged 4 commits into from
Jul 5, 2020
Merged

Add playlist tab to main page #3506

Stypox merged 4 commits into from
Jul 5, 2020

Conversation

@Royosef
Copy link
Contributor

@Royosef Royosef commented Apr 30, 2020 •
edited by Stypox
Loading

What is it?

  • Bug fix (user facing)
  • Feature (user facing)
  • Code base improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

Add bookmarked playlist as tab in the main page (by Settings > Content > Content of main page)

Fixes the following issue(s)

Testing apk

NewPipe_PlaylistTab-debug.zip

Agreement

wb9688
wb9688 requested changes May 1, 2020
View reviewed changes
Copy link
Member

@wb9688 wb9688 left a comment

Choose a reason for hiding this comment

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

I did a quick code review, so I didn't really look in detail. There are also some more variables that could be final. I think you should use the bookmark icon though.

@wb9688 wb9688 added this to the 0.19.4 milestone May 1, 2020
Stypox
Stypox requested changes May 4, 2020
View reviewed changes
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.

Thank you for the contribution :-D

Tip: in the "Fixes the following issues" section you should put issues with format fixes #1234, so that GitHub will automatically close that issue upon merge. Also, #1234 is enough, there is no need for the whole url ;-)

I saw the Tab file contains many magic variables that should be replaced by constants (i.e. -1, , ), but let's leave this for another code-improvement PR.

@Stypox Stypox self-assigned this May 4, 2020
@Royosef
Copy link
Contributor Author

Royosef commented May 8, 2020 •
edited
Loading

Thank you for the contribution :-D

Tip: in the "Fixes the following issues" section you should put issues with format fixes #1234, so that GitHub will automatically close that issue upon merge. Also, #1234 is enough, there is no need for the whole url ;-)

I saw the Tab file contains many magic variables that should be replaced by constants (i.e. -1, , ), but let's leave this for another code-improvement PR.

Cool, thanks!

@Royosef Royosef closed this May 8, 2020
@Royosef Royosef reopened this May 8, 2020
@wb9688 wb9688 added the feature request Issue is related to a feature in the app label May 9, 2020
This was linked to issues May 9, 2020
Content of main page - cannot choose dynamic playlists #786
Closed
Enhancement : tabs setting : display a playlist as a tab directly #1781
Closed
@Stypox Stypox modified the milestones: 0.19.4, 0.19.5 May 28, 2020
@wb9688
Copy link
Member

wb9688 commented Jun 28, 2020

I just rebased this.

@wb9688 wb9688 requested a review from Stypox June 28, 2020 13:22
wb9688
wb9688 requested changes Jun 28, 2020
View reviewed changes
Stypox
Stypox requested changes Jun 28, 2020
View reviewed changes
Stypox
Stypox requested changes Jun 28, 2020
View reviewed changes
Royosef and others added 4 commits July 4, 2020 11:35
@Royosef @Stypox
Add playlist tab to main page
dfc27b2 
Add bookmarked playlist as tab in the main page (by Settings > Content > Content of main page)
@Royosef @Stypox
Final declarations, naming & redundant code
13c0fde 
* add final declarations where missing
* fix typo "onSelectedLisener" to "onSelectedListener"
* rename "baseEqual" to "baseEquals"
* replace getPlaylistsObserver code with functions pointers
* remove duplicate code in constructors
* remove useless null checks
@Stypox
Fix style issues
2482125
@Stypox
Move local/remote playlist merge() to PlaylistLocalItem class
8cc2192 
In order not to have a utils class just for one function
@Stypox
Copy link
Member

Stypox commented Jul 4, 2020 •
edited
Loading

I rebased, squashed two review-fix commits and changed commit names to a better format. I moved merge() to PlaylistLocalItem and marked both your review comments as resolved @wb9688
Test apk: app-debug.zip

@Stypox Stypox requested a review from wb9688 July 4, 2020 09:47
@Stypox Stypox changed the title feat(playlist-tab) Add playlist tab to main page Jul 4, 2020
@wb9688 wb9688 requested a review from Stypox July 5, 2020 12:32
TobiGr
TobiGr approved these changes Jul 5, 2020
View reviewed changes
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.

@Stypox Feel free to merge once you finished your review.

@Stypox Stypox merged commit 248e2d7 into TeamNewPipe:dev Jul 5, 2020
This was referenced Jul 8, 2020
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TobiGr TobiGr TobiGr approved these changes

@Stypox Stypox Stypox approved these changes

@wb9688 wb9688 wb9688 approved these changes

Assignees

@Stypox Stypox

Labels

feature request Issue is related to a feature in the app

Projects

None yet

Milestone

0.19.6

Development

Successfully merging this pull request may close these issues.

Specific bookmarked tab Enhancement : tabs setting : display a playlist as a tab directly Content of main page - cannot choose dynamic playlists

4 participants

@Royosef @wb9688 @Stypox @TobiGr