Issue 2720 - [Adblocking settings] Add the other filter lists category

mobile/android/thirdparty/org/adblockplus/browser/UrlInputDialog.java

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 19 matching lines @@
 import android.view.View.OnClickListener;
 import android.view.inputmethod.EditorInfo;
 import android.widget.Button;
 import android.widget.EditText;
 import android.widget.TextView;
 import android.widget.TextView.OnEditorActionListener;
 
 public class UrlInputDialog extends Dialog implements TextWatcher,
     OnEditorActionListener, OnClickListener
 {
-  private static final int BUTTON_ENABLED_COLOR = 0xff0099cc;

[Felix Dahlke, 2015/07/26 17:06:23]

As you know, I don't think we should have hard coded (i.e., in Java code) style
values - how about moving this to XML? Apart from all the mechanisms Android
provides to customise this for other environments and the benefits of having all
this stuff in one place, it's a much less risky change to modify it in an XML
file than to mess with the Java code, and such meddling won't distract from
actual behaviour changes when looking for regressions etc.

[René Jeschke, 2015/07/28 10:55:27]

Done.

-  private static final int BUTTON_DISABLED_COLOR = 0xffc5dfe8;
   private final Type type;
   private final UrlReadyCallback callback;
 
   private EditText editText = null;
   private Button button = null;
 
   private boolean submitEnabled = false;
   private boolean triedSubmit = false;
   private String currentEditTextValue = null;
 
   public enum Type
   {
-    ADD_BLOCKING_LIST,

[Felix Dahlke, 2015/07/29 20:32:07]

Nit: As said elsewhere, we should avoid hard coding the term blocking list since
we don't actually use it (yet), nor are we sure we will do that.

[René Jeschke, 2015/07/30 09:26:44]

Done.

-    ADD_WHITELIST

[Felix Dahlke, 2015/07/26 17:06:22]

It seems to me we're not using ADD_WHITELIST, nor do I understand what the
distinction is supposed to be. Technically they're the same thing, after all,
just subscriptions.

[René Jeschke, 2015/07/28 10:55:27]

Because the same dialogue will be needed to implement the whitelisting settings
page and I do not see a point in _not_ preparing for this case now.

[Felix Dahlke, 2015/07/29 20:32:07]

Acknowledged. Nit then though: Maybe call it ADD_WHITELISTED_SITE for clarity's
sake.

[René Jeschke, 2015/07/30 09:26:44]

Done.

+    ADD_SUBSCRIPTION,
+    ADD_WHITELISTED_SITE
   }
 
   public UrlInputDialog(final Context context, final Type type, final UrlReadyCallback callback)
   {
     super(context);
     this.type = type;
     this.callback = callback;
   }
 
   @Override
@@ skipping 15 matching lines @@
     this.button.setOnClickListener(this);
 
     if (this.currentEditTextValue != null)
     {
       this.editText.setText(this.currentEditTextValue);
     }
     this.updateButtonState();
 
     switch (this.type)
     {
-      case ADD_BLOCKING_LIST:
-        this.button.setText(R.string.abb_block_list_button);
-        this.editText.setHint(R.string.abb_block_list_url);
+      case ADD_SUBSCRIPTION:
+        this.button.setText(R.string.abb_add_subscription_button);
+        this.editText.setHint(R.string.abb_add_subscription_url);
         this.setTitle(R.string.abb_pref_category_add_other_list);
         break;
       default:
         break;
     }
   }
 
   private void updateButtonState()
   {
     this.button.setEnabled(this.submitEnabled);
     this.button.setBackgroundColor(this.submitEnabled
-        ? BUTTON_ENABLED_COLOR
-        : BUTTON_DISABLED_COLOR);
+        ? this.getContext().getResources().getColor(R.color.abb_url_input_button_enabled)
+        : this.getContext().getResources().getColor(R.color.abb_url_input_button_disabled));
     this.button.invalidate();
   }
 
   @Override
   public void afterTextChanged(final Editable s)
   {
     this.currentEditTextValue = s.toString();
     this.submitEnabled = this.validateUrl(this.currentEditTextValue) != null;
     if (this.submitEnabled)
     {
@@ skipping 19 matching lines @@
     // ignored
   }
 
   private String validateUrl(final String possibleUrl)
   {
     String url = possibleUrl.trim();
     if (Patterns.WEB_URL.matcher(url).matches())
     {
       return url;
     }
     url = "http://" + url;

[Felix Dahlke, 2015/07/26 17:06:22]

IMHO we should just consider URLs without the protocol invalid. I'm saying that
partly because SSL should be used where anyhow possible, but we have no idea if
it's going to work or not, and there won't be a guaranteed fallback to HTTP if
the target doesn't support HTTPS. So I'd just leave this out.

[René Jeschke, 2015/07/28 10:55:26]

Do you really want to force all Android users, to type the _whole_ link if it's
just plain http? It's already complicated enough to enter URLs, and enforcing
specifying a scheme, that is the default for URLs doesn't make sense to me.

Also, the hint text from the mockups does also not include 'http://' but starts
directly with the domain. 

[Felix Dahlke, 2015/07/29 20:32:07]

Hm, fair enough with the typing... Maybe it'd be slightly better to have a
selection of HTTP or HTTPS and then just enter the URL without protocol here.
Anyway, for the sake of progress, I think it's fine to do it like this for now.
If HTTP doesn't work, it normally gets redirected to HTTPS, so that should be
rather robust.

     if (Patterns.WEB_URL.matcher(url).matches())
     {
       return url;
     }
     return null;
   }
 
   private void validateAndSubmit(final String inputText)
   {
     final String url = this.validateUrl(inputText);
@@ skipping 31 matching lines @@
 
   @Override
   public void onClick(final View v)
   {
     if (this.currentEditTextValue != null)
     {
       this.validateAndSubmit(this.currentEditTextValue);
     }
   }
 }