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

Hide refresh from toolbar #6850

Merged
merged 9 commits into from Jan 20, 2024

Conversation

ueen
Copy link
Contributor

@ueen ueen commented Jan 2, 2024

lengthy discussion here
#6836

update FeedUpdateRunningEvents
@ueen ueen force-pushed the hide_refresh_from_toolbar branch from 6ba5651 to 29989b5 Compare January 2, 2024 17:02
ueen and others added 2 commits January 3, 2024 00:35
…agment.java

Co-authored-by: ByteHamster <ByteHamster@users.noreply.github.com>
@@ -457,7 +462,7 @@ protected void updateToolbar() {
@Subscribe(sticky = true, threadMode = ThreadMode.MAIN)
public void onEventMainThread(FeedUpdateRunningEvent event) {
if (toolbar.getMenu().findItem(R.id.refresh_item) != null) {
MenuItemUtils.updateRefreshMenuItem(toolbar.getMenu(), R.id.refresh_item, event.isFeedUpdateRunning);
swipeRefreshLayout.setRefreshing(event.isFeedUpdateRunning);
Copy link
Member

Choose a reason for hiding this comment

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

The if statement above is no longer needed

ueen and others added 3 commits January 3, 2024 21:51
…java

Co-authored-by: ByteHamster <ByteHamster@users.noreply.github.com>
…agment.java

Co-authored-by: ByteHamster <ByteHamster@users.noreply.github.com>
@ByteHamster
Copy link
Member

I didn't run it on my phone yet (don't have an Android IDE on my work computer), but this looks good to me. If you fix the unused imports, this should be ready to be merged.

@ByteHamster ByteHamster merged commit 34fb205 into AntennaPod:develop Jan 20, 2024
7 checks passed
quails4Eva pushed a commit to quails4Eva/AntennaPod that referenced this pull request Jan 20, 2024
@ueen
Copy link
Contributor Author

ueen commented Jan 24, 2024

I tried this a while on my phone and the pull-to-refresh showing during the complete refresh is really annoying and looks like a bug.
There is an indication in the notification displayed by the download service.
So i woud suggest to shorten the pull-to-refresh loading to a second or so like before.

@ByteHamster
Copy link
Member

We need some indication for an ongoing refresh. The notification is not enough because users apparently don't want to grant the notification permission on Android 14. The app needs to fully work without notifications.

@ueen
Copy link
Contributor Author

ueen commented Jan 24, 2024

Well, the app doesn't work without the notification service, if the app is quite during refresh...
And that's what I feel like doing if the swipe to refresh is loading for 10s or more because I as a user think the app crashed somehow - and I only have around 16 podcasts, others have 100s

@ByteHamster
Copy link
Member

Wouldn't hiding it feel like the refresh was aborted early? Feel free to create an issue that we can discuss on the community call. I don't think this PR is the right place to discuss.

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