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

Allow Forced Refresh on Comment View #170

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Tukajo
Copy link
Contributor

@Tukajo Tukajo commented Apr 29, 2024

This is to allow for comment view's "refresh" button to bypass the cache. Otherwise, the network disk cache will always be used. Which is not the intent of a refresh.

@Tukajo
Copy link
Contributor Author

Tukajo commented Apr 29, 2024

@SimonHalvdansson TODO: It would probably be better to just use the volley cache all over instead of relying on your internal caching to resolve stories. That way we won't have two caches that are conflated with each other easily.

I am referring to the cache that @AppearamidGuy made in NetworkComponent.

@AppearamidGuy
Copy link
Contributor

Does it actually use cache for any requests? I think no, because responses don't contain any Cache-Control or Expires headers, so it always runs networks request.

Still I do agree that using Volley's cache is a good idea. Looking through CacheDispatcher it is possible to make it both return cached request and make a network request by overriding StringRequest#parseNetworkResponse like this (barely tested code, but I do see success callback twice in logs):

StringRequest stringRequest = new StringRequest(Request.Method.GET, url, response ->{...}, error ->{...}) {
            @Override
            protected Response<String> parseNetworkResponse(NetworkResponse response) {
                Response<String> res = super.parseNetworkResponse(response);
                if (res != null && res.cacheEntry != null) {
                    res.cacheEntry.ttl = Long.MAX_VALUE;
                }
                return res;
            }
        };

@Tukajo
Copy link
Contributor Author

Tukajo commented Jun 8, 2024

@AppearamidGuy I honestly don't remember now because it's been a few months since I looked at this and my computer has since been bricked (waiting for a repair).

What I do remember is, using a debugger to step through scenarios on my S23 Ultra and my changes had performed some extra network requests that were otherwise not happening before (meaning they were cached).

I am definitely open to somehow testing this better but my point still stands that this should just be using your cache as it is much better IMO to cache in the way they you had set it up.

Too bad there's no unit tests and I'm far too lazy to set those up right now 😅.

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.

2 participants