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

Add Drawer Personalization #4104

Closed
wants to merge 18 commits into from
Closed

Add Drawer Personalization #4104

wants to merge 18 commits into from

Conversation

werererer
Copy link

@werererer werererer commented Aug 9, 2020

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

  • seperate Drawer stuff from MainActivityClass
  • customize Drawer/Panel

Fixes the following issue(s)

Test Apk

newpipeDrawerupdateApk.zip

Agreement

This is my first PR that had android studio involved. It incoporates a feature to Configure the left Panel/Drawer. This can be done under settings/Content/Content of main page.

I also seperated Drawerlogic from the main Activity class. The Drawerlogic is now in the DrawerFragment class.

@werererer werererer mentioned this pull request Aug 9, 2020
@opusforlife2 opusforlife2 added GUI Issue is related to the graphical user interface feature request Issue is related to a feature in the app labels Aug 9, 2020
@opusforlife2
Copy link
Collaborator

Maybe it should be under Appearance? Worth discussion, I think.

@werererer
Copy link
Author

I think so since changing the drawer changes the Appearance

@TobiGr
Copy link
Contributor

TobiGr commented Aug 9, 2020

Thank you for your contribution @werererer!
Please update Gradle in a separate PR.
PS: you can tick checkboxes by using - [x] (no spaces between the brackets and the x)

@opusforlife2
Copy link
Collaborator

PS: you can tick checkboxes by using - [x] (no spaces between the brackets and the x)

Or, far easier, you can just tick the boxes by clicking on them after you've already created the post.

@werererer
Copy link
Author

What do you mean with Gradle in a separate PR @TobiGr.
Sorry I am not really a Java main so I don't know what you meant with "Please update Gradle in a separate PR".
PS: thanks for the tipps :D

Copy link
Contributor

@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.

Undo these changes

build.gradle Outdated Show resolved Hide resolved
gradle/wrapper/gradle-wrapper.properties Outdated Show resolved Hide resolved
@opusforlife2
Copy link
Collaborator

opusforlife2 commented Aug 9, 2020

Since you used Android Studio, you must have gotten that update prompt "Update Gradle Plugin". There is some stupid setting that Google introduced which automatically screws with your current projects, namely Newpipe. When you get that prompt, just close it. Or, now that you have updated already, just undo that change from the build.gradle file as wb9688 said.

@werererer
Copy link
Author

Ok

@werererer
Copy link
Author

Ok fixed it

@werererer
Copy link
Author

werererer commented Aug 11, 2020

Some tips for reviewing (as i didn't split up the pull request sry. After some research I know now that it should be done :/):

  • a lot of functions moved from Mainactivity to Drawefragment
  • thats why i delete that much there
  • AddSectionDialog, ChooseSectionsFragment, Section, SectionsJsonHelper and SectionsManager have the exact same task as the Tab counterpart.
  • except that they are for the drawer
  • I copied a lot of the code from other classes infact and altert it then according to the need
  • we may even let the sections and tabs classes extend the same interface/class (as they are so similar)

@Stypox Stypox added the bounty Whoever solves this gets a bounty: https://app.bountysource.com/teams/newpipe label Aug 12, 2020
@Stypox Stypox linked an issue Aug 12, 2020 that may be closed by this pull request
@MD77MD
Copy link

MD77MD commented Aug 13, 2020

we may even let the sections and tabs classes extend the same interface/class (as they are so similar)

@werererer could you take a look at this:
#3825 Reorder and manage newPipe tabs from home screen 

while your working on this, I wish if customizing can be done directly from drawer, without the need to dig into settings.
from my point of view, newPipe is always expanding and adding new sources for content (YouTube, SoundCloud...). at the same time, adding more personalization options like this PR as well as functionalities like pinning channels & playlists to taps. with time customizing drawer and tabs for each service can become a hassle. each time a user needs to change something, the user is required to dig deep into settings.

In my opinion, the easiest way to solve this is by using pop-up menu (similarly to video pop-up menu). the menu should have all the function needed (remove, add, reorder...etc). similarly, the menu can be activated by long pressing on the item or tab you want to change.

If customization could be done this way, it would make personalization of all the services so much easier. I know this needs more effort from you but it's worth it.

newPipe deserves perfection 💖

@werererer
Copy link
Author

good idea i will work on it later :)

@werererer
Copy link
Author

I looked at it and i realized that this would require way more work than expected. I also don't really have the knowledge to implement this :/(it would require a new drawer and panel that has a recyclerview in it?). As for this PR it may be overkill :/

@wb9688
Copy link
Contributor

wb9688 commented Aug 15, 2020

I think just having it inside the settings is OK

@werererer
Copy link
Author

ok I think everything works now I don't plan to make any greater changes in this PR (if I didn't introduce bugs)

@wb9688
Copy link
Contributor

wb9688 commented Aug 16, 2020

Could you rebase on dev and fix potential Checkstyle issues? I'll take a look at the UI and code later.

@werererer
Copy link
Author

how do i rebase? (sry still relatively new to github)

@werererer
Copy link
Author

nevermind working on it now

@wb9688
Copy link
Contributor

wb9688 commented Aug 16, 2020

Something like:

git remote add TeamNewPipe https://github.com/TeamNewPipe/NewPipe.git
git fetch TeamNewPipe
git rebase --interactive TeamNewPipe/dev

Then close the editor it opens. After that try building it again in Android Studio and you might get some (Checkstyle) errors that you need to fix.

Edit: you might need to do additional stuff in case of merge conflicts though. If it says "Could not apply", you have to run git status to check what files you need to manually merge and when you've manually merged them, git add those files and then git rebase --continue.

@werererer
Copy link
Author

fixed it

@werererer
Copy link
Author

is it correct that almost all variables have to be declared final now?

@wb9688
Copy link
Contributor

wb9688 commented Aug 16, 2020

@werererer: Yes, that's true and you could see on CI that you didn't do that. Also for the future: we prefer rebases over merges.

@werererer
Copy link
Author

Ah sorry I messed up than :/. Fixed the checkstyle now

@wb9688
Copy link
Contributor

wb9688 commented Aug 16, 2020

Oh, no problem. At least CI is green now. I hope I'll be able to look at this PR soon.

@werererer
Copy link
Author

My PR doesn't seem to be reviewed anymore :/. Should I split this PR up into multiple smaller ones so that it is easier to review? I am willing to do it if it raises the chance of getting this feature.

@werererer
Copy link
Author

@litetex Ok just as an info I am currently working on it and I realized that roughly 30% of the code became too outdated. I will have to rewrite them manually so it may take a few days.

@werererer
Copy link
Author

@litetex nevermind was easier than I though lol xD. It works now in my build I will clean now things up and make it ready to pull.

@werererer werererer marked this pull request as ready for review October 5, 2021 00:01
@werererer
Copy link
Author

@litetex ok it's done :)

@litetex litetex self-requested a review October 9, 2021 13:33
Copy link
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

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

Okay I reviewed the PR and found a bunch of issues:

  • There is documentation / comments missing on some key-classes/methods (e.g. https://github.com/TeamNewPipe/NewPipe/pull/4104/files#diff-6e999cc285df1003629c36815181e973e47fd00bb85c98782a0b42d1154d36c9R308 / )
  • Logic is stretched all over the place and not isolated inside classes
    • e.g. DrawerItem (which is a container to hold an item) is not isolated and has logic from UI, persistence and services inside it
  • I'm not happy with the package name, as it contains a "_"
  • There are multiple typos
  • Code is duplicated (e.g. DrawerItem#ITEM_ID...)
  • Logging is missing on certain places / e.printStackTrace()
  • The draweritems for the kiosk are in the settings multiple times?
  • There is unused and commented out code in some places
  • Persistence is done using one JSON string - I don't know if this is a good idea
  • It's possible to add draweritems multiple times - I don't think that that is making sence (e.g. download)
  • There are unused drawer items: BLANK, SETTING and ABOUT
  • Finding the edit option for the menu is a bit un-intuitive, as it's hidden in the content settings

I think we have to rethink if we even want to add this in the first place because the corresponding issue has 4 upvotes in total...

People seem to mainly want a "distraction" or "addiction" free mode which I personally don't understand because simply uninstalling the app or don't using the phone is a way better approach.

@werererer
Copy link
Author

@litetex Thanks for the review.

with most of your issues I agree and they are mostly a result of hastly written code..

Anyways I want to point out that my drawer_items code is mostly based on tab*.java code. I didn’t want to make another abstraction around it to avoid merge conflicts. So issues mentioned in the DrawerItem class may also appear in the Tab classes already existing in the code base and I dont know if I am able to resolve this issue quickly.

The reason drawer items exist multiple times in the config is because the number of kiosks is not defined and the default kiosks will be replaced with the given kiosks in order excluding all kiosks that the user specifically added himself(e.g. youtube trending). I didnt find a better way to configure it without having to write my own fragment for it.

That you can add downloads specifically multiple times is a bug I didnt found...

where would you put the configuration instead of putting it under content? I just put it near the configuration for the tabs.

To your last point about addiction. I think this app has great elements to cope with addiciton with youtube. You can toggle comments and recommendations. Stop watching youtube cold turkey is pretty hard/nearly impossible. Watching videos on newpipe though is an easier alternative as you can still watch certain youtubers that you really want to follow without getting distracted by unimportant content because of said features. (source personal experience)
So a year ago I thought about a feature to toggle kiosks as they also distracted me and tried to think of a more general feature.
Then I came up with configurable drawers...

Whether we still want to have this feature depends though It is not easy to maintain this code since I lost interest for it myself...

Anyways If we still want to have this feature I will fix mentioned issues.

@litetex litetex mentioned this pull request Oct 10, 2021
2 tasks
@litetex
Copy link
Member

litetex commented Dec 29, 2021

I did some further discussion with our dev team (again) and we came to the conclusion, that we don't want to add this feature as it's a) only relevant to few people (marked the issue as niche) and b) that the code - in the current form - it's not maintainable by us (would probably introduce new problems, etc).

TL;DR
The cost-benefit factor is too low.

For now I will close the PR due to the above points. Anyway, thank you for the PR and everyone else for the work that went into this.

@litetex litetex closed this Dec 29, 2021
@werererer
Copy link
Author

ok sounds reasonable

@a-kbd
Copy link

a-kbd commented Mar 18, 2022

What a shame, thank you for the hard work though @werererer, I wish it would have become a reality

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bounty Whoever solves this gets a bounty: https://app.bountysource.com/teams/newpipe feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

personalize sidebar
9 participants