Allow users to use their own keyboard layouts and dictionaries #124
Conversation
| boolean found = false; | ||
| int foundDirection = -1; | ||
| int foundDirection; |
There was a problem hiding this comment.
Thanks! The initial value of foundDirection doesn't have to be -1. It is always initialized with curDirection. 👍
| return dictionaries; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
POSIX rules require a newline at the end of the file. Please add a newline.
| try { | ||
| this.context = new ZxcvbnBuilder().buildContext(); | ||
| } catch (IOException e) { | ||
| throw new IllegalStateException(e); |
There was a problem hiding this comment.
It is already Wrapped with IllegalStateException in buildContext as follows.
Is it necessary to CATCH here as well?
Context buildContext() throws IOException {
try {
// ...
return new Context(dictionaryMap, keyboardMap);
} catch (IOException e) {
throw new IllegalStateException(e);
}
}
|
|
||
| Context buildContext() throws IOException { | ||
| try { | ||
| if (this.dictionaryMap.isEmpty() && this.keyboardMap.isEmpty()) { |
There was a problem hiding this comment.
It seems that the default dictionary and keyboard is added only when both are EMPTY.
How about checking each the dictionary and keyboard?
Example:
if (this.dictionaryMap.isEmpty()) {
for (Dictionary dictionary : StandardDictionaries.loadAllDictionaries()) {
this.dictionaryMap.put(dictionary.getName(), dictionary);
}
}
if (this.keyboardMap.isEmpty()) {
for (Keyboard keyboard : StandardKeyboards.loadAllKeyboards()) {
this.keyboardMap.put(keyboard.getName(), keyboard);
}
}
There was a problem hiding this comment.
@vvatanabe I designed it that way because I thought it would be difficult to tell when either the dictionary or the keyboard was not set, whether the user had not set it intentionally or had forgotten to set it.
However, let me consider it a bit further to see if there is a way to make it more clearly recognizable to users.
|
|
||
| public class RegexGuess extends BaseGuess { | ||
|
|
||
| private static final Logger logger = Logger.getLogger(RegexGuess.class.getName()); |
| } | ||
|
|
||
| @Override | ||
|
|
| import java.util.Arrays; | ||
| import java.util.List; | ||
|
|
||
| public class AlignedAdjacentAdjacentGraphBuilder extends Keyboard.AdjacentGraphBuilder { |
There was a problem hiding this comment.
This file and class name has been wrong for some time.
AlignedAdjacentAdjacentGraphBuilder -> AlignedAdjacentGraphBuilder
It's hard to review when there are more extra differences, so I can fix this with another issue.
| } | ||
| return freqLists; | ||
| public Map<String, Integer> getRankedDictionary() { | ||
| return rankedDictionary; | ||
| } | ||
| } |
There was a problem hiding this comment.
POSIX rules require a newline at the end of the file. Please add a newline.
| Map<Pattern, Integer[]> testMatches = new HashMap<>(); | ||
| testMatches.put(Pattern.Dictionary, new Integer[]{0, 6}); | ||
| //testMatches.put(Pattern.Dictionary, new Integer[]{0, 6}); |
There was a problem hiding this comment.
Why is this commented out? Did you forget?
There was a problem hiding this comment.
@vvatanabe It looks like the value put in the commented-out line is overwritten by the next line.
testMatches.put(Pattern.Dictionary, new Integer[]{7, 15});
So I commented out that line.
| import com.nulabinc.zxcvbn.matchers.ReverseDictionaryMatcher; | ||
| import com.nulabinc.zxcvbn.matchers.SequenceMatcher; | ||
| import com.nulabinc.zxcvbn.matchers.SpatialMatcher; | ||
| import org.junit.*; |
There was a problem hiding this comment.
Does this package require a full import of junit?
| import com.nulabinc.zxcvbn.matchers.Match; | ||
| import com.nulabinc.zxcvbn.matchers.MatchFactory; | ||
| import org.junit.Test; | ||
| import org.junit.*; |
There was a problem hiding this comment.
Does this package require a full import of junit?
|
@yasuyuki-baba These API designs are great. Thanks! |
|
@lumarino just a notice This approach allows custom dictionaries and keyboard settings to be defined entirely in code. By abstracting resources with InputStream, files other than classpaths (over HTTP, other directories in server) can also be used. HOW TO USE: |
|
@vvatanabe Thanks for reviewing my code! |
This means that if you want to use the standard dictionary and keyboard with ZxcvbnBuilder, you need to do something like ZxcvbnBuilder#testBuild2().
|
@vvatanabe I've made the following changes to make it explicit what dictionary and keyboard will be used. What do you think?
|
The above changes look good! Thanks. |
I found #106
This pull request is a very nice improvement. I am impressed.
So I tried to give more freedom to customize the dictionary and keyboard layout.
I have achieved it by using the builder pattern and trying to specify resources in the code.
See src/test/java/com/nulabinc/zxcvbn/ZxcvbnBuilderTest.java for the example of the code to customize.