Titanium JIRA Archive
Titanium SDK/CLI (TIMOB)

[TIMOB-25663] Android. HttpClient.cache. ImageView.cache

GitHub Issuen/a
TypeImprovement
PriorityCritical
StatusClosed
ResolutionWon't Do
Resolution Date2020-06-30T14:42:06.000+0000
Affected Version/sn/a
Fix Version/sn/a
ComponentsAndroid
LabelsAndroid, HttpClient, ImageView, parity
ReporterSergey Volkov
AssigneeVijay Singh
Created2018-01-11T12:55:06.000+0000
Updated2020-06-30T14:42:06.000+0000

Description

In iOS Titanium.Network.HttpClient has _cache_ property, which allow to store http responses in locally on device, Ti SDK for Android lack this property. But remote images loaded by ImageView are stored in cache and there is no way to disable it. I have implemented HttpResponseCache module for Android (simple binding to [android.net.http.HttpResponseCache](https://developer.android.com/reference/android/net/http/HttpResponseCache.html)), also I've added _cache_ properties to HttpClient and ImageView. So attached pathes intoduces this features: - conditional caching for HttpClient responses and for remote images loaded by ImageView - configurable http-response cache storage with some statistics In patches _Ti.Android.HttpResponseCache_ is not installed (enabled) on application start and should be installed manually by calling "install" method. Without enabled _Ti.Android.HttpResponseCache_ "HttpClient.cache = true" will work with _TiResponseCache_, but "ImageView.cache = false" will not work, because images cached by _TiResponseCache_ anyway. I think, if you'll find this changes usefull, _TiResponseCache_ class could be totally removed because, as I understand correctly, _TiResponseCache_ was created when _HttpResponseCache_ wasn't exists and stays for now as legacy.

Attachments

FileDateSize
0001-Android.-Add-HttpResponseCache-module.patch2018-01-11T12:53:02.000+00009302
0002-Android.-Add-HttpClient.cache-property.patch2018-01-11T12:53:02.000+00003337
0003-Android.-Add-ImageView.cache-property.patch2018-01-11T12:53:02.000+00008137

Comments

  1. Hans Knöchel 2018-01-11

    Hey [~s.volkov], the patches look interesting! You can submit a pull-request to the [titanium_mobile](https://github.com/appcelerator/titanium_mobile) repository to include this in the SDK. We will then review your pull request and guide you to the right direction if something is missing. Let us know if you can do that! Moving to TIMOB.
  2. Sergey Volkov 2018-01-11

    https://github.com/appcelerator/titanium_mobile/pull/9719
  3. Hans Knöchel 2018-01-12

    Assigning to version 7.1.0 for now, but it depends on the team resources for new features. Some general thoughts on the PR: * The PR is Android only, which is against the parity-first approach. So we might want to think about an iOS solution as well * Right now it seems like you are the only person requesting this feature, so we will need to investigate how critical it is for the general SDK * The PR misses [unit-tests](https://github.com/appcelerator/titanium_mobile/#unit-tests). You can see some examples [here](https://github.com/appcelerator/titanium-mobile-mocha-suite/tree/master/Resources)
  4. Sergey Volkov 2018-01-15

    [~hknoechel], this case is mostly about restoring parity, because iOS already has _cache_ property on httpClient. I've added tests for HttpResponseCache module and HttpClient.cache. It is currently impossible to create tests for ImageView.cache, because images are cached in memory by TiImageLruCache which is not exposed to js.
  5. Hans Knöchel 2018-01-16

    Hey [~s.volkov], thanks for your reply. -I am mostly concerned about the fact that iOS and Windows use the cache property as a boolean, where by the Android platform will use an own proxy.- EDIT: I saw it incorrectly! It's actually a boolean as well and the HttpResponseCache is for global configuration! Cool! But - I also agree caching should become more configurable, so we'd rather look for a Ti.Network.HttpResponseCache cache that will be exposed on all platforms. Properties like httpCacheSize should be easy to expose, methods like flush, install and close seem quite Android-specific. I would also like to consult [~jquick] for additional feedback, but due to the fact that we currently focus on bug-fixing across the SDK, this PR might need to wait for a later release than 7.1.0. We'll keep you posted in either way!
  6. Gary Mathews 2018-02-13

    Bumping out of 7.1.0 so we can work out a solution that's compatible with all platforms.
  7. Joshua Quick 2018-10-31

    I do agree that we should support the "cache" property in HTTPClient on Android. I don't like the idea of replacing our TiResponseCache with Google's HttpResponseCache. We definitely don't want to lose the custom features we've added to our class such as our relatively new "redirect" methods. Any new caching features we want to add should be added to our class. I don't think we should add any more network related APIs to our ImageView. I'd rather we keep ImageView simple. If an app developer wants more control over the HTTP request/response handling, then it should be done via HTTPClient and the downloaded image should then be applied to the ImageView via a file path. Case-in-point, [~topener] has created a "ti.imageview" module that wraps an ImageView and allows you to specify the HTTP request headers. Internally, it uses HTTPClient to do so. This is all done in pure JavaScript by dog-fooding our Titanium APIs, which means all platforms Titanium supports (Android, iOS, Windows) get this feature for free. https://github.com/appcelerator-modules/ti.imageview
  8. Sergey Volkov 2018-10-31

    Everything below IMHO, I mean no offence. It's been almost two years since I wrote this patch for 6.0.0 SDK, but anyway: I understand why TiResponseCache was written in 2010, but don't see any reason to keep it now (even for your newly written methods, because HttpResponseCache should also handle redirects). It seems obvious that _less code == less bugs_ ([this](https://github.com/NetrisTV/ti.exoplayer/issues/3), for example, caused by bug in TiResponseCache: it tries to get response for POST request from cache; just have no time to properly file the issue and PR). I've seen "ti.imageview" (and [Topener/To.ImageCache](https://github.com/Topener/To.ImageCache) by Rene). It's seems ok, except possible overhead with copying blob from java to js and back to java; also I'm not sure how it will work with TiImageLruCache. About "Android-specific API": HttpResponseCache is in Ti.Android namespace and mirroring native module API why wouldn't it be Android specific? It's not like you already have any other incompatible API for cache control by now. I'm fine if you'll close this ticket and PR, but I'm going to keep using this patch-set for local SDK builds because currently there is no other way for my use case.
  9. Joshua Quick 2018-10-31

    Your code loses an optimization we've made in ImageView where we directly check the HTTP response cache if the image has already been downloaded before proceeding with a threaded-out HTTP request. If the cached response is a redirect, then it'll keep following it in the cache until it reaches the end-point for the image. This is especially important for our image loading APIs such as backgroundImage and {{defaultImage} which still do a "blocking" download. Regarding TiImageLruCache, our plan is to remove it. It's not our job to decide which images/photos should be cached and it increases the likelihood of out-of-memory exceptions. Its biggest issue is that it keeps hard reference to images. It's up to app developers to decide which images should be cached in memory and that is best done via blobs. There is no additional overhead with blob usage since an image needs to be decoded into memory before being displayed anyways. In fact, it improves image loading performance since Google's Android APIs do not cache images outside of the APK "res" folder. (We may implement a weak-reference cache instead; we'll see.)
  10. Sergey Volkov 2019-01-11

    I've closed the PR. Feel free to close this ticket.

JSON Source