Issue 29375826: Issue 4903 - Allow to specify file system root (Closed)
|
Created:
Feb. 16, 2017, 11:41 a.m. by anton Modified:
March 17, 2017, 8:26 p.m. CC:
Felix Dahlke, sergei Visibility:
Public. |
DescriptionIssue 4903 - Allow to specify file system root
Patch Set 1 #
Total comments: 5
Patch Set 2 : updated README #
Total comments: 3
Patch Set 3 : removed init(), added doc #
MessagesTotal messages: 14
https://codereview.adblockplus.org/29375826/diff/29375827/libadblockplus-andr... File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29375826/diff/29375827/libadblockplus-andr... libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:110: public void init(Context context, boolean developmentBuild, String preferenceName) for back compatibility. do we need it?
https://codereview.adblockplus.org/29375826/diff/29375827/libadblockplus-andr... File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29375826/diff/29375827/libadblockplus-andr... libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:112: init(context, context.getCacheDir().getAbsolutePath(), developmentBuild, preferenceName); for back compatibility it's better to getCacheDir(). but because app cache can be cleared by the system and we don't want it it would be better to pass https://developer.android.com/reference/android/content/Context.html#getDataD.... what would you prefer?
https://codereview.adblockplus.org/29375826/diff/29375827/libadblockplus-andr... File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29375826/diff/29375827/libadblockplus-andr... libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:112: init(context, context.getCacheDir().getAbsolutePath(), developmentBuild, preferenceName); On 2017/02/16 11:47:15, anton wrote: > for back compatibility it's better to getCacheDir(). but because app cache can > be cleared by the system and we don't want it it would be better to pass > https://developer.android.com/reference/android/content/Context.html#getDataD.... > what would you prefer? getDataDir is available starting api 24 only. probably we should use https://developer.android.com/reference/android/content/Context.html#getFiles... instead
LGTM
https://codereview.adblockplus.org/29375826/diff/29375827/libadblockplus-andr... File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29375826/diff/29375827/libadblockplus-andr... libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:110: public void init(Context context, boolean developmentBuild, String preferenceName) On 2017/02/16 11:43:44, anton wrote: > for back compatibility. do we need it? Could it be marked as deprecated and I think we should also update readme file.
https://codereview.adblockplus.org/29375826/diff/29375827/libadblockplus-andr... File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29375826/diff/29375827/libadblockplus-andr... libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:110: public void init(Context context, boolean developmentBuild, String preferenceName) On 2017/03/13 08:49:23, sergei wrote: > On 2017/02/16 11:43:44, anton wrote: > > for back compatibility. do we need it? > > Could it be marked as deprecated and I think we should also update readme file. It could be marked with @Deprecated java annotation, but i would not say the method is deprecated. I will update README file
https://codereview.adblockplus.org/29375826/diff/29382559/libadblockplus-andr... File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29375826/diff/29382559/libadblockplus-andr... libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:112: init(context, context.getCacheDir().getAbsolutePath(), developmentBuild, preferenceName); As you already pointed, it would be better to use getFilesDir() here, since the cache can be cleared out by the system
https://codereview.adblockplus.org/29375826/diff/29382559/libadblockplus-andr... File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29375826/diff/29382559/libadblockplus-andr... libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:112: init(context, context.getCacheDir().getAbsolutePath(), developmentBuild, preferenceName); On 2017/03/13 13:58:11, diegocarloslima wrote: > As you already pointed, it would be better to use getFilesDir() here, since the > cache can be cleared out by the system Well, i'd prefer to remove it at all or leave as-is because of back-compatibility. I don't see any reason for any app "default" directories (like getCacheDir() or getFilesDir()) to be the only suitable and reasonable directory for the files so i believe the user should make the decision himself.
https://codereview.adblockplus.org/29375826/diff/29382559/libadblockplus-andr... File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29375826/diff/29382559/libadblockplus-andr... libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:112: init(context, context.getCacheDir().getAbsolutePath(), developmentBuild, preferenceName); On 2017/03/14 04:15:26, anton wrote: > On 2017/03/13 13:58:11, diegocarloslima wrote: > > As you already pointed, it would be better to use getFilesDir() here, since > the > > cache can be cleared out by the system > > Well, i'd prefer to remove it at all or leave as-is because of > back-compatibility. > I don't see any reason for any app "default" directories (like getCacheDir() or > getFilesDir()) to be the only suitable and reasonable directory for the files so > i believe the user should make the decision himself. I'm not against removing it, but if we decide to keep it, I would vote for changing it, because we want to provide the best blocking experience possible, and storing on cache directory which could be cleared by the system, can compromise that
On 2017/03/16 19:55:24, diegocarloslima wrote: > https://codereview.adblockplus.org/29375826/diff/29382559/libadblockplus-andr... > File > libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java > (right): > > https://codereview.adblockplus.org/29375826/diff/29382559/libadblockplus-andr... > libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:112: > init(context, context.getCacheDir().getAbsolutePath(), developmentBuild, > preferenceName); > On 2017/03/14 04:15:26, anton wrote: > > On 2017/03/13 13:58:11, diegocarloslima wrote: > > > As you already pointed, it would be better to use getFilesDir() here, since > > the > > > cache can be cleared out by the system > > > > Well, i'd prefer to remove it at all or leave as-is because of > > back-compatibility. > > I don't see any reason for any app "default" directories (like getCacheDir() > or > > getFilesDir()) to be the only suitable and reasonable directory for the files > so > > i believe the user should make the decision himself. > > I'm not against removing it, but if we decide to keep it, I would vote for > changing it, because we want to provide the best blocking experience possible, > and storing on cache directory which could be cleared by the system, can > compromise that Let's just remove init() with default path?
On 2017/03/17 06:48:01, anton wrote: > On 2017/03/16 19:55:24, diegocarloslima wrote: > > > https://codereview.adblockplus.org/29375826/diff/29382559/libadblockplus-andr... > > File > > > libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java > > (right): > > > > > https://codereview.adblockplus.org/29375826/diff/29382559/libadblockplus-andr... > > > libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:112: > > init(context, context.getCacheDir().getAbsolutePath(), developmentBuild, > > preferenceName); > > On 2017/03/14 04:15:26, anton wrote: > > > On 2017/03/13 13:58:11, diegocarloslima wrote: > > > > As you already pointed, it would be better to use getFilesDir() here, > since > > > the > > > > cache can be cleared out by the system > > > > > > Well, i'd prefer to remove it at all or leave as-is because of > > > back-compatibility. > > > I don't see any reason for any app "default" directories (like getCacheDir() > > or > > > getFilesDir()) to be the only suitable and reasonable directory for the > files > > so > > > i believe the user should make the decision himself. > > > > I'm not against removing it, but if we decide to keep it, I would vote for > > changing it, because we want to provide the best blocking experience possible, > > and storing on cache directory which could be cleared by the system, can > > compromise that > > Let's just remove init() with default path? I'm OK with that
On 2017/03/17 13:13:10, diegocarloslima wrote: > On 2017/03/17 06:48:01, anton wrote: > > On 2017/03/16 19:55:24, diegocarloslima wrote: > > > > > > https://codereview.adblockplus.org/29375826/diff/29382559/libadblockplus-andr... > > > File > > > > > > libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java > > > (right): > > > > > > > > > https://codereview.adblockplus.org/29375826/diff/29382559/libadblockplus-andr... > > > > > > libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:112: > > > init(context, context.getCacheDir().getAbsolutePath(), developmentBuild, > > > preferenceName); > > > On 2017/03/14 04:15:26, anton wrote: > > > > On 2017/03/13 13:58:11, diegocarloslima wrote: > > > > > As you already pointed, it would be better to use getFilesDir() here, > > since > > > > the > > > > > cache can be cleared out by the system > > > > > > > > Well, i'd prefer to remove it at all or leave as-is because of > > > > back-compatibility. > > > > I don't see any reason for any app "default" directories (like > getCacheDir() > > > or > > > > getFilesDir()) to be the only suitable and reasonable directory for the > > files > > > so > > > > i believe the user should make the decision himself. > > > > > > I'm not against removing it, but if we decide to keep it, I would vote for > > > changing it, because we want to provide the best blocking experience > possible, > > > and storing on cache directory which could be cleared by the system, can > > > compromise that > > > > Let's just remove init() with default path? > > I'm OK with that see patch set #3
On 2017/03/17 19:37:51, anton wrote: > On 2017/03/17 13:13:10, diegocarloslima wrote: > > On 2017/03/17 06:48:01, anton wrote: > > > On 2017/03/16 19:55:24, diegocarloslima wrote: > > > > > > > > > > https://codereview.adblockplus.org/29375826/diff/29382559/libadblockplus-andr... > > > > File > > > > > > > > > > libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java > > > > (right): > > > > > > > > > > > > > > https://codereview.adblockplus.org/29375826/diff/29382559/libadblockplus-andr... > > > > > > > > > > libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:112: > > > > init(context, context.getCacheDir().getAbsolutePath(), developmentBuild, > > > > preferenceName); > > > > On 2017/03/14 04:15:26, anton wrote: > > > > > On 2017/03/13 13:58:11, diegocarloslima wrote: > > > > > > As you already pointed, it would be better to use getFilesDir() here, > > > since > > > > > the > > > > > > cache can be cleared out by the system > > > > > > > > > > Well, i'd prefer to remove it at all or leave as-is because of > > > > > back-compatibility. > > > > > I don't see any reason for any app "default" directories (like > > getCacheDir() > > > > or > > > > > getFilesDir()) to be the only suitable and reasonable directory for the > > > files > > > > so > > > > > i believe the user should make the decision himself. > > > > > > > > I'm not against removing it, but if we decide to keep it, I would vote for > > > > changing it, because we want to provide the best blocking experience > > possible, > > > > and storing on cache directory which could be cleared by the system, can > > > > compromise that > > > > > > Let's just remove init() with default path? > > > > I'm OK with that > > see patch set #3 LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
