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

Fix "only this person" #473

Merged
merged 3 commits into from
Jan 3, 2025
Merged

Fix "only this person" #473

merged 3 commits into from
Jan 3, 2025

Conversation

w568w
Copy link
Member

@w568w w568w commented Jan 1, 2025

This PR:

  1. fixes the problem that when filtering posts by a specified user only in the deeper levels of the thread, the post page refreshes itself and becomes blank;
  2. fixes the problem that the status description and actual logic of the "show this user" are inconsistent: when a user filters by the poster of a certain post, long-pressing on another post displays "Show all posts," but clicking it actually filters by the user of that other post.

However, there's still an operation that can easily trap users: if a user filters by the poster of a deeper comment and then pulls down to refresh, the entire page becomes blank because the pull-to-refresh only fetches the first 10 posts. Perhaps the filter setting should be reset upon pull-to-refresh?

Perhaps the best solution would be to automatically fetch the next 10 posts on pull-to-refresh until posts satisfying the filter are found. However, this sounds like it would put a lot of pressure on the backend.

w568w added 2 commits January 1, 2025 18:28
…ld not refresh itself; (2) the status description and actual logic of the "show this user" are inconsistent

(1) This causes the page to become blank when users want to filter by posters only in the deeper levels of the thread.

(2) When a user filters by the poster of a certain post, long-pressing on another post displays "Show all posts," but clicking it actually filters by the user of that other post.
@w568w w568w added this to the 1.4.6 milestone Jan 1, 2025
@AInfinity-LilacDream
Copy link
Collaborator

Is it possible that when only this person triggers, we do not pull-to-refresh, but instead we record posts that have been fetched and set their, say, attribute to invisible to users?

@w568w
Copy link
Member Author

w568w commented Jan 2, 2025

Is it possible that when only this person triggers, we do not pull-to-refresh, but instead we record posts that have been fetched and set their, say, attribute to invisible to users?

This was done in 3cb7ca1. I removed the refreshListView() calls so it wouldn't trigger a refresh when clicking "filtering by this person".

@AInfinity-LilacDream
Copy link
Collaborator

"Perhaps the best solution would be to automatically fetch the next 10 posts on pull-to-refresh until posts satisfying the filter are found. However, this sounds like it would put a lot of pressure on the backend."
I think this may be a method but isn't practical. a solution may be quit the "only this person" mode when refreshing.

Copy link
Collaborator

@AInfinity-LilacDream AInfinity-LilacDream left a comment

Choose a reason for hiding this comment

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

if you consider my suggestion as impractical, then this pr should be good.

@AInfinity-LilacDream
Copy link
Collaborator

sorry for the late comment.

@AInfinity-LilacDream
Copy link
Collaborator

this should work.

@AInfinity-LilacDream AInfinity-LilacDream merged commit e982d21 into main Jan 3, 2025
@@ -68,7 +68,7 @@ String preprocessContentForDisplay(String content) {
if (element is UrlElement) {
// Only add tag if tag has not yet been added.
if (RegExp("\\[.*?\\]\\(${RegExp.escape(element.url)}\\)")
.hasMatch(content) ||
.hasMatch(content) ||
Copy link
Member Author

Choose a reason for hiding this comment

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

Please avoid including irrelevant code formatting in your commits.

Each commit should ideally contain only the necessary changes. As we don't currently have a code formatting guideline, if your IDE automatically formats code (e.g., randomly adding/removing spaces, tabs, newlines), please review your file changes carefully before committing.

Both VSCode and Android Studio allow you to commit specific lines within a file. This enables you to include only the intended changes in your current commit, deferring other formatting adjustments to subsequent commits (or reverting them).

Copy link
Member Author

@w568w w568w left a comment

Choose a reason for hiding this comment

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

Given the changes in 8f88a74, which I found quite puzzling, I'm going to submit a request to revert that commit.

Comment on lines +207 to +209
if (!scrollToEnd && selectedPerson != null) {
func();
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This strikes me as rather confusing and over-abstracted.

If you're passing in a function callback and a selectedPerson parameter, simply to check a condition and then conditionally execute that callback, wouldn't it make more sense to perform that check directly at the point where this function is invoked? Especially considering there's currently only one instance in the entire codebase where an actually useful value is being passed in, and the logic within the callback bears absolutely no relation to refreshing.

What's your rationale for putting this code – which, to me, looks completely non-reusable and seems to have no bearing on what refreshListView is supposed to do – inside the refreshListView method?

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.

2 participants