Update dependencies and fix tests #69
Conversation
| foreignKeys = [ | ||
| ForeignKey(entity = Book::class, parentColumns = [Book.ID], childColumns = [Highlight.BOOK_ID], onDelete = ForeignKey.CASCADE) | ||
| ], | ||
| indices = [Index(value = [Highlight.BOOK_ID])] |
| else -> { | ||
| Toast.makeText(this, getString(R.string.error), Toast.LENGTH_LONG).show() | ||
| } |
There was a problem hiding this comment.
This will show the toast when the user start a search in a book. I think instead we should simply add branches for all the available events.
Currently the SearchFragment is handling the StartNewSearch event which is dangerous as if we revert to a single Channel for event handling, it can by default have only a single observer. Best practice would be to handle all the events in the ReaderActivity, and forward downward any event if needed.
For this PR, I suggest we just move the StartNewSearch event to the ReaderViewModel.FeedbackEvent enum and let the EpubReaderFragment take ownership of this event handling, as it has access to the SearchFragment.
There was a problem hiding this comment.
EpubReaderFragment doesn't have a reference to SearchFragment, all it does show the fragment via the fragment manager, so it's not quite as easy as letting EpubReaderFragment take ownership of event handling for this. But yeah that's an overlook on my part putting a toast error message when it results in showing that when a new search begins. I think the best thing to do is just add an empty else branch.
There was a problem hiding this comment.
Sounds good for now, we can always fix this in a dedicated refactoring.
2 participants
I started out just updating dependencies, but decided to fix some of the issues with unit tests and json.
org.json:jsonlibrary for mocking JSONObject and the like in the tests was not working on newer versions. As it is already usingorg.robolectric:robolectricI annotated each relevant file with@RunWith(RobolectricTestRunner::class)jacksondependencies and deleted theMediaOverlayHandlerclassBOOK_ID, as that warning has been there for several months