razeware / emitron-Android

Android version of emitron
Apache License 2.0
55 stars 30 forks source link

Ema 276 library pull to refresh #285

Closed filbabic closed 4 years ago

filbabic commented 4 years ago

Added a Pull to refresh component, and some logic to fix failed requests.

Hopefully fixes #276.

I noticed an issue with loading the Library data. For me (device & emulator), I run the app, and the Library loading progress just keeps on spinning. If I don't switch to a different fragment and back, it just never stops spinning, and never loads the data.

Here's a video describing the behavior: https://www.dropbox.com/s/d2wz0sh86yqjcpp/continuous%20loading.webm?dl=0

I left my phone in the loading state to see if it'll work out, but after over 10 minutes of loading, nothing happened.

By adding some smaller changes such as:

OkHttpClient.Builder()
  .connectTimeout(30, TimeUnit.SECONDS)
  .callTimeout(30, TimeUnit.SECONDS)
  .readTimeout(30, TimeUnit.SECONDS)
  .writeTimeout(30, TimeUnit.SECONDS)

In the NetModule (fixes the issue), and

      UiStateManager.UiState.INIT_FAILED -> {
        loadCollections() // retry
      }

within the LibraryFragment UI handling, I manage to add some sort of a fallback, that just loads the data continuously until the data is actually there. I also noticed we don't have proper error handling to mitigate this, so the spinner just keeps on spinning in case we don't manage to load the data properly. Not sure if this is wanted behavior @orionthewake.

@sammyd Could you check this endpoint, to see if there's something iffy with the server/api?

Endpoint: https://api.raywenderlich.com/api/contents?page%5Bnumber%5D=1&page%5Bsize%5D=10&filter%5Bcontent_types%5D%5B%5D=collection&filter%5Bcontent_types%5D%5B%5D=screencast&filter%5Bq%5D=&sort=-released_at

This is pure console output, so if you need specific data pre-formatting/encoding, let me know.

I basically realized that the request takes over 15 seconds, and the default timeout was 10ish, so adding larger timeouts should fix the issue, but I'd love it if we could see if we could optimize the request somehow.

--

The behavior regarding the Pull To Request feature is shown below in the video:

Pull To Refresh: https://www.dropbox.com/s/mg9huokv5tcndbv/pull%20to%20refresh.webm?dl=0

It kind of falls out of place regarding our design system, as it's just a regular PTR, but I think it's a good fallback for folks that wanted this feature. Let me know if we'd like to apply more design to this, or change it up somehow!

--

I do have some concerns regarding our current loading process, and the way data is being handled/propagated throughout the app, I think it may be a bit too complex for the simplicity of what it does (loads 1-2 requests and that's it), so I'd love to see if I can try to optimize this or clean up the code a bit in the future. But also, it works fine, so I guess "if it ain't broke, don't fix it"? @orionthewake Would love to hear your thoughts.