Features for disappearing prompt and MasterPassword Lock Editing Conn… #480
Conversation
…ections Only functionality.
|
|
||
| @Override | ||
| public void onTextObtained(String dialogId, String[] obtainedString, boolean dialogCancelled, boolean save) { | ||
| public void onTextObtained(String dialogId, String[] obtainedString, boolean dialogCancelled, boolean save, boolean obtainedBooleans[]) { |
There was a problem hiding this comment.
Are these square brackets in the right place?
| builder.setIcon(icon); | ||
| if (!(alertDialog != null && alertDialog.isShowing()) && !isContextActivityThatIsFinishing(_context)) { | ||
| alertDialog = builder.create(); | ||
| if ( showHandler != null ) { |
There was a problem hiding this comment.
Please remove spacing around parentheses.
| if (!(alertDialog != null && alertDialog.isShowing()) && !isContextActivityThatIsFinishing(_context)) { | ||
| alertDialog = builder.create(); | ||
| if ( showHandler != null ) { | ||
| alertDialog.setOnShowListener(showHandler); |
There was a problem hiding this comment.
setOnShowListener shows its parameter is nullable, so maybe you can just pass showHandler even if it's null?
|
|
||
| public interface OnFragmentDismissedListener { | ||
| void onTextObtained(String dialogId, String[] obtainedStrings, boolean dialogCancelled, boolean save); | ||
| void onTextObtained(String dialogId, String[] obtainedStrings, boolean dialogCancelled, boolean save, boolean[] obtainedBools); |
There was a problem hiding this comment.
Please name obtainedBools vs obtainedBooleans consistently.
| private EditText textBox; | ||
| private EditText textBox2; | ||
| private EditText textBox3; | ||
| private Switch switch1; |
There was a problem hiding this comment.
Why is this switch numbered? Is there a specific function that it fulfills or is its function generic? If there is a specific function, let's please name it more specifically!
| private String t3; | ||
| private boolean keepPassword; | ||
|
|
||
| private boolean sw1; |
There was a problem hiding this comment.
If we are keeping the switch name, let's make this variable named with a full word.
| if (checkboxKeepPassword != null) { | ||
| keepPassword = checkboxKeepPassword.isChecked(); | ||
| } | ||
| if (switch1 != null) { |
There was a problem hiding this comment.
I am not quite certain why you have decided to make this an array of booleans and not opted out for a more specific naming convention that does not require for us to unpack booleans from an array by index.
| } | ||
| GetTextFragment frag = GetTextFragment.newInstance(dialogId, title, dismissalListener, | ||
| dialogType, messageNum, errorNum, t1, t2, t3, keep); | ||
| dialogType, messageNum, errorNum, t1, t2, t3, keep, false); |
There was a problem hiding this comment.
If this is always false, why is it a constructor parameter and not a constant?
| } | ||
| GetTextFragment frag = GetTextFragment.newInstance(dialogId, title, dismissalListener, | ||
| dialogType, messageNum, errorNum, t1, t2, t3, keep); | ||
| dialogType, messageNum, errorNum, t1, t2, t3, keep, false); |
There was a problem hiding this comment.
If this is always false, why is it a constructor parameter and not a constant?
| } | ||
| }); | ||
| AlertDialog alertDialog = alertDialogBuilder.create(); | ||
| if(Utils.querySharedPreferenceBoolean(appContext, Constants.onlyLockConnectionEditing)) { |
| } else { | ||
| togglingMasterPassword = true; | ||
| if (Utils.querySharedPreferenceBoolean(this, Constants.masterPasswordEnabledTag)) { | ||
| if (Utils.querySharedPreferenceBoolean(this, Constants.masterPasswordEnabledTag) || !((Utils.querySharedPreferenceString(this, Constants.masterPassword, "")).equals("")) && Utils.querySharedPreferenceBoolean(this, Constants.onlyLockConnectionEditing)) { |
There was a problem hiding this comment.
More safely, this is written "".equals(Utils.querySharedPreferenceString(this, Constants.masterPassword, "")).
Also, please take this condition out to an appropriately named separate method because it has gotten quite complex.
| Utils.setSharedPreferenceString(this, Constants.masterPassword, | ||
| BCrypt.with(LongPasswordStrategies.hashSha512(BCrypt.Version.VERSION_2A)).hashToString(Constants.HASH_COST, providedPassword.toCharArray())); | ||
| loadConnections = true; | ||
| } |
There was a problem hiding this comment.
Please put the } and else on the same line as } else {
|
|
||
| private boolean handlePasswordOnlyLockEditingConnections(String providedPassword, boolean dialogWasCancelled, String dialogId) { | ||
| boolean loadConnections; | ||
| if (dialogId == GetTextFragment.DIALOG_ID_GET_MATCHING_MASTER_PASSWORDS) { |
There was a problem hiding this comment.
In java, you don't typically check equality of strings with ==. However, I don't think we should pass dialogId into this method and then check it for equality with a static string in GetTextFragment. If you wanted the method to behave differently based on whether the dialog is one vs another, you should pass into the method a boolean indicating whether it's handling a master password vs a lock settings only password.
| boolean loadConnections; | ||
| if (dialogId == GetTextFragment.DIALOG_ID_GET_MATCHING_MASTER_PASSWORDS) { | ||
| Utils.setSharedPreferenceString(this, Constants.masterPassword, | ||
| BCrypt.with(LongPasswordStrategies.hashSha512(BCrypt.Version.VERSION_2A)).hashToString(Constants.HASH_COST, providedPassword.toCharArray())); |
There was a problem hiding this comment.
Should we not take this BCrypt.with() call to a separate method so this line of code is short and readable?
| public void handlePassword(String providedPassword, boolean dialogWasCancelled) { | ||
| public void handlePassword(String providedPassword, boolean dialogWasCancelled, String dialogId, boolean onlyLockEditingConnections) { | ||
| Log.i(TAG, "handlePassword"); | ||
| boolean loadConnections = onlyLockEditingConnections ? handlePasswordOnlyLockEditingConnections(providedPassword, dialogWasCancelled, dialogId) : handlePasswordDatabaseLock(providedPassword, dialogWasCancelled); |
There was a problem hiding this comment.
This line is very long and difficult to read - maybe we can take this out to a separate method?
| BCrypt.Result result = BCrypt.verifyer(BCrypt.Version.VERSION_2A, LongPasswordStrategies.hashSha512(BCrypt.Version.VERSION_2A)).verify(providedPassword.toCharArray(), Utils.querySharedPreferenceString(this, Constants.masterPassword, "")); | ||
| loadConnections = result.verified; | ||
| if(!loadConnections || dialogWasCancelled) { | ||
| String message = dialogWasCancelled ? this.getResources().getString(R.string.master_password_error_password_necessary) : this.getResources().getString(R.string.master_password_error_wrong_password); | ||
| Utils.showErrorMessage(this,message); | ||
| if (activeAlertDialog != null) { | ||
| activeAlertDialog.dismiss(); | ||
| activeAlertDialog = null; | ||
| } | ||
| } else if (togglingMasterPassword) { | ||
| Utils.setSharedPreferenceBoolean(this, Constants.onlyLockConnectionEditing, false); | ||
| Utils.setSharedPreferenceString(this, Constants.masterPassword, null); | ||
| togglingMasterPassword = false; | ||
| } | ||
| } | ||
| return loadConnections; |
There was a problem hiding this comment.
This wall of code should be refactored for readability.
| } | ||
| } | ||
| }, new DialogInterface.OnShowListener() { | ||
| private static final int AUTO_DISMISS_MILLIS = RemoteClientLibConstants.ON_SHOW_LISTENER_TIMER; |
There was a problem hiding this comment.
Why not just import ON_SHOW_LISTENER_TIMER from RemoteClientLibConstants?
| public static final int LOGCAT_MAX_LINES = 500; | ||
|
|
||
| public static final int ON_SHOW_LISTENER_TIMER = 5000; | ||
| public static final int ON_SHOW_LISTENER_CHECK_INTERVAL = 100; |
There was a problem hiding this comment.
Does it make sense for this to be less than 1000?
Features for disappearing prompt and MasterPassword Lock Editing Connections