Issue 29673565: Issue 6066 - Implement 'force update' functionality for subscriptions (Closed)
DescriptionAs I had no mockups or detailed specifications for this feature but there seems to be a significant amount of users asking for it, I've implemented it the way I think it's fine.
Patch Set 1 : #
Total comments: 11
Patch Set 2 : Fixed typos and line length #
Total comments: 10
Patch Set 3 : Fixes #
Total comments: 3
MessagesTotal messages: 14
https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java (right): https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Subscription.java:67: private static final long MINIMAL_DOWNLOAD_INTERVAL = DateUtils.MINUTE_IN_MILLIS; I double-checked with felix and matze that a decrease of this threshold is fine.
https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java (right): https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:262: .setMessage(hasInternet ? i prefer long ?: to look like this: condition ? do1 : do2 for readability. Not insisting. https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Downloader.java (right): https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Downloader.java:112: public void enqueueDownload(final URL url, final String id, final Map<String, String> headers, final boolean allowMetered) the line seems to be too long https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Engine.java (right): https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:793: public void enqueueDownload(final Subscription sub, final boolean forced, final boolean allowMetered) throws IOException the line seems to be too long https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java (right): https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java:26: public static boolean hasWifiConncetion(final Context context) typo: `Conncetion` https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java:34: public static boolean hasInternetConncetion(final Context context) same here
https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java (right): https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:262: .setMessage(hasInternet ? On 2018/01/19 10:53:02, anton wrote: > i prefer long ?: to look like this: > > condition > ? do1 > : do2 > for readability. Not insisting. Acknowledged. https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Engine.java (right): https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Engine.java:793: public void enqueueDownload(final Subscription sub, final boolean forced, final boolean allowMetered) throws IOException On 2018/01/19 10:53:02, anton wrote: > the line seems to be too long Acknowledged. https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java (right): https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java:26: public static boolean hasWifiConncetion(final Context context) On 2018/01/19 10:53:02, anton wrote: > typo: `Conncetion` Acknowledged. https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java:34: public static boolean hasInternetConncetion(final Context context) On 2018/01/19 10:53:02, anton wrote: > same here Acknowledged.
https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Downloader.java (right): https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Downloader.java:112: public void enqueueDownload(final URL url, final String id, final Map<String, String> headers, final boolean allowMetered) On 2018/01/19 10:53:02, anton wrote: > the line seems to be too long Acknowledged.
On 2018/01/19 11:00:47, jens wrote: > https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... > File > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Downloader.java > (right): > > https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Downloader.java:112: > public void enqueueDownload(final URL url, final String id, final Map<String, > String> headers, final boolean allowMetered) > On 2018/01/19 10:53:02, anton wrote: > > the line seems to be too long > > Acknowledged. Uploaded a new patch set
On 2018/01/19 11:02:55, jens wrote: > On 2018/01/19 11:00:47, jens wrote: > > > https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... > > File > > > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Downloader.java > > (right): > > > > > https://codereview.adblockplus.org/29673565/diff/29673617/adblockplussbrowser... > > > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Downloader.java:112: > > public void enqueueDownload(final URL url, final String id, final Map<String, > > String> headers, final boolean allowMetered) > > On 2018/01/19 10:53:02, anton wrote: > > > the line seems to be too long > > > > Acknowledged. > > Uploaded a new patch set LGTM, though not diving too deep in classes interaction
https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... File adblockplussbrowser/res/values/sysstrings.xml (right): https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... adblockplussbrowser/res/values/sysstrings.xml:16: <string name="key_update_subscriptions">force_update_subscriptions</string> Usually the value is the name without 'key_'. I would suggest to either rename the key to 'key_force_update_subscriptions' or the value to 'update_subscriptions' in order to keep consistency with the existing fields. https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java (right): https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:249: public boolean onPreferenceClick(Preference preference) So far, instead of adding specific preference logic here, we extend a preference class and add the specific behavior on each custom preference class. This helps to avoid largely increasing the size of this file with per-preference logic https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Downloader.java (right): https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Downloader.java:171: if (this.downloader.engine.canUseInternet() || job.allowMetered) I know that there's a check in MainPreferences which prevents a download to be added if there's no internet here, but in the future this can be a source of problems, because the name suggests that it will be downloaded if there's metered connection, but it will try to download even if the user doesn't currently have internet access (when allowMetered is true but there's no internet connectivity) Maybe the Engine.canUseInternet should be moved/combined to the new ConnectivityUtils class and have two definitions canUseInternet() canUseInternet(allowMetered) -> which would override the wifiOnly (key_automatic_updates) preference https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java (right): https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java:31: return current != null && current.getType() == ConnectivityManager.TYPE_WIFI; I think its interesting to also consider Bluetooth, Ethernet, WiMax as NonMetered connections. As I suggested in the other file, maybe you could move the Engine.canUseInternet logic here and possibly merge with this method https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java:38: return connManager != null && connManager.getActiveNetworkInfo() != null; Seems that connManager.getActiveNetworkInfo().isConnectedOrConnecting(); condition is missing here
https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... File adblockplussbrowser/res/values/sysstrings.xml (right): https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... adblockplussbrowser/res/values/sysstrings.xml:16: <string name="key_update_subscriptions">force_update_subscriptions</string> On 2018/01/19 13:59:00, diegocarloslima wrote: > Usually the value is the name without 'key_'. I would suggest to either rename > the key to 'key_force_update_subscriptions' or the value to > 'update_subscriptions' in order to keep consistency with the existing fields. Acknowledged. https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java (right): https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:249: public boolean onPreferenceClick(Preference preference) On 2018/01/19 13:59:00, diegocarloslima wrote: > So far, instead of adding specific preference logic here, we extend a preference > class and add the specific behavior on each custom preference class. This helps > to avoid largely increasing the size of this file with per-preference logic Right, but we only did that for those preferences, that open a new screen. For toggling AA and the automatic update rules, we used MainPreferences. The force update also not opens a new screen but only shows a dialog (like automatic updates). Because of that I would prefer to implement the listener and keep the logic in MainPreferences for now. https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Downloader.java (right): https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Downloader.java:171: if (this.downloader.engine.canUseInternet() || job.allowMetered) On 2018/01/19 13:59:00, diegocarloslima wrote: > I know that there's a check in MainPreferences which prevents a download to be > added if there's no internet here, but in the future this can be a source of > problems, because the name suggests that it will be downloaded if there's > metered connection, but it will try to download even if the user doesn't > currently have internet access (when allowMetered is true but there's no > internet connectivity) > > > Maybe the Engine.canUseInternet should be moved/combined to the new > ConnectivityUtils class and have two definitions > canUseInternet() > canUseInternet(allowMetered) -> which would override the wifiOnly > (key_automatic_updates) preference I was already considering to move canUseInternet to the ConnectivityUtils class, but I thought that should be done in a separate ticket, as canUseInternet needs a context (which we currently don't have in Downloader.java). https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java (right): https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java:31: return current != null && current.getType() == ConnectivityManager.TYPE_WIFI; On 2018/01/19 13:59:00, diegocarloslima wrote: > I think its interesting to also consider Bluetooth, Ethernet, WiMax as > NonMetered connections. As I suggested in the other file, maybe you could move > the Engine.canUseInternet logic here and possibly merge with this method Acknowledged. https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java:38: return connManager != null && connManager.getActiveNetworkInfo() != null; On 2018/01/19 13:59:00, diegocarloslima wrote: > Seems that connManager.getActiveNetworkInfo().isConnectedOrConnecting(); > condition is missing here Acknowledged.
On 2018/01/19 14:32:56, jens wrote: > https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... > File adblockplussbrowser/res/values/sysstrings.xml (right): > > https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... > adblockplussbrowser/res/values/sysstrings.xml:16: <string > name="key_update_subscriptions">force_update_subscriptions</string> > On 2018/01/19 13:59:00, diegocarloslima wrote: > > Usually the value is the name without 'key_'. I would suggest to either rename > > the key to 'key_force_update_subscriptions' or the value to > > 'update_subscriptions' in order to keep consistency with the existing fields. > > Acknowledged. > > https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... > File > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java > (right): > > https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:249: > public boolean onPreferenceClick(Preference preference) > On 2018/01/19 13:59:00, diegocarloslima wrote: > > So far, instead of adding specific preference logic here, we extend a > preference > > class and add the specific behavior on each custom preference class. This > helps > > to avoid largely increasing the size of this file with per-preference logic > > Right, but we only did that for those preferences, that open a new screen. For > toggling AA and the automatic update rules, we used MainPreferences. The force > update also not opens a new screen but only shows a dialog (like automatic > updates). Because of that I would prefer to implement the listener and keep the > logic in MainPreferences for now. > > https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... > File > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Downloader.java > (right): > > https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/Downloader.java:171: > if (this.downloader.engine.canUseInternet() || job.allowMetered) > On 2018/01/19 13:59:00, diegocarloslima wrote: > > I know that there's a check in MainPreferences which prevents a download to be > > added if there's no internet here, but in the future this can be a source of > > problems, because the name suggests that it will be downloaded if there's > > metered connection, but it will try to download even if the user doesn't > > currently have internet access (when allowMetered is true but there's no > > internet connectivity) > > > > > > Maybe the Engine.canUseInternet should be moved/combined to the new > > ConnectivityUtils class and have two definitions > > canUseInternet() > > canUseInternet(allowMetered) -> which would override the wifiOnly > > (key_automatic_updates) preference > > I was already considering to move canUseInternet to the ConnectivityUtils class, > but I thought that should be done in a separate ticket, as canUseInternet needs > a context (which we currently don't have in Downloader.java). > > https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... > File > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java > (right): > > https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java:31: > return current != null && current.getType() == ConnectivityManager.TYPE_WIFI; > On 2018/01/19 13:59:00, diegocarloslima wrote: > > I think its interesting to also consider Bluetooth, Ethernet, WiMax as > > NonMetered connections. As I suggested in the other file, maybe you could move > > the Engine.canUseInternet logic here and possibly merge with this method > > Acknowledged. > > https://codereview.adblockplus.org/29673565/diff/29674574/adblockplussbrowser... > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java:38: > return connManager != null && connManager.getActiveNetworkInfo() != null; > On 2018/01/19 13:59:00, diegocarloslima wrote: > > Seems that connManager.getActiveNetworkInfo().isConnectedOrConnecting(); > > condition is missing here > > Acknowledged. Uploaded a new patch set
https://codereview.adblockplus.org/29673565/diff/29677714/adblockplussbrowser... File adblockplussbrowser/res/values/strings.xml (right): https://codereview.adblockplus.org/29673565/diff/29677714/adblockplussbrowser... adblockplussbrowser/res/values/strings.xml:58: <string name="check_your_connection">It looks like you are offline. Please check your internet connection.</string> Tamara asked me to change this to a small 'i'
https://codereview.adblockplus.org/29673565/diff/29677714/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java (right): https://codereview.adblockplus.org/29673565/diff/29677714/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java:50: public static boolean canUseInternet(final Context serviceContext, final boolean acceptMetered) Small thing, to make it more generic I think that the Context param should be called only 'context' instead of 'serviceContext'
On 2018/01/23 17:37:17, diegocarloslima wrote: > https://codereview.adblockplus.org/29673565/diff/29677714/adblockplussbrowser... > File > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java > (right): > > https://codereview.adblockplus.org/29673565/diff/29677714/adblockplussbrowser... > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java:50: > public static boolean canUseInternet(final Context serviceContext, final boolean > acceptMetered) > Small thing, to make it more generic I think that the Context param should be > called only 'context' instead of 'serviceContext' LGTM, with a small remark in ConectivityUtils.canUseInternet()
On 2018/01/23 17:38:18, diegocarloslima wrote: > On 2018/01/23 17:37:17, diegocarloslima wrote: > > > https://codereview.adblockplus.org/29673565/diff/29677714/adblockplussbrowser... > > File > > > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java > > (right): > > > > > https://codereview.adblockplus.org/29673565/diff/29677714/adblockplussbrowser... > > > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java:50: > > public static boolean canUseInternet(final Context serviceContext, final > boolean > > acceptMetered) > > Small thing, to make it more generic I think that the Context param should be > > called only 'context' instead of 'serviceContext' > > LGTM, with a small remark in ConectivityUtils.canUseInternet() Okay, I will rename it.
https://codereview.adblockplus.org/29673565/diff/29677714/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java (right): https://codereview.adblockplus.org/29673565/diff/29677714/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/util/ConnectivityUtils.java:50: public static boolean canUseInternet(final Context serviceContext, final boolean acceptMetered) On 2018/01/23 17:37:16, diegocarloslima wrote: > Small thing, to make it more generic I think that the Context param should be > called only 'context' instead of 'serviceContext' Acknowledged. 
|
