Hide Photos removed from a category

[ramack]

fixes #233

[ramack]

@coolo do you see why the test fails here?
I see No views in hierarchy found matching: with text: is "Large" - can you give @remi-martin and me a small hint how to debug that?

[ramack]

Ah easy. After selecting English as language is is quite straight forward to run the test locally. There it looks like a refresh is missing after switching accounts...

[ramack]

@remi-martin can you look into this and refresh the album after switching the account or should I?

[ramack]

I stepped a little into it. For me the root cause is that you only display the images from the cache.

In ImageRepository.getImages() we should come back to reading also the remotes and emit the end of the stream only when the cache stream and the remote stream ended. This is why I initially had a .concatWith(remotes) there.

So the issue now is, that we can nicely show images from the cache and keep the cache up to date, but while loading we don't get the freshly loaded images into the view and stay with the cached version from the start - which is on the initial opening of the album the empty set :-(

[ramack]

Furthermore the subscribe()-calls in CategoriesRepository and ImageRepository do not have an onError which crashes the app if for example the server does not respond with a proper HTTP result, consider at least

•    restCategories.subscribe();
  

•    restCategories.subscribe(n -> {}, e -> {
  

•        Log.d("CategoriesRepository:", e.getLocalizedMessage() + " error");
  

+// TODO: check how to handle: snackbar?

•    });
  

What is the issue you had with the original code to concatenate the cache and remote results?

[remi-martin]

Sorry, I'm not supposed to work during the week end. For #238, I think we need to find a way to load while displaying and be sure that the display waits for update to finish before it does (I mean ImageRepository.getImaes wait for ImageRepository.update).

[remi-martin]

My issue was that we were downloading while getting image for display. But in order to delete images from the cache, I needed to fetch all images from the server and then iterate on the cache ones to delete / update them.
That's why I made 2 methods.

[ramack]

Don't we need a .concatWith(remotes) before .toObservable() here, as it is in the Categories?

[ramack]

This filter operation makes the cache basically meaningless, as it seems to block the output until the remotes are fully there. Can we do it the other way round? And keep the local stream in addition. Then you could do the removal from the database in a separate thread in an onComplete callback for the remotes and filter there to remove the local variant.

In the first round I would accept, that the image is still shown (as we already emitted it) and only remove it from the cache to have it removed from the view only in the next refresh. In the long term we could emit at the very end - with another concatWith() the ones that are deleted - with the PositionedItem we should be able to emit a "removal" for a position (if we extend it). But this is in my eyes not critical for this release.

[ramack]

In case we get a "304 Not Modified" HTTP response we could even drop the image from the remote.

[remi-martin]

When should we get an error 304 ? When we are attempting to fetch the images ? (Sounds weird)

[ramack]

When should we get an error 304 ? When we are attempting to fetch the images ? (Sounds weird)

Nono 304 is not an error. It only means that our cache is up to date and we don't need to redownload it. And then we also don't need to refresh the one that is already shown.

[remi-martin]

Ok, I know 100 - 300 status are not errors, just a mistake I made in my message. So if I understand well, the getImages() function should throw a 304 status so we don't have to update the view and the cache ?

[ramack]

I don't remember by heart when.exactly we get the 304, but in case we have emitted an image from.the cache and the concatenated remote stream returns the same image with a 304 we could drop it from the observable by a filter to avoid the redraw.

[remi-martin]

One problem I think, the reload comes from the fact that I clear the albums and images lists on each refresh because instead the lists will keep their length and will have an album or an image duplicated, so we have to rebuild the list. Maybe I can create a deleteCacheAlbums() and deleteCacheImages() that will return the deleted elements in order to delete them from the viewModel list as well. I can see one problem which is that we will remove after or while the display.

[remi-martin]

Only the first album and the first line of images are shown. I don't know why but it's maybe because the subscriber completes too soon

[ramack]

We are coming closer, great!
I digged a little into the strange "only 3-photos shown on first load" during the weekend I a think I found the root cause:
Having two separate RecyclerView in the NestedScrollView, both with layout_height=wrap_content is not a good idea.

To fix that we have to remove the NestedScrollView from fragment_albums.xml and merge the content of the two RecyclerViews. This combined Recycler shall support both types of views: albums and images. Here the main challenge lies in the layout, as I'd like to keep the album view with the full width in portrait and multiple columns for the squared images. This will be possible with a setSpanSizeLookup in the GridLayoutManager.

I have a ugly and unclear version of it running locally but it is not yet pushable and still crashing every now and then and the first load of the pictures is still not shown (which could be still the filter in ImageRepository.getImages) but at least the limit to the first line is gone by that. Feel free to do it on your side, or wait until I clean it up - hopefully next weekend.

Onother thing that I stubmled over is the order of the paramerters to ItemDiffCB in BindingRecyclerViewAdapter.update() - I have the impression it is reversed.

[ramack]

See my #270 - it's not yet perfect, but I believe it is at least a step forward.