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

Resets the paging3 changes of 3159 back to the (java) fragment code #4015

Merged

Conversation

Lakoja
Copy link
Collaborator

@Lakoja Lakoja commented Sep 11, 2023

No description provided.

…efore.

Should be the basis for further not-so-rattling improvements.
@mcclure
Copy link
Collaborator

mcclure commented Sep 11, 2023

I tested this patch against regressions caused by 3159:

  • "Glitchy animation" during pulldown refresh— fixed (but I have animations disabled, would be good if someone could test on a second phone)
  • Pulldown refresh fetches twice— Did not test, I don't know how to test this
  • "when [pulldown] refreshing while offline fullscreen error appears so one can't see read already loaded notifications anymore"— fixed (I could not reproduce)
  • Refreshing notifications collapses already expanded content warning #3623 - Refreshing notifications collapses already expanded content warning — fixed
  • Notifications scroll can get stuck in the distant past (say by days) and you can't jump up to "the present" except by using the menu / "Load More" should come back until we make an explicit decision to remove this — I don't know how to test this because it would require leaving the app open for a long period. However in all my tests, closing and reopening the app caused the notifications tab to always jump to "the present", which mostly obviates this problem. I don't know if that's "expected"/as-of-March behavior.

@connyduck
Copy link
Collaborator

It's missing the button functions (needs Kotlin to call the Api)

add some more "old" functions to MastodonApi?

Most menu items are not wired (they were added after this code)

don't we want to get rid of them as well and have the old bar back?

There are some new functions with "Old" in the name which should be removed when Kotlin

That is fine

Some (newer) code can still be removed as it is now unused

remove it as well please

@mcclure
Copy link
Collaborator

mcclure commented Sep 11, 2023

don't we want to get rid of them as well and have the old bar back?

There are 4 items in the menu

  • Refresh
  • Load newest notifications
  • Filter notifications
  • Delete notifications

I think when we bring the bar back we should remove "Filter notifications" and "Delete notifications". (Except when the bar is hidden, which I think you said we had a preference for.)

But it may be worth keeping the menu for "Refresh" and "Load newest notifications". You can already get these both things with gestures (pull down or tap tab header) but having a second option when that is not clear might be good.

@connyduck
Copy link
Collaborator

I think when we bring the bar back we should remove "Filter notifications" and "Delete notifications". (Except when the bar is hidden, which I think you said we had a preference for.)

Yes makes sense.

"Load newest notifications" - was that a workaround to jump back to the top when one is way behind on notifications? If yes it makes no sense anymore, if no what is the difference to "refresh"?

@Lakoja
Copy link
Collaborator Author

Lakoja commented Sep 11, 2023

Noticed something related in this code: #3648

Not sure if this is a problem (i. e. will fetch notifications endlessly as there are so many...).

Worth an investigation / followup?

@Lakoja
Copy link
Collaborator Author

Lakoja commented Sep 11, 2023

It's missing the button functions (needs Kotlin to call the Api)

add some more "old" functions to MastodonApi?

Some (newer) code can still be removed as it is now unused

remove it as well please

Done 2x

@Lakoja Lakoja marked this pull request as ready for review September 11, 2023 20:20
@Tak Tak merged commit f7b2962 into tuskyapp:develop Sep 12, 2023
1 of 3 checks passed
@Lakoja Lakoja deleted the revert-3159-paging3-notification-timeline-2 branch September 26, 2023 19:50
@charlag charlag added this to the Tusky 24 milestone Nov 18, 2023
connyduck added a commit that referenced this pull request May 31, 2024
The option was removed in #3877,
then forgotten to be added again in
#4015.
#4026 added it again, but took to
long to merge, so we made #4225 as
well and that is how we ended up with the option twice.
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