Issue 3916 - Supporting Adding filter lists via URL

adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java

Index: adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java
===================================================================
--- a/adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java
+++ b/adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/UrlInputOpenerPreference.java
@@ -20,16 +20,17 @@ package org.adblockplus.sbrowser.content
 import android.app.AlertDialog;
 import android.content.Context;
 import android.os.Bundle;
 import android.preference.EditTextPreference;
 import android.text.Editable;
 import android.text.InputType;
 import android.text.TextWatcher;
 import android.util.AttributeSet;
+import android.util.Patterns;
 import android.view.KeyEvent;
 import android.view.View;
 import android.view.inputmethod.EditorInfo;
 import android.widget.EditText;
 import android.widget.TextView;
 import android.widget.Toast;
 
 import org.adblockplus.adblockplussbrowser.R;
@@ -37,27 +38,29 @@ import org.adblockplus.sbrowser.contentb
 import org.apache.commons.validator.routines.DomainValidator;
 
 public class UrlInputOpenerPreference extends EditTextPreference implements TextWatcher,

[diegocarloslima, 2017/09/08 15:06:07]

I would suggest to rename the class to something like InputOpenerPreference or
InputValidatorDialogPreference, since now it became a more general purpose class
that can validate URLs and domain (and can validate other stuff in the future).

[jens, 2017/09/13 08:50:29]

Acknowledged.

     TextView.OnEditorActionListener
 {
 
   private OnUrlReadyListener onUrlReadyListener;
   private AlertDialog mAlertDialog;
+  private boolean validateTld;

[anton, 2017/08/25 10:40:40]

sorry, what is `Tld` (did i miss anything)? Can you change it to anything more
readable?

[jens, 2017/08/25 11:35:06]

It stands for top-level domain. I thought it is a common abbreviation. The
reason I used this name is, that we introduced a domain validation to ensure
that only  existing top-level domains can be entered when whitelisting a
website. I will change the name to 'validateTopLevelDomain'

[anton, 2017/08/25 11:43:30]

Okay, why not just call it 'validateDomain' for simplicity and add a comment
that it validates only TLD?
We can ask Diego what he thinks about it.

"There are only two hard things in Computer Science: cache invalidation and
naming things" ;)

[diegocarloslima, 2017/09/08 15:06:07]

I would suggest to create an enum for that, so we can extend this preference to
validate other types of inputs in the future.
I would call the enum ValidationType {DOMAIN, URL}

[jens, 2017/09/13 08:50:29]

Acknowledged.

 
-  public UrlInputOpenerPreference(Context context)
+  public UrlInputOpenerPreference(Context context, final boolean validateTld)
   {
-    this(context, null);
+    this(context, null, validateTld);
   }
 
-  public UrlInputOpenerPreference(Context context, AttributeSet attrs)
+  public UrlInputOpenerPreference(Context context, AttributeSet attrs, final boolean validateTld)
   {
     super(context, attrs);
 
     // Setting defaults
+    this.validateTld = validateTld;
     this.setIcon(android.R.drawable.ic_menu_add);
     this.setPositiveButtonText(android.R.string.ok);
     this.setNegativeButtonText(android.R.string.cancel);
     final EditText editText = getEditText();
     editText.addTextChangedListener(this);
     editText.setOnEditorActionListener(this);
     editText.setInputType(InputType.TYPE_TEXT_VARIATION_URI);
   }
@@ -94,25 +97,25 @@ public class UrlInputOpenerPreference ex
   public void onTextChanged(CharSequence s, int start, int before, int count)
   {
     // Ignored
   }
 
   @Override
   public void afterTextChanged(Editable s)
   {
-    setPositiveButtonEnabled(isValidDomain());
+    setPositiveButtonEnabled(validate());
   }
 
   @Override
   public boolean onEditorAction(TextView v, int actionId, KeyEvent event)
   {
     if (actionId == EditorInfo.IME_ACTION_DONE)
     {
-      if (this.isValidDomain())
+      if (this.validate())
       {
         mAlertDialog.getButton(AlertDialog.BUTTON_POSITIVE).performClick();
       }
       else
       {
         Toast.makeText(getContext(), R.string.whitelist_website_invalid_url_error, Toast.LENGTH_SHORT).show();
       }
       return true;
@@ -132,16 +135,33 @@ public class UrlInputOpenerPreference ex
     this.onUrlReadyListener = listener;
   }
 
   private boolean isValidDomain()
   {
     return DomainValidator.getInstance().isValid(getUrl());
   }
 
+  private boolean isValidUrl()
+  {
+    return Patterns.WEB_URL.matcher(getUrl()).matches();
+  }
+
+  private boolean validate()

[anton, 2017/08/25 10:40:40]

improvement suggestion: `validate` makes me think it's just an action (without
result)
and `isValid()` sounds better for me. Also you already have 'isValidUrl' and
'isValidDomain' methods.

[jens, 2017/08/25 11:35:06]

Acknowledged.

[diegocarloslima, 2017/09/08 15:06:07]

I would further suggest to rename it to isValidInput()

[jens, 2017/09/13 08:50:30]

Acknowledged.

+  {
+    if (validateTld)
+    {
+      return isValidDomain();
+    }
+    else
+    {
+      return isValidUrl();
+    }
+  }
+
   private String getUrl()
   {
     return getEditText().getText().toString();
   }
 
   private void setPositiveButtonEnabled(boolean enabled)
   {
     if (mAlertDialog != null)
@@ -149,9 +169,9 @@ public class UrlInputOpenerPreference ex
       mAlertDialog.getButton(AlertDialog.BUTTON_POSITIVE).setEnabled(enabled);
     }
   }
 
   public interface OnUrlReadyListener

[diegocarloslima, 2017/09/08 15:06:07]

I would also suggest to rename the listener to OnInputReadyListener and the
method to onInputReady(String input)

[jens, 2017/09/13 08:50:30]

Acknowledged.

   {
     void onUrlReady(String url);
   }
-}
+}