Issue 29401555: Issue 5088 - Remove code duplicate for refresh subscriptions (Closed)
|
Created:
April 3, 2017, 6:13 a.m. by anton Modified:
April 28, 2017, 10:22 a.m. CC:
René Jeschke Visibility:
Public. |
DescriptionIssue 5088 - Remove code duplicate for refresh subscriptions
Patch Set 1 #
Total comments: 4
Patch Set 2 : using "update" instead of "refresh" terminology #MessagesTotal messages: 7
https://codereview.adblockplus.org/29401555/diff/29401556/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (left): https://codereview.adblockplus.org/29401555/diff/29401556/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:46: import android.content.pm.PackageManager.NameNotFoundException; not related by the task, but desired clean-up https://codereview.adblockplus.org/29401555/diff/29401556/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:254: private static final int UPDATE_DELAY_MS = 1 * 1000; I'm not sure that 'refresh' is better than 'update' for subscriptions updating but we had 'refreshSubscriptions()' method in original AdblockEngine class. Also if someone already uses libadblockplus-android API it's preferred not to rename existing methods. So i use ''refresh' everywhere below in terms of 'update' subscriptions.
https://codereview.adblockplus.org/29401555/diff/29401556/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (left): https://codereview.adblockplus.org/29401555/diff/29401556/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:254: private static final int UPDATE_DELAY_MS = 1 * 1000; On 2017/04/03 06:17:54, anton wrote: > I'm not sure that 'refresh' is better than 'update' for subscriptions updating > but we had 'refreshSubscriptions()' method in original AdblockEngine class. Also > if someone already uses libadblockplus-android API it's preferred not to rename > existing methods. > > So i use ''refresh' everywhere below in terms of 'update' subscriptions. Well, I think that update is a better term in this case than refresh. For someone already used to the API, maybe refresh makes more sense, but for someone new to it, update would be easier to understand. I wouldn't insist, but I would prefer update to be used.
On 2017/04/07 09:36:54, diegocarloslima wrote: > https://codereview.adblockplus.org/29401555/diff/29401556/libadblockplus-andr... > File > libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java > (left): > > https://codereview.adblockplus.org/29401555/diff/29401556/libadblockplus-andr... > libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:254: > private static final int UPDATE_DELAY_MS = 1 * 1000; > On 2017/04/03 06:17:54, anton wrote: > > I'm not sure that 'refresh' is better than 'update' for subscriptions updating > > but we had 'refreshSubscriptions()' method in original AdblockEngine class. > Also > > if someone already uses libadblockplus-android API it's preferred not to > rename > > existing methods. > > > > So i use ''refresh' everywhere below in terms of 'update' subscriptions. > > Well, I think that update is a better term in this case than refresh. For > someone already used to the API, maybe refresh makes more sense, but for someone > new to it, update would be easier to understand. I wouldn't insist, but I would > prefer update to be used. Yes, i prefer 'update' too. Let wait for Felix decision
On 2017/04/07 09:37:57, anton wrote: > On 2017/04/07 09:36:54, diegocarloslima wrote: > > > https://codereview.adblockplus.org/29401555/diff/29401556/libadblockplus-andr... > > File > > > libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java > > (left): > > > > > https://codereview.adblockplus.org/29401555/diff/29401556/libadblockplus-andr... > > > libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:254: > > private static final int UPDATE_DELAY_MS = 1 * 1000; > > On 2017/04/03 06:17:54, anton wrote: > > > I'm not sure that 'refresh' is better than 'update' for subscriptions > updating > > > but we had 'refreshSubscriptions()' method in original AdblockEngine class. > > Also > > > if someone already uses libadblockplus-android API it's preferred not to > > rename > > > existing methods. > > > > > > So i use ''refresh' everywhere below in terms of 'update' subscriptions. > > > > Well, I think that update is a better term in this case than refresh. For > > someone already used to the API, maybe refresh makes more sense, but for > someone > > new to it, update would be easier to understand. I wouldn't insist, but I > would > > prefer update to be used. > > Yes, i prefer 'update' too. Let wait for Felix decision LGTM, although as I stated before, I prefer the update term
Looks good, but I guess it'll need some changing after my comment. https://codereview.adblockplus.org/29401555/diff/29401556/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (left): https://codereview.adblockplus.org/29401555/diff/29401556/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:254: private static final int UPDATE_DELAY_MS = 1 * 1000; On 2017/04/07 09:36:54, diegocarloslima wrote: > On 2017/04/03 06:17:54, anton wrote: > > I'm not sure that 'refresh' is better than 'update' for subscriptions updating > > but we had 'refreshSubscriptions()' method in original AdblockEngine class. > Also > > if someone already uses libadblockplus-android API it's preferred not to > rename > > existing methods. > > > > So i use ''refresh' everywhere below in terms of 'update' subscriptions. > > Well, I think that update is a better term in this case than refresh. For > someone already used to the API, maybe refresh makes more sense, but for someone > new to it, update would be easier to understand. I wouldn't insist, but I would > prefer update to be used. I'd also go for "update", since that's the terminology we use in Libadblockplus as well (there is no function for updating all subscriptions, but there is Subscription::UpdateFilters). Since libadblockplus-android is relatively new (the only user we're aware of is ZTE), we can probably still safely break the API.
On 2017/04/10 11:59:47, Felix Dahlke wrote: > Looks good, but I guess it'll need some changing after my comment. > > https://codereview.adblockplus.org/29401555/diff/29401556/libadblockplus-andr... > File > libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java > (left): > > https://codereview.adblockplus.org/29401555/diff/29401556/libadblockplus-andr... > libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:254: > private static final int UPDATE_DELAY_MS = 1 * 1000; > On 2017/04/07 09:36:54, diegocarloslima wrote: > > On 2017/04/03 06:17:54, anton wrote: > > > I'm not sure that 'refresh' is better than 'update' for subscriptions > updating > > > but we had 'refreshSubscriptions()' method in original AdblockEngine class. > > Also > > > if someone already uses libadblockplus-android API it's preferred not to > > rename > > > existing methods. > > > > > > So i use ''refresh' everywhere below in terms of 'update' subscriptions. > > > > Well, I think that update is a better term in this case than refresh. For > > someone already used to the API, maybe refresh makes more sense, but for > someone > > new to it, update would be easier to understand. I wouldn't insist, but I > would > > prefer update to be used. > > I'd also go for "update", since that's the terminology we use in Libadblockplus > as well (there is no function for updating all subscriptions, but there is > Subscription::UpdateFilters). > > Since libadblockplus-android is relatively new (the only user we're aware of is > ZTE), we can probably still safely break the API. Uploaded new patch set
LGTM 
|
