Improved QRCode design #3743

hypercubestart · 2020-03-24T22:51:07Z

See https://forum.opendatakit.org/t/qr-code-screen-redesign/24996/13

I have been working with a team to improve UI/UX the QR code design. Please see the above link for screenshots and prior discussion.

We introduce the QRCodeTabs activity which contains a viewpager in its layout. The viewpager has two tabs: an embedded qr code scanner fragment and a show qr code fragment. We chose this design because other apps we referenced used a similar layout.

This PR also removes the need of a decided ScanQRCodeActivity, since putting the Scan QR Fragment first in the ViewPager allows the Configure via QR Code menu item to send the user directly to the newly created QRCodeTabs activity.

Before submitting this PR, please make sure you have:

[x ] run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
[x ] verified that any code or assets from external sources are properly credited in comments and/or in the about file.

seadowg · 2020-03-25T13:20:07Z

Hey @hypercubestart. Could you make sure to check off the todo list items so we know if this is ready for review or not?

hypercubestart · 2020-03-25T17:58:47Z

@seadowg yep, ready to review

lognaturel · 2020-03-27T22:32:59Z

As I was trying it out, I noticed that switching tabs in landscape crashes:

    java.lang.NullPointerException: Attempt to invoke virtual method 'void com.journeyapps.barcodescanner.DecoratedBarcodeView.pause()' on a null object reference
        at org.odk.collect.android.fragments.QRScannerFragment.onPauseFragment(QRScannerFragment.java:142)
        at org.odk.collect.android.preferences.QRCodeTabs$1.onPageSelected(QRCodeTabs.java:68)
        at androidx.viewpager.widget.ViewPager.dispatchOnPageSelected(ViewPager.java:1947)
        at androidx.viewpager.widget.ViewPager.scrollToItem(ViewPager.java:686)
        at androidx.viewpager.widget.ViewPager.setCurrentItemInternal(ViewPager.java:670)
        at androidx.viewpager.widget.ViewPager.setCurrentItemInternal(ViewPager.java:631)
        at androidx.viewpager.widget.ViewPager.setCurrentItem(ViewPager.java:612)
        at com.google.android.material.tabs.TabLayout$ViewPagerOnTabSelectedListener.onTabSelected(TabLayout.java:3278)
        at com.google.android.material.tabs.TabLayout.dispatchTabSelected(TabLayout.java:1793)
        at com.google.android.material.tabs.TabLayout.selectTab(TabLayout.java:1786)
        at com.google.android.material.tabs.TabLayout.selectTab(TabLayout.java:1746)
        at com.google.android.material.tabs.TabLayout$Tab.select(TabLayout.java:2139)
        at com.google.android.material.tabs.TabLayout$TabView.performClick(TabLayout.java:2327)
        at android.view.View$PerformClick.run(View.java:21256)

hypercubestart · 2020-03-29T19:24:11Z

@lognaturel Thanks for taking a look! Bug is now fixed.
PR changing documentation can be found here: getodk/docs#1204

I would appreciate it if you could review this PR again!

seadowg

Big change around some annoying Android deprecations. Sorry! I'll pause reviewing for you to look at that.

seadowg · 2020-03-30T08:43:34Z

collect_app/src/main/java/org/odk/collect/android/adapters/TabAdapter.java

+import org.odk.collect.android.fragments.QRScannerFragment;
+import org.odk.collect.android.fragments.ShowQRCodeFragment;
+
+public class TabAdapter extends FragmentPagerAdapter  {


We should really be using ViewPager2 and FragmentStateAdapter here. ViewPager was recently deprecated so adding it in now is queuing up rework in the future unfortunately! Would you be able to have a look and see much work it would be to convert these changes over?

yep, updated now! thanks!

seadowg

A few changes inline.

As a bigger piece of feedback it looks like there isn't really a lot of tests in place for the changes in this PR. Right now I can change TabAdapter#getItemCount to just return 0 and comment out onCreateOptionsMenu/onPrepareOptionsMenu to basically remove all the features in this PR and then run tests and I won't see any failures.

As far as I can see there are 4 key "flows" in this PR:

Scan a QR code
View a QR code
Import a QR code (from disk)
Share a QR code with another app

We should make sure we have tests in place to protect these flows and any error cases that could come up. Granted some of this wasn't tested before but as we touch these things we should make sure we don't move on without making sure the new code has a reason to exist. Happy to chat throught the best way to get that testing in place either here or on Slack!

collect_app/build.gradle

collect_app/src/main/java/org/odk/collect/android/adapters/TabAdapter.java

collect_app/src/main/java/org/odk/collect/android/fragments/QRScannerFragment.java

collect_app/src/main/java/org/odk/collect/android/fragments/ShowQRCodeFragment.java

collect_app/src/main/res/values/strings.xml

collect_app/src/main/res/layout/fragment_scan.xml

collect_app/src/main/res/layout/show_qrcode_fragment.xml

collect_app/src/main/java/org/odk/collect/android/preferences/QRCodeTabs.java

lognaturel

Great comments by @seadowg. I'm not sure why I wasn't thinking about tests when we last spoke. He's absolutely right that there's logic here that should be covered. Definitely pull someone into a Slack conversations if you want more guidance/ideas/support on that.

collect_app/src/main/res/values/strings.xml

collect_app/src/main/java/org/odk/collect/android/preferences/qr/QRScannerFragment.java

hypercubestart · 2020-04-28T03:25:44Z

@seadowg the latest commits should improve the test quality of View QR Code by introducing Dependency Inversion into how the QR Code is generated. Last problem I am dealing with is how to test pressShareQRCode and verify the nested intent. I think this PR is ready to go after this.

...ct_app/src/androidTest/java/org/odk/collect/android/preferences/qr/QrCodeActivitiesTest.java

seadowg · 2020-04-29T13:48:31Z

...ct_app/src/androidTest/java/org/odk/collect/android/preferences/qr/QrCodeActivitiesTest.java

+                .clickConfigureQR()
+                .clickOnId(R.id.menu_item_share);
+
+        intended(hasAction(Intent.ACTION_CHOOSER));


I think you still want another assertion here on the file that's sent right?

yes, I'm not quite sure how though, because it may also be helpful to check that the stream is valid

I think you want to check that the intent has the URI in it right?

yes, but this Intent wraps another Intent with contains the URI as an Extra. I'm having trouble testing this inner intent.

nvm, figured it out! can you please take another look? I'm a little worried now that the test is too coupled with the code, especially the expectedUri part

https://github.com/hypercubestart/collect/blob/4005fe290c0d61bdd52271c16e0ce4a9f53cf4ba/collect_app/src/androidTest/java/org/odk/collect/android/preferences/qr/ConfigureWithQRCodeTest.java#L125
Uri expected = FileProvider.getUriForFile(ApplicationProvider.getApplicationContext(),
BuildConfig.APPLICATION_ID + ".provider",
new File(path));

which I copied over directly

seadowg · 2020-04-29T13:50:13Z

collect_app/src/main/java/org/odk/collect/android/injection/config/AppDependencyModule.java

+        return new QRCodeGenerator() {
+            @Override
+            public Observable<Bitmap> generateQRCode(Collection<String> selectedPasswordKeys) {
+                return QRCodeUtils.getQRCodeGeneratorObservable(selectedPasswordKeys);


If possible it'd be nicer for QRCodeUtils.getQRCodeGeneratorObservable's logic to move to this implementation (and probably have it in its own class file). Wrapping like this is a nice trick for dealing with statics we don't control but we do control this one!

Is it okay to break up QRCodeUtils logic like this? Because it uses certain functions/static constants that aren't accessible outside the QRCodeUtils or outside the utils package?

My concern is that If this is the only thing that needs to be moved out, I'm not sure if it's worth it to separate it from the other QRCodeUtils functions which may decrease readability.

I do see your point but it seems to be that everything in that utils provides information about a single QR code image (the image itself, it's path etc) other than decodeFromBitmap. Could it be broken into a QRCodeGenerator and a QRCodeReader interface?

If its okay with you, I went ahead and broke it into QRCodeGenerator interface, and left the remaining functions untouched in the QRCodeUtils.java

seadowg

Looks good! Just one comment but it won't hold up going to QA.

seadowg · 2020-05-08T10:15:09Z

...app/src/androidTest/java/org/odk/collect/android/preferences/qr/ConfigureWithQRCodeTest.java

+    private static final int CHECKER_BACKGROUND_DRAWABLE_ID = R.drawable.checker_background;
+
+    @Rule
+    public IntentsTestRule<MainMenuActivity> rule = new IntentsTestRule<>(MainMenuActivity.class);


We've found recently that we could run into ordering problems with rules so it'd be best if this rule was also part of the RuleChain below. Just remove the @Rule and add around(rule) to the end of the chain.

mmarciniak90 · 2020-05-11T11:18:28Z

I noticed small shortcoming but it's just appearances issue 😃
Lines under tabs in dark mode have blue color instead of the color which matches the theme.

I added a screenshot from Delete Saved Form to compare

mmarciniak90 · 2020-05-11T14:42:20Z

@hypercubestart I also noticed an issue with importing QR code from a file. Even if I select proper QR code, settings aren't applied.

Another thing is scanning inverted QR code which looks like broken.

Additionally, when I scan incorrect QR code I see the message QR does not contain valid settings but I'm not moved to the previous view. That cause that scanning is not 'active' - I need to change device orientation to active scanning again.

hypercubestart · 2020-05-11T18:23:34Z

@mmarciniak90 thanks for all the testing! all issues should be fixed now, except the scanning inverted QR Code. Is this a feature that was always there? Is it something that we should support since it doesn't seem the xzing qr scanning library supports inverted qr code scanning?

@lognaturel tagging for your opinion on this

lognaturel · 2020-05-11T22:41:34Z

Good catches and fixes. 👍

@hypercubestart inverted QR code scanning is supported by XZing and had just been added to Collect: https://github.com/getodk/collect/pull/3622/files. See if that helps you bring it back. I don't think it's critical because configuration happens infrequently and usually in controlled environments so we can always file a separate issue if needed.

For the importing a QR code from a file fix, it would have been nice to see a test alongside it! That kind of broken functionality usually lends itself really well to a red-green cycle -- start with a failing test and then write code that fixes the test. Maybe you can still introduce a test now to prevent future regressions? I think it's more important to get this merged so not a big deal if you don't have time.

hypercubestart · 2020-05-11T23:53:28Z

@lognaturel I don't think I'll be able to introduce the changes by the codefreeze, so we should move those into a separate issue. thanks!

mmarciniak90 · 2020-05-12T10:26:15Z

New issue with inverted QR code reported #3823

@hypercubestart
Noticed incorrect case:

User set Aggregate as a server
Username and password to Aggregate are declared
Admin password is NOT set
User opens QR code - information Contains sensitive information: server password is visible
QR code is scanned by another device

RESULT: Aggregate password is set as a server password as well as ADMIN password

Verified cases with success on Android 6.0, 7.0, 8.1, 9.0:

Lines under tabs in dark mode have the correct color
scanning QR code after scan code without valid settings
import QR code from a file
select a file without QR code as a file to import QR code
share
scanned Central's settings
scanned Aggregate's settings
scanned Google Drive's settings

@getodk-bot unlabel "needs testing"

hypercubestart · 2020-05-12T18:14:02Z

@mmarciniak90 thanks so much for catching that, I really appreciate it. Issue is fixed now

lognaturel · 2020-05-12T18:14:23Z

Good catch, @mmarciniak90, and thanks for the quick fix, @hypercubestart. We can add more tests around that password toggle behavior in a follow-up PR.

mmarciniak90 · 2020-05-13T12:15:33Z

Re-tested cases with success on Android 6.0, 7.0, 8.1, 9.0:

generated QR code with Aggregate password
applied settings from collect.settings file
lines under tabs in dark mode have the correct color
scanned QR code after scan code without valid settings
imported QR code from a file
selected a file without QR code as a file to import QR code
shared
scanned Central's settings
scanned Aggregate's settings
scanned Google Drive's settings

kkrawczyk123 · 2020-05-13T12:34:09Z

Verified the same cases on Androids 5.1 and 10. I have noticed an issue with saving QR code to drive and attaching to email message for my Android 5.1 device but it was also visible on master and store version so I documented it in #3826 in case any user would encounter similar behavior.

@getodk-bot unlabel "needs testing"
@getodk-bot label "behavior verified"

hypercubestart force-pushed the qr_redesign branch from 3dc17d6 to 8b80f1a Compare March 25, 2020 00:03

hypercubestart mentioned this pull request Mar 29, 2020

Update QRCode Import/Export Docs getodk/docs#1204

Merged

seadowg requested changes Mar 30, 2020

View reviewed changes

hypercubestart force-pushed the qr_redesign branch from 3f7491b to b15e1e3 Compare March 31, 2020 23:16

seadowg requested changes Apr 1, 2020

View reviewed changes

lognaturel reviewed Apr 2, 2020

View reviewed changes

collect_app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved

seadowg reviewed Apr 8, 2020

View reviewed changes

collect_app/src/main/java/org/odk/collect/android/preferences/qr/QRScannerFragment.java Show resolved Hide resolved

hypercubestart force-pushed the qr_redesign branch from 1dd845c to c454ed5 Compare April 28, 2020 02:41

seadowg requested changes Apr 29, 2020

View reviewed changes

lognaturel added this to the v1.27 milestone Apr 30, 2020

stlp and others added 15 commits May 6, 2020 14:28

moved some functions to qrcode tabs

5444269

added scan button in toolbar

dba05b0

Added embedded scan (WIP)

80ea5f9

scan ulitity

a7c571c

added permission request

3c9c995

scanner works

5116c3b

beep sound and flashlight for embedded qr camera

4684371

add files from merge commit

8904e1d

moved import qrcode item to overflow for QRCodeTabs

3bcdf63

qr_code redesign

b04c7ca

minor fixes

0b22c75

hook menu item with new QRCodeTabs and fix qrcode generation

bcd27b9

removed unused

092365a

change test name

647138a

fix NPE bug

f43aa17

style

d2c4bf0

hypercubestart force-pushed the qr_redesign branch from c454ed5 to d2c4bf0 Compare May 6, 2020 21:37

hypercubestart added 2 commits May 6, 2020 15:20

update qrcodeutilstest

4959cba

comments

4005fe2

seadowg approved these changes May 8, 2020

View reviewed changes

seadowg added the needs testing label May 8, 2020

hypercubestart added 5 commits May 8, 2020 16:37

rule fixing

644f357

merge

ee35d8a

fix RuleChain

9fb6b6d

fix

ee53a50

remove @rule

b267f65

hypercubestart added 2 commits May 11, 2020 10:53

dark color qr code tabs

65e9dff

continuous qr code + handle import qrcode

1810f6c

mmarciniak90 mentioned this pull request May 12, 2020

Scanning inverted QR code to apply settings does not work #3823

Closed

getodk-bot removed the needs testing label May 12, 2020

fix typo

5318fc1

lognaturel added the needs testing label May 12, 2020

getodk-bot added behavior verified and removed needs testing labels May 13, 2020

seadowg merged commit 010a3ca into getodk:master May 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved QRCode design #3743

Improved QRCode design #3743

hypercubestart commented Mar 24, 2020 •

edited

seadowg commented Mar 25, 2020

hypercubestart commented Mar 25, 2020

lognaturel commented Mar 27, 2020

hypercubestart commented Mar 29, 2020

seadowg left a comment

seadowg Mar 30, 2020

hypercubestart Mar 31, 2020

seadowg left a comment

lognaturel left a comment

hypercubestart commented Apr 28, 2020 •

edited

seadowg Apr 29, 2020

hypercubestart Apr 30, 2020 •

edited

seadowg Apr 30, 2020

hypercubestart Apr 30, 2020 •

edited

hypercubestart May 7, 2020

seadowg Apr 29, 2020

hypercubestart May 1, 2020

seadowg May 1, 2020

hypercubestart May 7, 2020

seadowg left a comment

seadowg May 8, 2020

mmarciniak90 commented May 11, 2020

mmarciniak90 commented May 11, 2020

hypercubestart commented May 11, 2020

lognaturel commented May 11, 2020

hypercubestart commented May 11, 2020

mmarciniak90 commented May 12, 2020

hypercubestart commented May 12, 2020

lognaturel commented May 12, 2020

mmarciniak90 commented May 13, 2020

kkrawczyk123 commented May 13, 2020

Improved QRCode design #3743

Improved QRCode design #3743

Conversation

hypercubestart commented Mar 24, 2020 • edited

Before submitting this PR, please make sure you have:

seadowg commented Mar 25, 2020

hypercubestart commented Mar 25, 2020

lognaturel commented Mar 27, 2020

hypercubestart commented Mar 29, 2020

seadowg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seadowg left a comment

Choose a reason for hiding this comment

lognaturel left a comment

Choose a reason for hiding this comment

hypercubestart commented Apr 28, 2020 • edited

Choose a reason for hiding this comment

hypercubestart Apr 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypercubestart Apr 30, 2020 • edited

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

seadowg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmarciniak90 commented May 11, 2020

mmarciniak90 commented May 11, 2020

hypercubestart commented May 11, 2020

lognaturel commented May 11, 2020

hypercubestart commented May 11, 2020

mmarciniak90 commented May 12, 2020

hypercubestart commented May 12, 2020

lognaturel commented May 12, 2020

mmarciniak90 commented May 13, 2020

kkrawczyk123 commented May 13, 2020

hypercubestart commented Mar 24, 2020 •

edited

hypercubestart commented Apr 28, 2020 •

edited

hypercubestart Apr 30, 2020 •

edited

hypercubestart Apr 30, 2020 •

edited