Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for QR codes & deep links #3383

Merged
merged 4 commits into from
Jan 21, 2019
Merged

Add support for QR codes & deep links #3383

merged 4 commits into from
Jan 21, 2019

Conversation

tobiasKaminsky
Copy link
Member

  • bump min sdk from 14 to 15. Needed by lib, but according to google we do not have any with <4.0.3
    image
  • in login screen, you can now scan a QR code (requires Camera access)
  • you can also click on any nc://login… link and it will also log you in.
  • if new library version up released, we can switch to use this

Signed-off-by: tobiasKaminsky tobias@kaminsky.me

@AndyScherzinger
Copy link
Member

AndyScherzinger commented Dec 17, 2018

Can I be annoyed by this? Simply because I vote/fight/argue for a version bump 14->15 (see #2923) and get blocked but when we need a QR code scanner we happily do so?

@tobiasKaminsky
Copy link
Member Author

Hm. You are right, sorry ❤️!
The last time we talked about this, there were active 4.0.x devices, now there are none.
So we still should keep min 14, until we have a go in #2923.

I can show the button for >=15 only, but I have to figure out how I can do this in build.gradle.

@AndyScherzinger
Copy link
Member

AndyScherzinger commented Dec 17, 2018

The last time we talked about this, there were active 4.0.x devices, now there are none.

Well, 13 to be true ;) And for customers it could be a case if they run a branded client ;)

So we still should keep min 14, until we have a go in #2923.

No, we should drop 14. If whoever / @oparoz can't provide any feedback regarding this, even within the last 3 months then they shouldn't have a say in this or have to deal with the situation afterwards. You are either involved in this or you are not and if you are not (which seems obvious) than you don't get to vote on this. Just my 2 cents.

I can show the button for >=15 only, but I have to figure out how I can do this in build.gradle.

Please let's move on to v15 🙏

@tobiasKaminsky
Copy link
Member Author

Well, 13 to be true ;)

API14: 4.0, 4.0.1, 4.0.2
API15: 4.0.3, 4.0.4

So, apart from any customers, fdroid, manual installations, we do not have 14 in use.

@AndyScherzinger
Copy link
Member

So, apart from any customers, fdroid, manual installations, we do not have 14 in use.

correct 👍

@codecov
Copy link

codecov bot commented Dec 19, 2018

Codecov Report

Merging #3383 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##             master   #3383      +/-   ##
===========================================
- Coverage      6.13%   6.11%   -0.03%     
  Complexity        1       1              
===========================================
  Files           315     316       +1     
  Lines         30508   30559      +51     
  Branches       4402    4410       +8     
===========================================
- Hits           1872    1868       -4     
- Misses        28353   28405      +52     
- Partials        283     286       +3
Impacted Files Coverage Δ Complexity Δ
...owncloud/android/authentication/DeepLinkLogin.java 0% <0%> (ø) 0 <0> (?)
.../android/authentication/AuthenticatorActivity.java 1.62% <0%> (-0.07%) 0 <0> (ø)
...m/owncloud/android/services/OperationsService.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...ava/com/owncloud/android/utils/PermissionUtil.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...in/java/com/owncloud/android/datamodel/OCFile.java 57.6% <0%> (-1.39%) 0% <0%> (ø)
.../third_parties/daveKoeller/AlphanumComparator.java 79.76% <0%> (-1.2%) 0% <0%> (ø)

@codecov
Copy link

codecov bot commented Dec 19, 2018

Codecov Report

Merging #3383 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##             master   #3383      +/-   ##
===========================================
- Coverage      6.14%   6.13%   -0.01%     
  Complexity        1       1              
===========================================
  Files           315     316       +1     
  Lines         30522   30600      +78     
  Branches       4373    4383      +10     
===========================================
+ Hits           1876    1878       +2     
- Misses        28363   28440      +77     
+ Partials        283     282       -1
Impacted Files Coverage Δ Complexity Δ
.../android/authentication/AuthenticatorActivity.java 1.58% <0%> (-0.11%) 0 <0> (ø)
...m/owncloud/android/services/OperationsService.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...ava/com/owncloud/android/utils/PermissionUtil.java 0% <0%> (ø) 0 <0> (ø) ⬇️
.../android/authentication/DeepLinkLoginActivity.java 0% <0%> (ø) 0 <0> (?)
.../third_parties/daveKoeller/AlphanumComparator.java 83.33% <0%> (+2.38%) 0% <0%> (ø) ⬇️

@AndyScherzinger
Copy link
Member

@tobiasKaminsky I revamped the qr code icon :)
device-2018-12-19-090801

@nextcloud nextcloud deleted a comment Dec 19, 2018
@nextcloud nextcloud deleted a comment Dec 19, 2018
@nextcloud nextcloud deleted a comment Dec 19, 2018
@AndyScherzinger
Copy link
Member

@tobiasKaminsky tested the scanner with non-Nc-login-link qr codes. The scan then returns to the login screen (like in the screenshot above) but I don't get any feedback as a user that the qr code is not a Nc-Login-code so I think it would be good to show a Toast or Snackbar stating exactly that. :)

@AndyScherzinger
Copy link
Member

AndyScherzinger commented Dec 19, 2018

@tobiasKaminsky some issues I found:

  • use non fitting qr code -> return back to login screen (no feedback)
  • use nc:// url with wrong credentials (spinner and return back to login overview screen where you choose between login and provider!) -> should end up on login screen again and also provide a feedback about the failed login
  • test suite still uses nextcloud: as the protocoll not nc:// should be updated within this PR

What works:

  • login via proper qr code with valid credentials 👍

@tobiasKaminsky
Copy link
Member Author

@AndyScherzinger can you re-test it again?

@nextcloud nextcloud deleted a comment Jan 16, 2019
@nextcloud nextcloud deleted a comment Jan 16, 2019
@nextcloud nextcloud deleted a comment Jan 16, 2019
@AndyScherzinger
Copy link
Member

@tobiasKaminsky erm, my commit is gone?! with my visual changes?!

@AndyScherzinger
Copy link
Member

@tobiasKaminsky rebased and fixed the scan icon. non-fitting QR codes still return back to login screen without any feedback. Just try this one:
frame

@nextcloud nextcloud deleted a comment Jan 18, 2019
@tobiasKaminsky
Copy link
Member Author

2019-01-21-080810

Due to force push, where your qr image gone away, a permission for vibrate was also missing…

@tobiasKaminsky
Copy link
Member Author

Hopefully one last final rebase

@AndyScherzinger
Copy link
Member

AndyScherzinger commented Jan 21, 2019

👍 Nice, all good. Just signing the code is missing...

Approved with PullApprove

tobiasKaminsky and others added 4 commits January 21, 2019 17:27
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
- fix codacy

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
…ip ci]

Signed-off-by: nextcloud-android-bot <android@nextcloud.com>
@nextcloud-android-bot
Copy link
Collaborator

Lint

TypemasterPR
Warnings7169
Errors00

FindBugs (new)

Warning TypeNumber
Bad practice Warnings35
Correctness Warnings156
Internationalization Warnings15
Malicious code vulnerability Warnings12
Multithreaded correctness Warnings9
Performance Warnings117
Security Warnings58
Dodgy code Warnings133
Total535

FindBugs (master)

Warning TypeNumber
Bad practice Warnings35
Correctness Warnings156
Internationalization Warnings15
Malicious code vulnerability Warnings12
Multithreaded correctness Warnings9
Performance Warnings117
Security Warnings58
Dodgy code Warnings133
Total535

@marinofaggiana
Copy link
Member

marinofaggiana commented Mar 4, 2019

@tobiasKaminsky @AndyScherzinger where is the image that you have used for QRCode button ?

@AndyScherzinger
Copy link
Member

@marinofaggiana it from the material design icons list: http://materialdesignicons.com/icon/qrcode-scan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants