Loading screen improvement #4938

okan35 · 2020-11-20T20:56:25Z

What is it?

Bugfix (user facing)
Feature (user facing)
Codebase improvement (dev facing)
Meta improvement to the project (dev facing)

Description of the changes in your PR

Currently when we refresh what's new, we see only loading screen on main screen with number of videos loading and at the same time we can see this on notification.
I removed loading screen from main screen so while app is loading new videos, we can also browse and watch videos.We should not be waiting for videos to finish loading so we can watch video or check what we have in list of videos, this definitely was not a good idea so I changed this.
While videos load we can still check the content. And if the someone wants to see number of videos that are loading, he/she can do so by checking the notification.

pull from remote master

B0pol · 2020-11-21T04:00:20Z

I really dislike this behaviour.

The loading screen is there to show the user it's loading and it's not frozen.

Now you don't know the progress, it refreshes when it's loaded and really feels "wow it didn't work, I'm going to press refresh button again" with about 50 subs.

On top of that, it seems you didn't read contribution guidelines, because you don't follow them

Send PR that only cover one specific issue/solution/bug. Do not send PRs that are huge and consists of multiple
independent solutions

Then why do you send a PR with an unecessary dependency on a pull to refresh PR?

If you want to add a feature or change one, please open an issue describing your change. This gives the team and community a chance to give feedback before you spend any time on something that could be done differently or not done at all. It also prevents two contributors from working on the same thing and one being disappointed when only one user's code can be added.

okan35 · 2020-11-21T06:37:49Z

I now see where you are coming from because if you click to refresh the feed it shows like there is nothing going on. But if you pull to refresh it shows you a loading icon until the process is done and after click or pull to refresh videos still load.

We can also add that small icon so people who dont pull to refresh but click to refresh can see what is going on.

As to issue part, this actually was in #3329, you can see my comment and I don't know why it was closed as the requests in that issue were not done yet and only implemented small part of it which is pull to refresh.

Also instead of opening new issue I added mine in the same issue as it is covered under improvement of refresh.

opusforlife2 · 2020-11-21T09:45:40Z

@okan35 We now strictly follow a policy of "one idea per issue/PR". So you can just open a new issue, describe the problem this PR is solving, and link it to this PR.

okan35 · 2020-11-21T11:49:58Z

@okan35 We now strictly follow a policy of "one idea per issue/PR". So you can just open a new issue, describe the problem this PR is solving, and link it to this PR.

I just did that and later will add the progress dialogue or icon whatever for click refresh, pull to refresh already does that.

opusforlife2 · 2020-11-21T17:30:48Z

@okan35 You can use keywords to link a PR to an issue. https://docs.github.com/en/free-pro-team@latest/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue

okan35 · 2020-11-21T17:37:55Z

@okan35 You can use keywords to link a PR to an issue. https://docs.github.com/en/free-pro-team@latest/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue

Will do it next time, didn't know this existed.

okan35 · 2020-11-21T17:56:02Z

Added the necessary change for showing loading icon on click to refresh and updated the app. I believe the feature is done now.

Stypox

I like this, though I am not sure if it is a good idea not to show any progress indicator except for the hard-to-access notification. You could show the loading bar, which previously was shown at the center of the screen, just below the Feed last updated: ... bar at the top. Also, please remove all comments, they are not needed anymore. Thank you for this contribution that would decrease time waste :-D

Stypox · 2020-12-12T14:05:51Z

app/src/main/java/org/schabi/newpipe/local/feed/FeedFragment.kt

-        animateView(refresh_root_view, false, 0)
-        animateView(items_list, false, 0)
+        animateView(refresh_root_view, true, 0)
+        animateView(items_list, true, 0)


There is no need for this, i.e. to show items_list, if you never hide it ;-)

Stypox · 2020-12-12T14:06:40Z

app/src/main/java/org/schabi/newpipe/local/feed/FeedFragment.kt


    private fun handleErrorState(errorState: FeedState.ErrorState): Boolean {
        hideLoading()
+        swipeRefreshLayout.isRefreshing=false


No need for this, it is already inside hideLoading()

okan35 · 2020-12-13T14:38:13Z

I like this, though I am not sure if it is a good idea not to show any progress indicator except for the hard-to-access notification. You could show the loading bar, which previously was shown at the center of the screen, just below the Feed last updated: ... bar at the top. Also, please remove all comments, they are not needed anymore. Thank you for this contribution that would decrease time waste :-D

I pushed the changes for those reviews you left. I would suggest that we do that feature of showing progress of video fetching in another pull request. But it is a good idea to show it like that, below feed last updated when user refreshes video list, and when it finishes we can hide it back,

Stypox

I would suggest that we do that feature of showing progress of video fetching in another pull request.

Yeah, ok, but both PRs need to ship in the same release, otherwise we are taking away the progress indicator from users, which would be cumbersome. Are you willing to make another PR right after this one?

Stypox · 2020-12-14T18:47:06Z

app/src/main/java/org/schabi/newpipe/local/feed/FeedFragment.kt

            return feedFragment
        }
    }
-}


Add back the newline at the end of the file (checkstyle should have complained)

Stypox · 2020-12-14T18:47:12Z

app/src/main/java/org/schabi/newpipe/local/feed/FeedFragment.kt

        }
    }

-    // /////////////////////////////////////////////////////////////////////////


Bring back these comments.. sorry if I was unclear, I meant that old code shouldn't be commented-out but directly removed. But useful comments like this should be kept.

okan35 · 2020-12-14T20:08:39Z

I can make another pr but I will do it around this week as it is a matter of time :D

video fetch

okan35 · 2020-12-19T15:04:30Z

I added that progress text below video last fetched and those comments are back as I went to that commit. Updated the apk. To me it looked nice as it shows up with progress and disappears when it is finished.

Stypox

Could you rebase?

Stypox · 2021-01-14T21:49:48Z

app/src/main/java/org/schabi/newpipe/local/feed/FeedFragment.kt

        }
    }

-    // /////////////////////////////////////////////////////////////////////////


Bring back these comments ;-)

litetex · 2021-10-01T17:40:01Z

Closing this for now:

no progress
no GitHub actions build
merge conflicts
Snyk Found vulnerabilities!
No feedback from the author

Feel free to reopen it when there is progress again.

okan35 added 2 commits November 20, 2020 18:50

Merge pull request #1 from TeamNewPipe/dev

4bcf224

pull from remote master

removed loading screen from main screen

1db2cf1

loading added to click to refresh

fa0c4ce

Stypox requested changes Dec 12, 2020

View reviewed changes

review changes

a8efe1d

Stypox requested changes Dec 14, 2020

View reviewed changes

okan35 added 3 commits December 19, 2020 16:00

added progress for video fetch

acdaf30

Merge branch 'newBranchVideoFetch' into whatsNewLoadingScreenChange

eb5aaae

video fetch

use getString

a9b55cf

Stypox reviewed Jan 14, 2021

View reviewed changes

TobiGr force-pushed the dev branch 2 times, most recently from 679bc75 to 2aeccc0 Compare March 16, 2021 08:24

litetex marked this pull request as draft October 1, 2021 17:39

litetex closed this Oct 1, 2021

litetex mentioned this pull request Oct 10, 2021

Add CODEOWNERS file #7216

Closed

2 tasks

Uh oh!

Loading screen improvement #4938

Loading screen improvement #4938

Uh oh!

Conversation

okan35 commented Nov 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is it?

Description of the changes in your PR

Fixes the following issue(s)

APK testing

Due diligence

Uh oh!

B0pol commented Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

okan35 commented Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

opusforlife2 commented Nov 21, 2020

Uh oh!

okan35 commented Nov 21, 2020

Uh oh!

opusforlife2 commented Nov 21, 2020

Uh oh!

okan35 commented Nov 21, 2020

Uh oh!

okan35 commented Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Stypox left a comment

Choose a reason for hiding this comment

Uh oh!

Stypox Dec 12, 2020

Choose a reason for hiding this comment

Uh oh!

Stypox Dec 12, 2020

Choose a reason for hiding this comment

Uh oh!

okan35 commented Dec 13, 2020

Uh oh!

Stypox left a comment

Choose a reason for hiding this comment

Uh oh!

Stypox Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

Stypox Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

okan35 commented Dec 14, 2020

Uh oh!

okan35 commented Dec 19, 2020

Uh oh!

Stypox left a comment

Choose a reason for hiding this comment

Uh oh!

Stypox Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

litetex commented Oct 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

okan35 commented Nov 20, 2020 •

edited

Loading

B0pol commented Nov 21, 2020 •

edited

Loading

okan35 commented Nov 21, 2020 •

edited

Loading

okan35 commented Nov 21, 2020 •

edited

Loading