Pattern Lock Added #2083

shashvat-kedia · 2017-12-25T10:19:53Z

Fix to Issue #2077

BUGS & IMPROVEMENTS

Message when pattern removed Pattern Lock Added #2083 (comment)
Message when switching the security method Pattern Lock Added #2083 (comment)
Landscape mode Pattern Lock Added #2083 (comment) [WONT FIX]

shashvat-kedia · 2017-12-25T10:20:46Z

This PR does not consist of the changes made for Issue #2027

jesmrec · 2018-01-09T07:52:49Z

Thanks for contributing @SD1998 !! i have check a bit this feature and i suggest a couple of improvements:

The section "Details" of the settings view should be renamed to "Security"
The word "Lock" is uppercase in Pattern and lowercase in Passcode
Pattern Lock can only be set if the passcode if not set. But if both are enabled, the pattern lock can not be disabled. Which priorities did you set for both security methods?

CC @davigonz

shashvat-kedia · 2018-01-09T13:58:37Z

@jesmrec I will start working on the changes you have suggested.

jesmrec · 2018-01-09T14:54:02Z

ok @SD1998. It is important to set a policy of priorities how the different "locks" are going to be handled for the user (third point of my previous message). Thinking also that other locks can be included in the future (for example, the fingerprint).

iOS handles it (touch id) in the following way: passcode is always mandatory if the user wants to enable the touch id. Both are displayed and the user can cancel the touch id to use only the passcode. This is only an example, and we should have an idea to manage the different locks?

Which is your idea about?

shashvat-kedia · 2018-01-09T16:52:25Z

@jesmrec Actually the way I have implemented the feature both the type of locks cannot be set at the same time.

jesmrec · 2018-01-09T17:10:33Z

ok, so you have a bug ;) if you enable first passcode, then you can enable pattern as well.

So, the policy is that only one lock should be enabled at time. What do you thing @davigonz ? maybe passcode should be always enabled as shortcut?

davigonz · 2018-01-10T07:54:54Z

Actually the way I have implemented the feature both the type of locks cannot be set at the same time

So, the policy is that only one lock should be enabled at time.

@jesmrec @SD1998

Well, for this cases, I think we should follow the same way in which the Android system protects our phone.

Android allows us to set a pattern to lock the device without setting a passcode and vice versa, but forces us to set a passcode or pattern when using fingerprint for instance, so passcode and pattern would be different ways to unlock ownCloud app and cannot be enabled at the same time.

shashvat-kedia · 2018-01-10T18:31:15Z

@davigonz @jesmrec
comment
Yes I agree with you and I implemented the feature so that any one of the locks can be used and not both of them together but I have no idea why it is not working I will fix this and send a PR.

shashvat-kedia · 2018-01-17T12:29:43Z

@jesmrec I have made the changes.

shashvat-kedia · 2018-01-17T12:33:08Z

@jesmrec @davigonz I implemented the feature in such a way that if the passcode was set then the pattern could not be added but I forgot to account for the other case(i.e if the pattern is set then the user should not be able to set passcode) hence the bug (comment). But now I have fixed it.

jesmrec · 2018-01-17T12:40:57Z

Thanks @SD1998, we will pass code review and QA, and we will let you know if we find some issues.

davigonz · 2018-01-22T13:56:25Z

Now that this branch is out-of-date with master, please use what I described in #2075 (comment) here as well

jesmrec · 2018-01-25T12:58:39Z

This is ready for QA, isn't it @SD1998?

shashvat-kedia · 2018-01-25T13:00:12Z

@jesmrec Yes I just have to update this branch so that it is up to date with master (of ownCloud). I am doing it now.

shashvat-kedia · 2018-01-26T11:50:58Z

@davigonz Whenever I rebase with master of ownCloud should I create a new PR?

davigonz · 2018-01-26T12:20:41Z

Whenever I rebase with master of ownCloud should I create a new PR?

@SD1998 I don't think so, if you are able to rebase it properly , the PR wouldn't be necessary and you have even created a new branch to implement the feature so 👍

shashvat-kedia · 2018-01-26T17:09:11Z

@davigonz I guess the rebase worked.

shashvat-kedia · 2018-02-01T13:22:57Z

@jesmrec Has the QA started for this and PR?

jesmrec · 2018-02-01T13:33:34Z

@SD1998 not yet, but i will take care next days. It will be included in 2.7.0 for sure.

davigonz · 2018-02-01T13:35:49Z

Has the QA started for this and PR?

not yet, but i will take care next days. It will be included in 2.7.0 for sure.

@SD1998 @jesmrec It would be better to review a bit the code before starting QA

jesmrec · 2018-02-01T13:51:17Z

of course @davigonz, i will be ready for your signal ;)

davigonz · 2018-02-05T13:50:44Z

AndroidManifest.xml

@@ -61,6 +61,8 @@
    <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" />
    <uses-permission android:name="android.permission.RECEIVE_BOOT_COMPLETED" />
    <uses-permission android:name="android.permission.WAKE_LOCK" />
+    <uses-permission android:name="android.permission.READ_PHONE_STATE" />


What are you using this permission and the one just below for?

@davigonz I added them when I working on the Issue #2027 (when I was working on master rather than creating a new branch) but I think I forgot to remove it during rebase. I will remove them now.

davigonz · 2018-02-05T13:53:04Z

AndroidManifest.xml

-        </activity>
+            android:theme="@style/Theme.ownCloud.Fullscreen" />
+        <activity android:name=".ui.activity.CapturedFileChecker" />
+        <activity android:name=".ui.activity.Camera" />


I think that CapturedFileChecker and Camera are unused

Yes I have removed them.

davigonz · 2018-02-05T13:55:49Z

build.gradle

    compile "com.android.support:appcompat-v7:${supportLibraryVersion}"
    compile 'com.getbase:floatingactionbutton:1.10.1'
    compile group: 'commons-io', name: 'commons-io', version: '2.5'

    /// dependencies for local unit tests
+    compile 'com.android.support.constraint:constraint-layout:1.0.2'


This dependency would be better placed out of the comment /// dependencies for local unit testssince is not used for that purpose.

davigonz · 2018-02-05T14:04:52Z

res/values/strings.xml

@@ -28,6 +28,7 @@
    <string name="drawer_open">Open</string>
    <string name="prefs_category_general">General</string>
    <string name="prefs_category_more">More</string>
+    <string name="prefs_category_security"> Security </string>


You do not need the extra white spaces at the beginning and end in these string resources.

davigonz · 2018-02-05T14:06:15Z

res/values/strings.xml

+    <string name="pattern_enter_pattern"> Please enter your pattern </string>
+    <string name="pattern_configure_pattern"> Enter your pattern </string>
+    <string name="pattern_remove_pattern"> Remove your pattern </string>
+    <string name="pattern_incorrect_pattern"> Incorrect Pattern </string>


Pattern would better be lowercase

davigonz · 2018-02-05T14:09:11Z

res/values/strings.xml

+    <string name="pattern_reenter_pattern"> Please re-enter your pattern </string>
+    <string name="pattern_not_same_pattern"> The patterns are not same </string>
+    <string name="pattern_stored"> Pattern Stored </string>
+    <string name="pattern_removed"> Pattern Removed </string>


Removed would better be lowercase

davigonz · 2018-02-05T14:09:20Z

res/values/strings.xml

+    <string name="pattern_not_same_pattern"> The patterns are not same </string>
+    <string name="pattern_stored"> Pattern Stored </string>
+    <string name="pattern_removed"> Pattern Removed </string>
+    <string name="passcode_already_set"> Passcode Already Set </string>


Already and Set would better be lowercase

davigonz · 2018-02-05T14:09:40Z

res/values/strings.xml

+    <string name="pattern_stored"> Pattern Stored </string>
+    <string name="pattern_removed"> Pattern Removed </string>
+    <string name="passcode_already_set"> Passcode Already Set </string>
+    <string name="pattern_already_set"> Pattern Already Set </string>


davigonz · 2018-02-05T14:12:58Z

build.gradle

@@ -46,15 +46,16 @@ dependencies {
    compile "com.android.support:design:${supportLibraryVersion}"
    compile 'com.jakewharton:disklrucache:2.0.2'
    compile 'com.google.android.exoplayer:exoplayer:r2.2.0'
+    compile 'com.andrognito.patternlockview:patternlockview:1.0.0'


When we use a third-party library we usually include the information in our THIRD_PARTY.txt file. Could you do the same with this library? You can have a look at the format we followed with other libraries in that file.

As in this picture for the floating action button dependency it is written that it is placed in libs/com-getbase-floatingactionbutton-1-10-0-exploded-aar. Similarly what should I write for the placed at point (for the pattern lock view dependency).What I mean is what does placed at signify.

Forget this change for the moment, it seems that our THIRD_PARTY.txt is not properly updated, I will ping you when is ready.

@davigonz Then I will commit the other changes. Thanks.

davigonz · 2018-02-05T14:17:41Z

src/com/owncloud/android/authentication/PatternManager.java

+import java.util.Set;
+
+/**
+ * Created by shashvatkedia on 21/12/17.


Given that this is a new class, please delete this line and add a new comment in the top with the authors and copyright information:

/** * ownCloud Android client application * * @author YOUR NAME HERE * Copyright (C) 2018 ownCloud GmbH. * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2, * as published by the Free Software Foundation. * * This program is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. * * You should have received a copy of the GNU General Public License * along with this program. If not, see <http://www.gnu.org/licenses/>. * */

shashvat-kedia · 2018-02-27T13:22:19Z

@jesmrec has the QA started?

jesmrec · 2018-03-01T10:46:14Z

@SD1998 let's go for it!!

jesmrec · 2018-03-01T10:48:09Z

IMPROVEMENT

Steps:

Add pattern lock
Click on pattern lock in order to remove it

Current: Message The pattern will be requested every time the app is started
Expected: The pattern will not be asked anymoreor similar. The current message fits when you set a new pattern lock, but not much if you remove it.

Tested with Nexus 6P v7

jesmrec · 2018-03-01T11:07:28Z

IMPROVEMENT

Steps:

Add pattern lock
Try to enable passcode (or viceversa)

Current: Message Pattern already set (or Passcode already set) in a Snackbar

This message does not clarify the fact that only one of these methods can be enabled at time.

Suggestions:

Please, disable XXX first (where XXX is the enabled method)
...

(any other @davigonz ? )

Tested with Nexus 6P v7

jesmrec · 2018-03-01T11:12:40Z

IMPROVEMENT

Not sure if this one depends on the library you used to develop the feature

Steps:

Add pattern lock
Set the device in landscape in the pattern lock view

Current: Cancel button is shown at the right of the pattern points
Expected: Cancel button is shown below, as in landscape.

Tested with Nexus 6P v7

shashvat-kedia · 2018-03-01T11:16:13Z

@jesmrec this does not depend on the library I have used. I placed the cancel button to the right of the patternLockView because in landscape orientation the cancel button does not fit properly bellow the patternLockView (i.e due to the margin between the button and the patternLockView the cancel button is not properly visible on my device (Oneplus 3T) hence I placed it beside the patternLockView).

davigonz · 2018-03-01T11:23:00Z

Please, disable XXX first (where XXX is the enabled method)
(any other @davigonz ? )

@SD1998

Yes, or something like: "Pattern and passcode locks cannot be enabled at the same time, please disable XXX first"

jesmrec · 2018-03-01T11:36:07Z

@SD1998 QA is passed, and i only noticed the suggested improvements. Functionally is working pretty fine (congrats for the work!!), so we can discuss the improvements, and then, after checking them and rebasing the branch, it will be ready to merge.

shashvat-kedia · 2018-03-02T08:49:45Z

@jesmrec @davigonz I have made the changes and done the rebase.

jesmrec · 2018-03-02T12:02:34Z

Changes tested and all OK.

Approved

davigonz · 2018-03-02T12:06:03Z

Changes tested and all OK.
Approved

Great, merging it... Thanks @SD1998 for this feature

jesmrec added this to the 2.7.0 milestone Jan 16, 2018

shashvat-kedia force-pushed the fix/2077 branch from 49c965d to 945bbcc Compare January 26, 2018 17:08

shashvat-kedia force-pushed the fix/2077 branch 4 times, most recently from e5948db to 838cad3 Compare February 1, 2018 13:19

shashvat-kedia force-pushed the fix/2077 branch from 838cad3 to f572574 Compare February 2, 2018 11:21

jesmrec mentioned this pull request Feb 2, 2018

[PR 2083] Pattern lock owncloud/QA#526

Closed

davigonz suggested changes Feb 5, 2018

View reviewed changes

shashvat-kedia force-pushed the fix/2077 branch from 9eb02ec to d75effb Compare February 27, 2018 13:23

shashvat-kedia added 13 commits March 2, 2018 13:26

Fix to Issue owncloud#2077

d9979ed

Landscape Orientation Support Added

42ecd87

Bug Fixed

3d289b2

Code review fixes

5508c87

Unused permissions removed

973c2f8

Code review fixes

f0202f6

Code review fixes

69bac27

Typo in comment fixed

0d0d969

Comments and Variables fixed

8f9735b

owncloud-android-library added

f52a8f2

THIRD_PARTY.txt updated

7874183

QA fixes

e12efec

Unused string resource removed

2b5a0d8

shashvat-kedia force-pushed the fix/2077 branch from cc6dde2 to 2b5a0d8 Compare March 2, 2018 07:56

jesmrec added the Approved by qa label Mar 2, 2018

davigonz merged commit 4a2491a into owncloud:master Mar 2, 2018

jesmrec removed this from the 2.7.0 milestone Mar 2, 2018

shashvat-kedia deleted the fix/2077 branch March 2, 2018 13:52

Pattern Lock Added #2083

Pattern Lock Added #2083

Conversation

shashvat-kedia commented Dec 25, 2017 • edited by jesmrec Loading

shashvat-kedia commented Dec 25, 2017

jesmrec commented Jan 9, 2018

shashvat-kedia commented Jan 9, 2018

jesmrec commented Jan 9, 2018 • edited Loading

shashvat-kedia commented Jan 9, 2018

jesmrec commented Jan 9, 2018

davigonz commented Jan 10, 2018 • edited Loading

shashvat-kedia commented Jan 10, 2018

shashvat-kedia commented Jan 17, 2018

shashvat-kedia commented Jan 17, 2018

jesmrec commented Jan 17, 2018

davigonz commented Jan 22, 2018 • edited Loading

jesmrec commented Jan 25, 2018 • edited Loading

shashvat-kedia commented Jan 25, 2018 • edited Loading

shashvat-kedia commented Jan 26, 2018

davigonz commented Jan 26, 2018 • edited Loading

shashvat-kedia commented Jan 26, 2018

shashvat-kedia commented Feb 1, 2018

jesmrec commented Feb 1, 2018

davigonz commented Feb 1, 2018 • edited Loading

jesmrec commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashvat-kedia commented Feb 27, 2018

jesmrec commented Mar 1, 2018

jesmrec commented Mar 1, 2018

IMPROVEMENT

jesmrec commented Mar 1, 2018

IMPROVEMENT

jesmrec commented Mar 1, 2018

IMPROVEMENT

shashvat-kedia commented Mar 1, 2018 • edited Loading

davigonz commented Mar 1, 2018

jesmrec commented Mar 1, 2018 • edited Loading

shashvat-kedia commented Mar 2, 2018

jesmrec commented Mar 2, 2018

davigonz commented Mar 2, 2018 • edited Loading

shashvat-kedia commented Dec 25, 2017 •

edited by jesmrec

Loading

jesmrec commented Jan 9, 2018 •

edited

Loading

davigonz commented Jan 10, 2018 •

edited

Loading

davigonz commented Jan 22, 2018 •

edited

Loading

jesmrec commented Jan 25, 2018 •

edited

Loading

shashvat-kedia commented Jan 25, 2018 •

edited

Loading

davigonz commented Jan 26, 2018 •

edited

Loading

davigonz commented Feb 1, 2018 •

edited

Loading

shashvat-kedia commented Mar 1, 2018 •

edited

Loading

jesmrec commented Mar 1, 2018 •

edited

Loading

davigonz commented Mar 2, 2018 •

edited

Loading