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

Added Comments :) #186

Merged
merged 16 commits into from
May 11, 2022
Merged

Added Comments :) #186

merged 16 commits into from
May 11, 2022

Conversation

Bnyro
Copy link
Member

@Bnyro Bnyro commented May 8, 2022

No description provided.

@IceDBorn
Copy link

IceDBorn commented May 8, 2022

@Bnyro Leaving my finger while scrolling down comments minimizes the video player.

@Bnyro
Copy link
Member Author

Bnyro commented May 8, 2022

@IceDBorn Thank you for the report, already realized it too, I hope that @lilcheti has an idea why, cause I don't

@Bnyro
Copy link
Member Author

Bnyro commented May 8, 2022

Got it fixed :D

@Bnyro
Copy link
Member Author

Bnyro commented May 8, 2022

Just added that you can load more comments through nextpage by scrolling down while being at the comments page, the pr should be ready to become merged now. Requesting reviews :D

@IceDBorn
Copy link

IceDBorn commented May 8, 2022

@Bnyro Almost great! (crashed one time while loading more comments)

@Bnyro
Copy link
Member Author

Bnyro commented May 8, 2022

@Bnyro Almost great! (crashed one time while loading more comments)

Same here xD

@Bnyro
Copy link
Member Author

Bnyro commented May 8, 2022

@Bnyro Almost great! (crashed one time while loading more comments)

How often did you load new comments? I could just limit it to prevent the ram to be full

@Bnyro
Copy link
Member Author

Bnyro commented May 8, 2022

@Bnyro Almost great! (crashed one time while loading more comments)

How often did you load new comments? I could just limit it to prevent the ram to be full

If the ram is the issue, idk

@Bnyro
Copy link
Member Author

Bnyro commented May 8, 2022

@lilcheti @FireMasterK

@IceDBorn
Copy link

IceDBorn commented May 8, 2022

@Bnyro Almost great! (crashed one time while loading more comments)

How often did you load new comments? I could just limit it to prevent the ram to be full

I was just scrolling down at full speed

@Bnyro
Copy link
Member Author

Bnyro commented May 8, 2022

Ok, the ram is not the issue, tested it xD

@Bnyro
Copy link
Member Author

Bnyro commented May 8, 2022

@Bnyro Almost great! (crashed one time while loading more comments)

How often did you load new comments? I could just limit it to prevent the ram to be full

I was just scrolling down at full speed

Uh, ok. Any idea what the reason for the crash could be?

@IceDBorn
Copy link

IceDBorn commented May 8, 2022

Uh, ok. Any idea what the reason for the crash could be?

@Bnyro The UI freezes while loading comments, swiping with my finger might have crashed it because it was frozen.

@Bnyro
Copy link
Member Author

Bnyro commented May 8, 2022

Uh, ok. Any idea what the reason for the crash could be?

@Bnyro The UI freezes while loading comments, swiping with my finger might have crashed it because it was frozen.

Yeah, that's possible

@Bnyro
Copy link
Member Author

Bnyro commented May 8, 2022

Let's wait for Kavins or Farbods review

@lilcheti
Copy link
Member

lilcheti commented May 8, 2022

Is there a ram leakage?

@Bnyro
Copy link
Member Author

Bnyro commented May 8, 2022

Is there a ram leakage?

Idk, Dev options say it took 400mb at maximum so that shouldn't be the issue

@FireMasterK
Copy link
Member

It's not a memory leak, I had a look while profiling, the layout calculations take too much time, so the application freezes.

@@ -16,7 +16,7 @@ class CommentsAdapter(private val comments: MutableList<Comment>): RecyclerView

fun updateItems(newItems: List<Comment>){
comments.addAll(newItems)
notifyDataSetChanged()
notifyItemRangeInserted(comments.size, newItems.size)
Copy link
Member

Choose a reason for hiding this comment

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

You should calculate comments.size before adding the newItems :)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm what's the difference?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added it but I don't know what saving it in a var should change xD

@enessgokalp
Copy link
Contributor

enessgokalp commented May 9, 2022

Can you add comment replies? @Bnyro

@Bnyro
Copy link
Member Author

Bnyro commented May 9, 2022

@enessgokalp I think it’s currently not supported by the Piped API

@FireMasterK
Copy link
Member

@enessgokalp I think it’s currently not supported by the Piped API

It is supported, to get replies, you will have to fetch nextpage for it with the page in repliesPage.

@Bnyro
Copy link
Member Author

Bnyro commented May 9, 2022

Why isn't this in the docs? :)
However, I'm not gonna implement it for now cause I'm busy at the moment, maybe in the future

@lilcheti lilcheti merged commit 932ecfc into libre-tube:master May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants