zhanghai / PatternLock

Material Design Pattern Lock with auth flow implementation
687 stars 153 forks source link

rowCount and colCount attributes added to PatternView #14

Closed faruktoptas closed 7 years ago

zhanghai commented 8 years ago

Some doubts regarding making Cell.of() into PatternView.of().

Creating a list of Cells requires an instance of PatternView seems weird logically - I doubt it's the best way of implementation (nor is it semantically appropriate, because even so it should be something like PatternView.cellOf()). What's more, you can add the cells from a large PatternView to a small PatternView, thus nullifying the range check. I believe in this case checks should be done on input, such as in setPattern().

I believe the original PatternView use Cell.of() primarily to avoid uncessary allocation, especially when handling the frequent touch events, so they added that, just as a known set of singletons.

However, since we are now giving users much more freedom upon Cells, we should no longer take this singleton approach. I believe we can still have the cache (via something like getCellAt() other than Cell.of(), etc), but we should let users create new instances of Cell inside Cell.of() now.

faruktoptas commented 8 years ago

checkRange() method needs to know the row and column counts. These values are the properties of the PatternView object. So I moved it into this class. It will be better to rename the method to cellOf().

zhanghai commented 8 years ago

Cell.chekckRange() should be PatternView.checkRange() now, but Cell.of() should not. Checks should be performed upon input such as inside setPattern().

faruktoptas commented 8 years ago

So what about sCells? A static property of Cell or a property of PatternView object?

zhanghai commented 8 years ago

PatternView.mCells, and a new private method PatternView.getCellAt() for usage such as in PatternView.handleAction*().

faruktoptas commented 8 years ago

I added Cell.of() static method to create cell instances and renamed PatternView.of() to PatternView.cellOf().

faruktoptas commented 8 years ago

Anything else to fix?

zhanghai commented 8 years ago

Sorry for the inactivity. I was thinking about how to modify PatternUtils.patternToString() and PatternUtils.stringToPattern() to accomodate varing row and column count, meanwhile better staying compatible with previous versions. I think this problem should better be settled before we merge this feature into master.

faruktoptas commented 8 years ago

You are right. Before merging, you should find a right way for these methods.

faruktoptas commented 7 years ago

Hi,

Have you find time to think about this?

zhanghai commented 7 years ago

Think I'll take the CM approach: https://github.com/CyanogenMod/android_frameworks_base/commit/abf94441542ac8dcd3eb1236a98ef5bcb0de52c8#diff-83414c2aa5cb9ae3cff6ec2b493f3d1a

(Also note that the CM implementation did not handle multiple different sized views as well.)

(Sorry for the inactivity, I'm busy with some personal matters recently, and hopefully I'll resolve this today.)

faruktoptas commented 7 years ago

Getting the hash of the pattern in a byte array seems a good approach.

zhanghai commented 7 years ago

@faruktoptas Reviewing the code. Seems that your detectAndAddHit cannot detect the case of the middle cell in a 5x3 setup. (Seems the CM/LineageOS implementation also suffers from this bug.) A11y is also not properly implemented yet.

zhanghai commented 7 years ago

Finally done, hurray! Merging this PR now.

faruktoptas commented 7 years ago

Thank you, good job :) 👍