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 setting to collapse parents #2433

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alde
Copy link

@alde alde commented Apr 30, 2017

This PR adds a setting to collapse parents when collapsing a child comment.

A proposed solution to #2430.

@ccrama
Copy link
Collaborator

ccrama commented May 1, 2017

Really interesting, thank you for the PR!

One thing I noticed is that you change the ViewHolder and the comment source on the doOnClick function, which also handles hiding the menu if the menu is currently displayed on a comment. It appears that it would hide the top-level comment's menu when running this code, and a better solution might be to only change the source of the comment for the hideAll() function.

@alde
Copy link
Author

alde commented May 1, 2017

Ah, I guessed that there would be a more logical place, but I had trouble finding it. It's my first time poking around with android, so trying to follow the flow of the code was not obvious to me.

I'll see if I can come up with a better solution.

@alde
Copy link
Author

alde commented May 1, 2017

Ah now I remember, I tried editing only hideAll(), but the problem I ran into was that it made me have to click twice, once to collapse the children, and once to collapse the parent.

I have done a small refactoring of the doOnClick to better isolate the side-effects of the setting, and as far as I have been able to test I haven't found any issue

edit: This did however make the patch a bit bigger than I wanted it to be

@ccrama
Copy link
Collaborator

ccrama commented May 3, 2017

Thank you for looking into it!

That is an interesting side effect, have you found any other side-effects of your updated code? And sorry for the size of that class, it is one of the most complicated classes to work with so thank you for giving it a shot!

@alde
Copy link
Author

alde commented May 3, 2017

Yes, the counter of hidden items would be number of children of the clicked comment, not of the parent comment.

But with the refactored code things seem to work as expected.

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.

None yet

2 participants