Skip to content
New issue

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

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

Already on GitHub? # to your account

Issue4987/previewer issues #4988

Merged
merged 3 commits into from
Oct 2, 2018

Conversation

mikehardy
Copy link
Member

@mikehardy mikehardy commented Sep 21, 2018

Pull Request template

Purpose / Description

3 of the 9 robots google play sent at our last alpha errored in an easy way on the CardBrowser, this fixes that error as well as quite a few CardBrowser/Previewer integration issues

Fixes

Fixes #4987

Approach

  1. commit 1 - fixing the Previewer cycling - showing <, > and - at right times
  2. commit 2 - Preview option menu item in CardBrowser dynamic so it is hidden if the browser is empty (prevents the crash), also save CardBrowser search state when you go to other activities and return
  3. commit 3 - A UI test to exercise all this stuff, but isolated in it's own package and excluded from CI (they are flaky there)

How Has This Been Tested?

A ton by hand, and I made another UI unit test. I posted videos on the linked issue demonstrating before and after

  • You have not changed whitespace unnecessarily (it makes diffs hard to read)
  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • Your code follows the style of the project (e.g. never omit braces in if statements)
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code

@mikehardy mikehardy requested a review from timrae September 21, 2018 07:16
displayCardQuestion();
}
else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please fix the style file? It should be } else {, I think this is the third time I mentioned this :p

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry! And every time I was thinking "yeah, we do elses on new lines", completely missing that you were telling me the opposite. So yes, I'm persistently generating it wrong. Will fix

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Codacy, you want to go here: left bar "Code patterns" -> Pick "Checkstyle" -> seek to "RightCurly". I believe the default setting is what you want and what is specified in our code style doc?

http://checkstyle.sourceforge.net/apidocs/com/puppycrawl/tools/checkstyle/checks/blocks/RightCurlyOption.html#SAME

Our internal / commited AndroidStudio config already does this, as mentioned above I was somehow thinking preference was opposite so had been line-breaking. 😞

@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 21, 2018
@ankidroid ankidroid deleted a comment Sep 27, 2018
@ankidroid ankidroid deleted a comment Sep 27, 2018
@ankidroid ankidroid deleted a comment Sep 27, 2018
@ankidroid ankidroid deleted a comment Sep 27, 2018
@ankidroid ankidroid deleted a comment Sep 27, 2018
@ankidroid ankidroid deleted a comment Sep 27, 2018
@ankidroid ankidroid deleted a comment Sep 27, 2018
@ankidroid ankidroid deleted a comment Sep 27, 2018
@ankidroid ankidroid deleted a comment Sep 27, 2018
@ankidroid ankidroid deleted a comment Sep 27, 2018
@ankidroid ankidroid deleted a comment Sep 27, 2018
@ankidroid ankidroid deleted a comment Sep 27, 2018
@ankidroid ankidroid deleted a comment Sep 27, 2018
@ankidroid ankidroid deleted a comment Sep 27, 2018
@ankidroid ankidroid deleted a comment Sep 27, 2018
@ankidroid ankidroid deleted a comment Sep 27, 2018
@ankidroid ankidroid deleted a comment Sep 27, 2018
@ankidroid ankidroid deleted a comment Sep 27, 2018
@mikehardy mikehardy force-pushed the issue4987/previewer-issues branch from 4a1e8a3 to fff7fd9 Compare September 28, 2018 03:25
- don't display a back arrow when we first display
- while going back, don't skip answer states (back is same as forward now)
- Previously Preview was always available, fixes ankidroid#4987 crash when no cards
- Previously if you went to another Activity with an active search, it was forgotten
@mikehardy mikehardy force-pushed the issue4987/previewer-issues branch from fff7fd9 to 98047f2 Compare October 1, 2018 00:35
@ankidroid ankidroid deleted a comment Oct 1, 2018
@ankidroid ankidroid deleted a comment Oct 1, 2018
@ankidroid ankidroid deleted a comment Oct 1, 2018
@mikehardy
Copy link
Member Author

mikehardy commented Oct 1, 2018

@timrae I just rebased and re-packaged the commits so they are clean and coherent, but didn't change the logic any after peeling out the threading change. I'd like another alpha soon that fixes both crashes #4987 and #5000 and I did my level best to respond to everything here, so I'm going to wait a day or so to give you a chance to raise your hand in case you object but otherwise merge it

@mikehardy mikehardy merged commit 086e129 into ankidroid:master Oct 2, 2018
@mikehardy
Copy link
Member Author

@timrae we have two crash fixes lined up on master now, I don't know what it's like to operate the release machinery but if you wanted to pull the cord and fire it up to send a new alpha out, I'll run around with the broom and dustpan after to pick up any pieces (fingers crossed there aren't any...)

@mikehardy mikehardy deleted the issue4987/previewer-issues branch October 3, 2018 12:01
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash in AbstractFlashCardViewer.onCollectionLoaded() ArrayIndexOutOfBoundsException
2 participants