Allow barcodeless cards #324
Conversation
There was a problem hiding this comment.
Reviewed 6 of 7 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @TheLastProject)
a discussion (no related file):
For the discoverability of this feature, it may be better if the "This card has no barcode" button is at the top of the list instead of the bottom. Maybe also the button should be visible the entire time but when there is no text typed the button is disabled.
Additionally, should the instructions be updated? Maybe
Enter the card ID then select the image which represents the barcode you want to use, or select "This card has no barcode" to not use a barcode.
app/src/test/java/protect/card_locker/LoyaltyCardViewActivityTest.java, line 250 at r2 (raw file):
assertEquals(0, db.getLoyaltyCardCount()); cardIdField.setText("cardId");
Should there be a test which demonstrates that a card without a barcode can be created successfully and also loaded successfully?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @brarcher)
a discussion (no related file):
Previously, brarcher (Branden Archer) wrote…
For the discoverability of this feature, it may be better if the "This card has no barcode" button is at the top of the list instead of the bottom. Maybe also the button should be visible the entire time but when there is no text typed the button is disabled.
Additionally, should the instructions be updated? Maybe
Enter the card ID then select the image which represents the barcode you want to use, or select "This card has no barcode" to not use a barcode.
I'm a bit worried that if there is one button above all the barcodes, people will press the button without thinking and reading instructions because "oh, button". I think the button could be on top maybe if we add a button style to the actual barcodes. What do you think?
app/src/test/java/protect/card_locker/LoyaltyCardViewActivityTest.java, line 250 at r2 (raw file):
Previously, brarcher (Branden Archer) wrote…
Should there be a test which demonstrates that a card without a barcode can be created successfully and also loaded successfully?
Fair point, will add that
This prevents a bug with an empty PDF 417 barcode being generated
There was a problem hiding this comment.
Reviewable status: 3 of 8 files reviewed, 2 unresolved discussions (waiting on @brarcher)
a discussion (no related file):
Previously, TheLastProject (Sylvia van Os) wrote…
I'm a bit worried that if there is one button above all the barcodes, people will press the button without thinking and reading instructions because "oh, button". I think the button could be on top maybe if we add a button style to the actual barcodes. What do you think?
Nevermind, it works now.
I also fixed an unrelated bug in the process with empty PDF 417 barcodes, just stating this for future reference:
There was a problem hiding this comment.
Reviewable status: 3 of 8 files reviewed, 2 unresolved discussions (waiting on @brarcher)
a discussion (no related file):
Previously, TheLastProject (Sylvia van Os) wrote…
Nevermind, it works now.
I also fixed an unrelated bug in the process with empty PDF 417 barcodes, just stating this for future reference:
Done.
app/src/test/java/protect/card_locker/LoyaltyCardViewActivityTest.java, line 250 at r2 (raw file):
Previously, TheLastProject (Sylvia van Os) wrote…
Fair point, will add that
Done.
There was a problem hiding this comment.
Reviewed 5 of 5 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @brarcher and @TheLastProject)
app/src/main/java/protect/card_locker/BarcodeImageWriterTask.java, line 93 at r3 (raw file):
public Bitmap doInBackground(Void... params) { if (cardId.isEmpty())
This seems like a fix of a symptom instead of the root cause. I see without this PR that if one enter some text for the Card ID then deletes the PDF 417 barcode does show an option. Would it be better to catch this case in BarcodeSelectorActivity, maybe in createBarcodeOption()?
app/src/test/java/protect/card_locker/LoyaltyCardViewActivityTest.java, line 46 at r3 (raw file):
@RunWith(RobolectricTestRunner.class) @Config(constants = BuildConfig.class, sdk = 23) public class LoyaltyCardViewActivityTest
I do not think there are currently tests for the barcode selector activity. Would you mind also adding a test that demonstrates when the Card ID field is empty that the button is disabled, and when there is text and the button is clicked it returns the expected result?
app/src/test/java/protect/card_locker/LoyaltyCardViewActivityTest.java, line 543 at r3 (raw file):
Ah, I see there was a typo in the comment from the test above this one. Originally this app was something else, and I forked it to be this app. I guess I did not fix some of the comments.(:
You do not have to fix the other comments. Please do update this comment though:
Save and check the loyalty card
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @brarcher)
app/src/main/java/protect/card_locker/BarcodeImageWriterTask.java, line 93 at r3 (raw file):
Previously, brarcher (Branden Archer) wrote…
This seems like a fix of a symptom instead of the root cause. I see without this PR that if one enter some text for the Card ID then deletes the PDF 417 barcode does show an option. Would it be better to catch this case in BarcodeSelectorActivity, maybe in createBarcodeOption()?
I have implemented your request in 62f7241, but I'm not sure if it's cleaner because:
- Now we have a difference in behaviour for the PDF 417 code generation (even if it's just in a single edge case)
- Both the BarcodeImageWriter and the BarcodeSelectorActivity control the visibility of the ImageView object now, instead of this being controlled completely by the BarcodeImageWriter
Up to you though.
app/src/test/java/protect/card_locker/LoyaltyCardViewActivityTest.java, line 46 at r3 (raw file):
Previously, brarcher (Branden Archer) wrote…
I do not think there are currently tests for the barcode selector activity. Would you mind also adding a test that demonstrates when the Card ID field is empty that the button is disabled, and when there is text and the button is clicked it returns the expected result?
Done. I wanted to also test the values of the other fields but I didn't manage to get that done because of all the async code.
app/src/test/java/protect/card_locker/LoyaltyCardViewActivityTest.java, line 543 at r3 (raw file):
Previously, brarcher (Branden Archer) wrote…
Ah, I see there was a typo in the comment from the test above this one. Originally this app was something else, and I forked it to be this app. I guess I did not fix some of the comments.(:
You do not have to fix the other comments. Please do update this comment though:
Save and check the loyalty card
Done.
|
I just noticed that this will break the CsvDatabaseImport so I quickly patched that out. Probably should add another ImportExportTest too. |
There was a problem hiding this comment.
Reviewed 4 of 5 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @brarcher and @TheLastProject)
app/src/main/java/protect/card_locker/BarcodeImageWriterTask.java, line 93 at r3 (raw file):
Previously, TheLastProject (Sylvia van Os) wrote…
I have implemented your request in 62f7241, but I'm not sure if it's cleaner because:
- Now we have a difference in behaviour for the PDF 417 code generation (even if it's just in a single edge case)
- Both the BarcodeImageWriter and the BarcodeSelectorActivity control the visibility of the ImageView object now, instead of this being controlled completely by the BarcodeImageWriter
Up to you though.
I went back and forth on this, and in the end I think you are right. Maybe it is better for the BarcodeImageWriter to control the visibility. Could you move it back to the way you had it? Thanks!
app/src/main/java/protect/card_locker/CsvDatabaseImporter.java, line 134 at r5 (raw file):
From your comment:
I just noticed that this will break the CsvDatabaseImport so I quickly patched that out. Probably should add another ImportExportTest too.
Good find! That did not occur to me to test. Could you write a test for this case as well in ImportExportTest?
app/src/test/java/protect/card_locker/BarcodeSelectorActivityTest.java, line 56 at r4 (raw file):
cardId.setText("abcdefg"); activityController.pause();
Why is it necessary to pause and resume the activity? Is the issue that the event which updates the button may not have been run yet? I think there is a way to drain the event queue in Robolectric, which may be more direct. Does either of these remove the need for the pause/resume:
shadowOf(getMainLooper()).idle()
shadowOf(getMainLooper()).runUiThreadTasksIncludingDelayedTasks()
References:
http://robolectric.org/javadoc/3.1/org/robolectric/shadows/ShadowLooper.html#runUiThreadTasksIncludingDelayedTasks()
http://robolectric.org/javadoc/3.0/org/robolectric/shadows/ShadowLooper.html#idle--
app/src/test/java/protect/card_locker/LoyaltyCardViewActivityTest.java, line 46 at r3 (raw file):
Previously, TheLastProject (Sylvia van Os) wrote…
Done. I wanted to also test the values of the other fields but I didn't manage to get that done because of all the async code.
My other comment mentions a way to run the async tasks in the test, namely using
shadowOf(getMainLooper()).idle()
Might that help in testing the other fields?
There was a problem hiding this comment.
I see that your ability to create pull requests for new features outstrips my time to review them (:
I appreciate your involvement in the project, and thanks for your patience as it takes me time to look over the proposed changes.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @brarcher and @TheLastProject)
This reverts commit 62f7241.
There was a problem hiding this comment.
Reviewable status: 8 of 10 files reviewed, 6 unresolved discussions (waiting on @brarcher and @TheLastProject)
app/src/main/java/protect/card_locker/BarcodeImageWriterTask.java, line 93 at r3 (raw file):
Previously, brarcher (Branden Archer) wrote…
I went back and forth on this, and in the end I think you are right. Maybe it is better for the BarcodeImageWriter to control the visibility. Could you move it back to the way you had it? Thanks!
Done.
There was a problem hiding this comment.
Reviewable status: 8 of 11 files reviewed, 6 unresolved discussions (waiting on @brarcher and @TheLastProject)
app/src/main/java/protect/card_locker/CsvDatabaseImporter.java, line 134 at r5 (raw file):
Previously, brarcher (Branden Archer) wrote…
From your comment:
I just noticed that this will break the CsvDatabaseImport so I quickly patched that out. Probably should add another ImportExportTest too.
Good find! That did not occur to me to test. Could you write a test for this case as well in ImportExportTest?
Done.
There was a problem hiding this comment.
Reviewable status: 8 of 11 files reviewed, 6 unresolved discussions (waiting on @brarcher)
app/src/test/java/protect/card_locker/BarcodeSelectorActivityTest.java, line 56 at r4 (raw file):
Previously, brarcher (Branden Archer) wrote…
Why is it necessary to pause and resume the activity? Is the issue that the event which updates the button may not have been run yet? I think there is a way to drain the event queue in Robolectric, which may be more direct. Does either of these remove the need for the pause/resume:
shadowOf(getMainLooper()).idle()shadowOf(getMainLooper()).runUiThreadTasksIncludingDelayedTasks()References:
http://robolectric.org/javadoc/3.1/org/robolectric/shadows/ShadowLooper.html#runUiThreadTasksIncludingDelayedTasks()
http://robolectric.org/javadoc/3.0/org/robolectric/shadows/ShadowLooper.html#idle--
Both of those work it seems
app/src/test/java/protect/card_locker/LoyaltyCardViewActivityTest.java, line 46 at r3 (raw file):
Previously, brarcher (Branden Archer) wrote…
My other comment mentions a way to run the async tasks in the test, namely using
shadowOf(getMainLooper()).idle()Might that help in testing the other fields?
Sadly, the other fields don't update. Now with .idle nor with .runUiThreadTasksIncludingDelayedTasks
There was a problem hiding this comment.
Reviewed 3 of 3 files at r6.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @brarcher and @TheLastProject)
app/src/test/java/protect/card_locker/BarcodeSelectorActivityTest.java, line 56 at r4 (raw file):
Previously, TheLastProject (Sylvia van Os) wrote…
Both of those work it seems
Why not go with
shadowOf(getMainLooper()).idle()
as there are no delayed tasks being used, only ones which attempt to run immediately.
app/src/test/java/protect/card_locker/ImportExportTest.java, line 386 at r6 (raw file):
boolean result = MultiFormatImporter.importData(db, inStream, DataFormat.CSV); assertEquals(true, result); assertEquals(1, db.getLoyaltyCardCount());
Can you check the fields are as expected? See importWithoutNullColors() for an example.
There was a problem hiding this comment.
Reviewable status: 9 of 11 files reviewed, 3 unresolved discussions (waiting on @brarcher)
app/src/test/java/protect/card_locker/BarcodeSelectorActivityTest.java, line 56 at r4 (raw file):
Previously, brarcher (Branden Archer) wrote…
Why not go with
shadowOf(getMainLooper()).idle()as there are no delayed tasks being used, only ones which attempt to run immediately.
Done.
app/src/test/java/protect/card_locker/ImportExportTest.java, line 386 at r6 (raw file):
Previously, brarcher (Branden Archer) wrote…
Can you check the fields are as expected? See importWithoutNullColors() for an example.
Done.
There was a problem hiding this comment.
Thanks for contributing this feature!
Reviewed 2 of 2 files at r7.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved
|
Thank you for your patience and willingness to review my contributions :) |
Fixes #291.
This change is