[TIMOB-25663] Android: HttpClient.cache. ImageView.cache #9719

drauggres · 2018-01-11T15:46:47Z

JIRA: https://jira.appcelerator.org/browse/TIMOB-25663

Add Ti.Android.HttpResponseCache module
Add Ti.Network.HTTPClient cache property
Add Ti.UI.ImageView cache property

build · 2018-01-11T15:54:23Z

	Messages
📖	🎉 Another contribution from our awesome community member, drauggres! Thanks again for helping us make Titanium SDK better. 👍

Generated by 🚫 dangerJS

hansemannn · 2018-06-28T07:46:37Z

@drauggres Thanks for all your work on this! We will schedule this for the next release now. Can you please re-sign the Axway CLA? It was changed lately to migrate the legal part of the swaggy open source community to our legal team.

build · 2018-06-28T08:11:25Z

	Messages
📖	🎉 Another contribution from our awesome community member, drauggres! Thanks again for helping us make Titanium SDK better. 👍

Generated by 🚫 dangerJS

sgtcoolguy · 2018-07-23T14:09:27Z

tests/Resources/ti.android.httpresponsecache.test.js

+'use strict';
+var should = require('./utilities/assertions');
+
+describe('Titanium.Android.HttpResponseCache', function () {


you can just do describe.android('Titanium.Android.HttpResponseCache', function () { and all the nested tests all be limited to android, so that you don't need to ad date filter to each it.

sgtcoolguy · 2018-07-23T14:11:01Z

tests/Resources/app.js

+require('./ti.accelerometer.test');
+require('./ti.analytics.test');
+require('./ti.api.test');
+require('./ti.android.httpresponsecache.test');


Once I merge to the full test suite, I'd modify the app.js like this, but for PRs the easier way to avoid it is to simply ensure your additional tests are named ti.whatever.addontest.js and they'll get picked up automatically. The addontest.js suffix is what gets it picked up.

Remove app.js
Probably someone should update README

sgtcoolguy · 2018-07-23T14:13:26Z

tests/Resources/ti.network.httpclient.test.js

+		xhr.send();
+	});
+
+	it.android('HttpResponseCache xhr.cache custom', function (finish) {


Again, you can just add an "add-on test" to add new unit tests to the suite and not have to copy/override the existing API test file. Makes it easier for you and for us to see the new tests added.

Copying and modifying is what's necessary for modifying existing tests (say fixing a test we skip on a given platform and re-enabling it).

So anyways, for next time, you could just create a ti.network.httpclient.addontest.js and add these two new tests to it.

sgtcoolguy · 2018-07-23T14:15:56Z

.../android/src/java/ti/modules/titanium/android/httpresponsecache/HttpResponseCacheModule.java

+	{
+		HttpResponseCache cache = HttpResponseCache.getInstalled();
+		if (cache != null) {
+			cache.getNetworkCount();


missing return here.

sgtcoolguy · 2018-07-23T14:24:33Z

.../android/src/java/ti/modules/titanium/android/httpresponsecache/HttpResponseCacheModule.java

+	// clang-format off
+	@Kroll.method
+	@Kroll.getProperty
+	public long getHttpCacheSize()


I'm not real fond of the long name here. I assume it was used to not clash with the size() method?

Why not just mirror the underlying API's use of maxSize for the property/method name?

Additionally, please note that we're going to be moving away from getter/setter methods that just wrap properties in favor of using typical JS property accessors. So in this case, that'd be removing the @Kroll.method annotation and just assigning this method to be the backing impl for cache.httpCacheSize calls in JS. (Or hopefully cache.maxSize if the property is renamed.

maxSize it is

sgtcoolguy · 2018-07-23T14:25:51Z

.../android/src/java/ti/modules/titanium/android/httpresponsecache/HttpResponseCacheModule.java

+	// clang-format off
+	@Kroll.method
+	@Kroll.setProperty
+	public void setHttpCachePath(String value)


Again, not fond of the long name. Why not just use path as the property?

Also, the setter method would be deprecated in 8.0.0 immediately anyhow. So maybe just use this as the @Kroll.setProperty impl backing cache.path = 'whatever'; calls. (i.e. remove @Kroll.method)

httpCachePath -> path, property only

sgtcoolguy

I think this is a very nice API addition that could be very helpful for app developers. Thanks, @drauggres !

My first concern is that the path set is only a relative path and doesn't offer control whether the cache gets put in the external cache or the app cache dir, which can have security/data privacy issues. I think the property should be the full path so the dev has control to explicitly choose that (though perhaps it could default to app cache directory or something). To enable that we may need to expose the external cache dir in our APIs.

I'm also concerned that this is very Android-specific, as I believe iOS could also implement something similar as well here.

@hansemannn Can you please take a look and see if we couldn't make this a more general API cross-platform? Maybe hang it off Ti.Network.Cache or something?

sgtcoolguy · 2018-07-23T14:26:10Z

.../android/src/java/ti/modules/titanium/android/httpresponsecache/HttpResponseCacheModule.java

+	// clang-format off
+	@Kroll.method
+	@Kroll.setProperty
+	public void setHttpCacheSize(long value)


setMaxSize(long value)?

sgtcoolguy · 2018-07-23T14:26:52Z

.../android/src/java/ti/modules/titanium/android/httpresponsecache/HttpResponseCacheModule.java

+	// clang-format off
+	@Kroll.method
+	@Kroll.getProperty
+	public String getHttpCachePath()


getPath()? Remove the @Kroll.method annotation (and then the clang-format comments are unnecessary too)?

sgtcoolguy · 2018-07-23T14:28:01Z

.../android/src/java/ti/modules/titanium/android/httpresponsecache/HttpResponseCacheModule.java

+		if (cache != null) {
+			return cache.size();
+		}
+		return 0;


Should this return -1? Or do we want to have differing values for "no cache installed" versus "cache installed, but unable to determine size"?

Or do we want to have differing values for "no cache installed" versus "cache installed, but unable to determine size"?

I think, we shouldn't mix it.

sgtcoolguy · 2018-07-23T14:31:17Z

android/titanium/src/java/org/appcelerator/titanium/view/TiDrawableReference.java

@@ -163,7 +165,14 @@ public static TiDrawableReference fromUrl(KrollProxy proxy, String url)
 		if (url == null || url.length() == 0 || url.trim().length() == 0) {
 			return new TiDrawableReference(proxy.getActivity(), DrawableReferenceType.NULL);
 		}
-		return fromUrl(proxy.getActivity(), proxy.resolveUrl(null, url));
+		boolean useCaches = false;


You can shorten this to:

boolean useCaches = TiConvert.toBoolean(proxy.getProperty(TiC.PROPERTY_CACHE), false);

sgtcoolguy · 2018-07-23T14:31:36Z

android/titanium/src/java/org/appcelerator/titanium/view/TiDrawableReference.java

@@ -239,8 +248,15 @@ public static TiDrawableReference fromObject(KrollProxy proxy, Object object)
 			object = proxy.resolveUrl(null, (String) object);
 		}

+		boolean useCaches = false;


Same as above, you can shrink this down to a one-liner

sgtcoolguy · 2018-07-23T14:34:50Z

apidoc/Titanium/Android/HttpResponseCache.yml

+extends: Titanium.Proxy
+platforms: [android]
+createable: false
+since: "7.1.0"


Sorry, we clearly missed this release. This will need to be updated for whatever release we can get this in. master is currently targeting 7.4.0, but I think feature freeze may be happening for that real soon.

Changed to 7.4.0

sgtcoolguy · 2018-07-23T14:35:56Z

apidoc/Titanium/Android/HttpResponseCache.yml

+
+  - name: httpCacheSize
+    summary: Maximum  cache size in bytes.
+    description: |


Maye want to add some details/notes that this needs to be set before #install() and once that's called this can't be changed.

Add this note:

To apply changes of this property to existing HttpResponseCache instance, you should call [install](Titanium.Android.HttpResponseCache.install) method.

sgtcoolguy · 2018-07-23T14:37:04Z

apidoc/Titanium/Android/HttpResponseCache.yml

+  - name: httpCachePath
+    summary: Relative path for cache
+    description: |
+        HTTP response cache will be stored on external storage if it exists on moment


Same as above, maybe more more explicit that this property is consulted at time #install() is called, but changing later won't do anything. (In fact we may even want to spit out some warning logs in the impl if they get called after #install()).

Add this note:

To apply changes of this property to existing HttpResponseCache instance, you should call [install](Titanium.Android.HttpResponseCache.install) method.

sgtcoolguy · 2018-07-23T14:45:26Z

.../android/src/java/ti/modules/titanium/android/httpresponsecache/HttpResponseCacheModule.java

+			cache.close();
+		}
+		TiApplication tiApp = TiApplication.getInstance();
+		File dir = tiApp.getApplicationContext().getExternalCacheDir();


Why not have the path (or httpCacheDir) property value determine the directory and relative path to store the cache?

I'm worried here because the Android docs point out that if the requests contain private data, the external cache dir is a bad place to stick the cache due to access controls not being enforced on external storage.

I'm assuming it's because we don't actually expose the external cache dir anywhere in our APIs? cc @garymathews @jquick-axway

I'm assuming it's because we don't actually expose the external cache dir anywhere in our APIs?

Yes. Also this is how currently cacheDir calculated for TiResponseCache.

drauggres · 2018-07-24T11:49:39Z

@sgtcoolguy Thanks for thorough code review.

My first concern is that the path set is only a relative path and doesn't offer control whether the cache gets put in the external cache or the app cache dir, which can have security/data privacy issues. I think the property should be the full path so the dev has control to explicitly choose that (though perhaps it could default to app cache directory or something). To enable that we may need to expose the external cache dir in our APIs.

What about additional boolean property (smth like preferExternalCacheDir) with default value false? It will be much easier to implement.

jquick-axway · 2018-10-31T01:13:13Z

I have to agree with @hansemannn comment here that the API to control the response cache (deletion, size, location, etc.) should be redesigned to so that it can work on both Android and iOS.

I also don't like the idea of replacing our TiResponseCache with Google's HttpResponseCache. We've added additional APIs to our class to have better control over it and to better handle "redirect" cache handling. Whatever feature we're missing should be added to TiResponseCache.

I do agree that we should add "cache" property support to HTTPClient. That is best done via a separate simpler PR. I've already written a ticket for it here: TIMOB-26501

I'd prefer to keep our ImageView simple. Especially from a portability standpoint. If you want greater control over HTTP handling when loading images, then that is best done via our HTTPClient and the downloaded image can then be applied to ImageView.

drauggres · 2019-01-11T09:46:26Z

Resume:

Ti.Network.HTTPClient cache property on Android: see TIMOB-26501
image cache control: load remote images via HTTPClient and control caching manually (or use To.ImageCache)
general HttpResponseCache control and statistics: need to implement it in TiResponseCache

build added android community 🔥 docs needs tests 🚨 labels Jan 11, 2018

drauggres force-pushed the TIMOB-25663 branch from a1c18f6 to 9f0a469 Compare January 11, 2018 16:18

build added android community 🔥 docs needs tests 🚨 labels Jan 11, 2018

drauggres force-pushed the TIMOB-25663 branch from 9f0a469 to 8b37968 Compare January 15, 2018 13:41

drauggres force-pushed the TIMOB-25663 branch from 8b37968 to 8aaab57 Compare February 2, 2018 08:17

build added android community 🔥 docs and removed needs tests 🚨 labels Feb 2, 2018

drauggres added 5 commits February 13, 2018 15:09

[TIMOB-25663] Android. Add HttpResponseCache module

c261949

[TIMOB-25663] Android. Add HttpClient.cache property

95e30c4

[TIMOB-25663] Android. Add ImageView.cache property

7ee8202

[TIMOB-25663] expose module properties

0ae09c5

[TIMOB-25663] add tests

b57ee3e

drauggres force-pushed the TIMOB-25663 branch from 8aaab57 to b57ee3e Compare February 13, 2018 12:46

build added android community 🔥 docs labels Feb 13, 2018

Merge branch 'master' into TIMOB-25663

6800cbc

hansemannn added this to the 7.4.0 milestone Jun 28, 2018

Merge branch 'master' into TIMOB-25663

3e439bf

sgtcoolguy reviewed Jul 23, 2018

View reviewed changes

sgtcoolguy suggested changes Jul 23, 2018

View reviewed changes

sgtcoolguy requested a review from hansemannn July 23, 2018 14:48

drauggres force-pushed the TIMOB-25663 branch from 3e439bf to 771466a Compare July 23, 2018 15:47

drauggres added 4 commits July 23, 2018 19:54

[TIMOB-25663] Android: rename HttpResponseCache props

6385e65

[TIMOB-25663] Android: fix HttpResponseCache tests

c89a3f6

[TIMOB-25663] Android: simplify code

c0285f7

Merge branch 'master' into TIMOB-25663

c098c62

drauggres force-pushed the TIMOB-25663 branch from 771466a to c098c62 Compare July 23, 2018 16:55

[TIMOB-25663] Android: HttpResponseCache docs and test

a74f545

hansemannn modified the milestones: 7.4.0, 7.5.0 Aug 24, 2018

lokeshchdhry requested review from jquick-axway and garymathews October 12, 2018 21:47

jquick-axway modified the milestones: 7.5.0, 8.0.0 Oct 29, 2018

sgtcoolguy added the merge conflicts 🚨 label Oct 30, 2018

drauggres closed this Jan 11, 2019

[TIMOB-25663] Android: HttpClient.cache. ImageView.cache #9719

[TIMOB-25663] Android: HttpClient.cache. ImageView.cache #9719

Conversation

drauggres commented Jan 11, 2018

build commented Jan 11, 2018 • edited

hansemannn commented Jun 28, 2018

build commented Jun 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgtcoolguy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drauggres commented Jul 24, 2018

jquick-axway commented Oct 31, 2018

drauggres commented Jan 11, 2019

build commented Jan 11, 2018 •

edited

build commented Jun 28, 2018 •

edited