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

Show comment replies #9020

Closed
wants to merge 23 commits into from
Closed

Show comment replies #9020

wants to merge 23 commits into from

Conversation

xz-dev
Copy link

@xz-dev xz-dev commented Sep 25, 2022

Moved to #9410

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

Adds a "Show Replies" for the comments with replies for Youtube, just look like youtube.

Before/After Screenshots/Screen Record

Before After1 After2 After3
Before After1 After2 After3
![Screenshot_20221029-135951_NewPipe Debug]()

Fixes the following issue(s)

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.

Due diligence

@xz-dev xz-dev changed the title Show comment replies by bottom sheet dialog Show comment replies using bottom sheet dialog Sep 25, 2022
@krlvm
Copy link
Contributor

krlvm commented Sep 25, 2022

Can you also set android:navigationBarColor to match bottom sheet background color?

@xz-dev
Copy link
Author

xz-dev commented Sep 25, 2022

Can you also set android:navigationBarColor to match bottom sheet background color?

I find something, but I don't know how to fix it.
material-components/material-components-android#267
Maybe we need another professional guy fix it

@AudricV AudricV added feature request Issue is related to a feature in the app multiservice Issues related to multiple services labels Sep 25, 2022
@xz-dev
Copy link
Author

xz-dev commented Sep 25, 2022

I find there are some code need clean, I'll do latter.

Co-authored-by: krlvm <51774833+krlvm@users.noreply.github.com>
@krlvm
Copy link
Contributor

krlvm commented Sep 25, 2022

I don't know if it is possible for me to suggest changes there, I've submitted bottom sheet improvements to @xz-dev fork: xz-dev#1

@krlvm
Copy link
Contributor

krlvm commented Sep 25, 2022

You should not do it.
It is automatically ensured by inheritance.
That's how the current theming strategy works.

@xz-dev
Copy link
Author

xz-dev commented Sep 25, 2022

You should not do it. It is automatically ensured by inheritance. That's how the current theming strategy works.

But it cann't been build, should I use other way to fix it?

@krlvm

This comment was marked as resolved.

@xz-dev
Copy link
Author

xz-dev commented Sep 25, 2022

Sorry, it's been my fault. Please, revert the lastest commit and edit following lines

Line 32:

<item name="bottomSheetDialogTheme">@style/BottomSheetDialogTheme.Light.Base</item>

Line 57:

<item name="bottomSheetDialogTheme">@style/BottomSheetDialogTheme.Dark.Base</item>

Pushed

@xz-dev
Copy link
Author

xz-dev commented Sep 25, 2022

I should go to sleep, and the count support of reply is ready(include NewPipe), hope we can finish it early. :)

@xz-dev
Copy link
Author

xz-dev commented Sep 25, 2022

I find a bug that it don't load all reply, I need finger out how reply page work.

@nicoursi
Copy link

I installed this artifact to test it. It is nice, well done. The only thing is that one has to tap on the "show replies" string and is is a bit unconfortable. Could the clickable area be extended at least orizontally? So that, to get more replies I could tap on the middle or on the right side and not only on the "show reply" text.

@opusforlife2
Copy link
Collaborator

So that, to get more replies I could tap on the middle or on the right side and not only on the "show reply" text.

Rather than that, the Show Replies text could be inlaid in a button with a different colour from the background so that it is clear where to tap.

@nicoursi
Copy link

nicoursi commented Sep 25, 2022

It is already clear as it is blue, but currently the only clickable area is on the left and sometimes is more comfortable on the right or middle. Same principle of expanding the comment text where you can tap anywhere

@opusforlife2
Copy link
Collaborator

When you have a scrollable view, it needs some dead zones where no action gets triggered accidentally. Here, for example, you can safely scroll up and down without opening replies when you might not want to.

@krlvm
Copy link
Contributor

krlvm commented Sep 25, 2022

I would also suggest preventing the bottom sheet from expanding up so that it doesn't overlap the video.

@xz-dev
Copy link
Author

xz-dev commented Sep 26, 2022

I would also suggest preventing the bottom sheet from expanding up so that it doesn't overlap the video.

As what I say, rolling will also overlap the video, video need keep in top.
But, I think it need anther PR to fix it.

@xz-dev
Copy link
Author

xz-dev commented Sep 26, 2022

I installed this artifact to test it. It is nice, well done. The only thing is that one has to tap on the "show replies" string and is is a bit unconfortable. Could the clickable area be extended at least orizontally? So that, to get more replies I could tap on the middle or on the right side and not only on the "show reply" text.

I just imitate Youtube.
BTW, I try to full the button just now, it's hard to know where is clickable on the right.

@opusforlife2
Copy link
Collaborator

@xz-dev Take how much ever time you need, mate. No hurries. This is purely voluntary work, after all. Mental health comes first, and programming much later.

@xz-dev
Copy link
Author

xz-dev commented Oct 28, 2022

I'm coming back and I make more big change at here.
I remove the dialog, because it looks out of place here.

@nicoursi
Copy link

nicoursi commented Oct 28, 2022

Wow, now it is even faster!

The only issue i can find so far is that if i rotate the screen while the replies are open the app crashes.

@xz-dev
Copy link
Author

xz-dev commented Oct 29, 2022

Wow, now it is even faster!

The only issue i can find so far is that if i rotate the screen while the replies are open the app crashes.

Fixed be3a242

@nicoursi
Copy link

It does not crash with screen rotation but it closes the replies. I think the replies should be kept open with rotation.

With the new commit, it says "5 new streams". I think it should say "5 replies".

Thanks a lot for your efforts, you rock!

@xz-dev
Copy link
Author

xz-dev commented Oct 30, 2022

It does not crash with screen rotation but it closes the replies. I think the replies should be kept open with rotation.

With the new commit, it says "5 new streams". I think it should say "5 replies".

Thanks a lot for your efforts, you rock!

LOL, Fixed. f29c941

@nicoursi
Copy link

nicoursi commented Nov 4, 2022

Could you rebase so that you can include the latest exractor fix and we can continue testing your PR? 😀

@xz-dev
Copy link
Author

xz-dev commented Nov 5, 2022

Could you rebase so that you can include the latest exractor fix and we can continue testing your PR? grinning

Updated.

@nicoursi
Copy link

nicoursi commented Nov 6, 2022

For some reason this bug happens only on this PR, it does not happen on the nightlies version.

To reproduce:

  1. Open this channel https://www.youtube.com/channel/UCZQIPu6LfVfzEnJqDHpWamg
  2. Long press and then play in background the video "ATTACCHI CONTRO MEDICI REINTEGRATI"

` ## Exception

  • User Action: ui error
  • Request: ACRA report
  • Content Country: IT
  • Content Language: it-IT
  • App Language: it_IT
  • Service: none
  • Version: 0.24.0
  • OS: Linux Android 11 - 30
Crash log

java.lang.IllegalArgumentException: No view found for id 0x7f0a0199 (org.schabi.newpipe.debug:id/fragment_container_view) for fragment CommentsFragment{7c055c9} (48d1ff96-556e-4b2a-8b57-c0c6f2a9d30e id=0x7f0a0199)
	at androidx.fragment.app.FragmentStateManager.createView(FragmentStateManager.java:513)
	at androidx.fragment.app.FragmentStateManager.moveToExpectedState(FragmentStateManager.java:261)
	at androidx.fragment.app.FragmentManager.executeOpsTogether(FragmentManager.java:1840)
	at androidx.fragment.app.FragmentManager.removeRedundantOperationsAndExecute(FragmentManager.java:1758)
	at androidx.fragment.app.FragmentManager.execPendingActions(FragmentManager.java:1701)
	at androidx.fragment.app.FragmentManager$4.run(FragmentManager.java:488)
	at android.os.Handler.handleCallback(Handler.java:938)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	at android.os.Looper.loop(Looper.java:223)
	at android.app.ActivityThread.main(ActivityThread.java:7669)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:594)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947)


`

@xz-dev
Copy link
Author

xz-dev commented Nov 7, 2022

For some reason this bug happens only on this PR, it does not happen on the nightlies version.

To reproduce:

1. Open this channel  https://www.youtube.com/channel/UCZQIPu6LfVfzEnJqDHpWamg

2. Long press and then play in background the video "ATTACCHI CONTRO MEDICI REINTEGRATI"

` ## Exception

* **User Action:** ui error

* **Request:** ACRA report

* **Content Country:** IT

* **Content Language:** it-IT

* **App Language:** it_IT

* **Service:** none

* **Version:** 0.24.0

* **OS:** Linux Android 11 - 30

Crash log
`

I had download the CI debug APK, I can't reproduce the error.

@sonarcloud
Copy link

sonarcloud bot commented Nov 7, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@xz-dev
Copy link
Author

xz-dev commented Nov 7, 2022

I think the replies should be kept open with rotation.

It's a very basic problem that currently happens to all listing pages, and I don't think I should not fix it at here.

@AudricV
Copy link
Member

AudricV commented Nov 9, 2022

Could you squash the commits with the previous model used into the new one, in order to have a proper branch?

Also, please rebase your PR instead of adding a separate extractor change and use in the future a separate Git branch for your pull requests, like stated in our contribution guidelines.

Thank you in advance.

@tvnmguy
Copy link

tvnmguy commented Nov 14, 2022

1:

•Highlight mother comment/main comment's background.

If we select dark theme from settings, then highlight with black tint/gradient.
If we select black theme, then highlight with dark tint/gradient.

•Add total number of comment replies in mother comment/main comment.

•Make mother comment/main comment's icon/avatar little bit larger from comment replies account icon/avatar (Reduce size of comment replies account icon/avatar).

For example👇

img_2022_11_14_08_50_35.jpg

IMG_20221114_093319.jpg

For reference;

IMG_20221114_083408.jpg

IMG_20221114_083331.jpg

2:

Adjustment of Replies texts, Mother comment/Main comment, Comment replies, Account icon/avatar for better visual experience and readability 👇

IMG_20221114_083049.jpg

IMG_20221114_082252.jpg

IMG_20221114_083232.jpg

Advantage: We can eliminate scattered feeling. All (Replies, Mother/Main comment, Comment replies) are in single line.

IMG_20221114_101055.jpg

3:

I don't know is this possible, if yes;

Highlight content creator/channel owner comment replies via icon/avatar or like '3 Replies from the hated one and others' text.

IMG_20221114_091625.jpg

IMG_20221114_090910.jpg

Copy link
Member

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

Code looks good. I still don't like the cloning of the comments instance. However, I cannot think of an alternative.
I did not have time to test the changes thoroughly.

@xz-dev xz-dev mentioned this pull request Nov 16, 2022
5 tasks
@xz-dev
Copy link
Author

xz-dev commented Nov 16, 2022

Could you squash the commits with the previous model used into the new one, in order to have a proper branch?

Also, please rebase your PR instead of adding a separate extractor change and use in the future a separate Git branch for your pull requests, like stated in our contribution guidelines.

Thank you in advance.

Ok, move to #9410

@xz-dev xz-dev closed this Nov 16, 2022
@xz-dev
Copy link
Author

xz-dev commented Nov 16, 2022

I still don't like the cloning of the comments instance.

I also wan't, but I have no better idea without change Extractor

@xz-dev
Copy link
Author

xz-dev commented Nov 16, 2022

1:

•Highlight mother comment/main comment's background.

If we select dark theme from settings, then highlight with black tint/gradient. If we select black theme, then highlight with dark tint/gradient.

•Add total number of comment replies in mother comment/main comment.

•Make mother comment/main comment's icon/avatar little bit larger from comment replies account icon/avatar (Reduce size of comment replies account icon/avatar).

For examplepoint_down

img_2022_11_14_08_50_35.jpg IMG_20221114_093319.jpg

For reference;

IMG_20221114_083408.jpg IMG_20221114_083331.jpg

2:

Adjustment of Replies texts, Mother comment/Main comment, Comment replies, Account icon/avatar for better visual experience and readability point_down

IMG_20221114_083049.jpg IMG_20221114_082252.jpg IMG_20221114_083232.jpg

Advantage: We can eliminate scattered feeling. All (Replies, Mother/Main comment, Comment replies) are in single line.

IMG_20221114_101055.jpg

3:

I don't know is this possible, if yes;

Highlight content creator/channel owner comment replies via icon/avatar or like '3 Replies from the hated one and others' text.

IMG_20221114_091625.jpg IMG_20221114_090910.jpg

I think the more important is let it working, if you have more idea about design, you can make a PR to my fork.(it will been included)

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 GUI Issue is related to the graphical user interface multiservice Issues related to multiple services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display comment replies