Feature/logs filtering #24

levinzonr · 2018-12-17T16:04:17Z

added #18

.db file sharing

msmoljan

Hi Roman,

I added some comments regarding change requests. Some of the requests are not trivial, so feel free to ping me anytime for explanation or if you have suggestions :)

Thanks!

msmoljan · 2018-12-18T17:11:45Z

logviewer/src/main/java/com/nodesagency/logviewer/screens/categories/CategoriesFragment.kt

+
+    override fun onOptionsItemSelected(item: MenuItem?): Boolean {
+        return when(item?.itemId) {
+            R.id.actionShareDb -> {


Since we're allowing the users of the library to create their own repository implementation, we could also let them share their backing storage. Additionally, it would be good to move everything I/O-related away from the presentation layer. This also includes the createDbCopy below.

Suggestions:

Create a repository method that tells you if it can offer a shareable "dump" of the backing data store (e.g. it could be a database like in our case, a text file or something else). Because some repositories might not be able to offer this, we have to also be able to return false here and the "share" button should appear/disappear accordingly.

Create another repository method returning a Uri to this dump file directly, so it can be immediately used in the intent. This way we don't care what the file is (e.g. SQLite, txt, csv or sth else), and we're encapsulating the I/O code into the repository implementation.

Totally makes sence, I've add this to LogRepository interface as a getShareableCopyUri() : Uri? ( Need help with this naming ) and implementation of copy is moved to LogDatabase

msmoljan · 2018-12-18T17:17:57Z

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

@@ -2,7 +2,14 @@
    <string name="title_log_categories">Log categories</string>
    <string name="tagTitle">Tag:</string>
    <string name="severityTitle">Severity:</string>
+
+    <!-- TODO: Remove or change this placeholder text -->
+    <string name="hello_blank_fragment">Hello blank fragment</string>


We can remove this and the TODO

msmoljan · 2018-12-18T17:25:38Z

logviewer/src/main/java/com/nodesagency/logviewer/data/database/LogDatabase.kt

@@ -12,7 +12,7 @@ import com.nodesagency.logviewer.data.model.Category
 import com.nodesagency.logviewer.data.model.LogEntry
 import com.nodesagency.logviewer.data.model.Severity

-private const val DATABASE_NAME = "log_database"
+internal const val DATABASE_NAME = "log_database"


Based on a later comment regarding moving the I/O operations behind the repository, the LogDatabase could provide the backing DB's dump as a file Uri through some new method. This way we could also revert this back to private, as this class shouldn't expose any details about the backing DB to the outside world.

msmoljan · 2018-12-18T17:45:33Z

logviewer/src/main/java/com/nodesagency/logviewer/domain/FilterState.kt

@@ -0,0 +1,18 @@
+package com.nodesagency.logviewer.domain
+
+sealed class FilterState(


I assume this class should be internal, as we wouldn't want to expose it to the library users.

Could the name also be simplified to just Filter? It's less redundant and Filter.ByTag would be really nice to read :)

Oh sry, I'm wrong regarding internal, as I see it needs to be exposed through the repository interface.

Also, since it's public, please document the class, as well as its public methods and children with kotlindoc comments (you can use Logger.kt for reference).

It is especially important to document how each filter is expected to behave, i.e. if the query needs to match the string exactly or if it's enough that they're similar in a way. Since we're using the SQL LIKE internally, every other repository implementation should also be expected to behave that way. E.g. you only need to enter a substring of a tag and it will find all tags containing that substring.

Docs are added and class is renamed 👍

msmoljan · 2018-12-18T17:49:52Z

logviewer/src/main/java/com/nodesagency/logviewer/data/LogRepository.kt

@@ -44,4 +46,9 @@ interface LogRepository {
    fun deleteAllCategoriesAndLogs()

    fun getLogDetails(logEntryId: Long): LogDetails
+
+
+    fun getLogsFilteredBy(state: FilterState) : DataSource.Factory<Int, LogEntry>


If the FilterState is renamed to Filter as mentioned in another comment, please rename the parameter to filter as well. :)

msmoljan · 2018-12-18T18:11:16Z

logviewer/src/main/res/menu/menu_categories.xml

+        app:actionViewClass="androidx.appcompat.widget.SearchView" />
+
+    <item
+        app:showAsAction="ifRoom"


The presentation shouldn't know anything about our data layer implementation detail. This should be easily replacable after the other comments are accounted for.

Didn't quite whats this is referring to, hope it's fixed already

msmoljan · 2018-12-18T18:15:12Z

logviewer/src/main/res/menu/menu_categories.xml

+
+        <menu>
+            <item
+                android:id="@+id/actionShareDb"


Related to the other comments, at this point we shouldn't care if what we're sharing is a database, an XML file or something else, as long as it's some kind of a log dump file.

Hence we could name this something like actionShareLogDump. Please also adjust the title string in the same way (both the resource name and the actual string).

msmoljan · 2018-12-18T18:27:25Z

logviewer/src/main/java/com/nodesagency/logviewer/data/database/daos/CategoryDao.kt

@@ -16,6 +16,9 @@ internal interface CategoryDao {
    @Query("SELECT * FROM Categories ORDER BY name ASC")
    fun getAlphabeticallySortedCategories(): DataSource.Factory<Int, Category>

+    @Query("SELECT * FROM Categories WHERE (lower(name) LIKE :name)")


Will this return the proper category if the :name is "I/O", but the name (the stored value compared) is lowercased to "i/o"?

LIKE is supposed to be case insensitive so I've removed lower() completely. Will cover this case with unit tests later

msmoljan · 2018-12-18T18:30:55Z

logviewer/src/main/java/com/nodesagency/logviewer/data/database/daos/LogEntryDao.kt

@@ -18,4 +18,13 @@ internal interface LogEntryDao {
    @Insert
    fun insert(entry: LogEntry)



Similar question regarding lowercasing for getLogsWithSeverity, getLogsWithMessage and getLogsWithTag

Will this return the proper category if the :name is "I/O", but the name (the stored value compared) is lowercased to "i/o"?

LIKE is supposed to be case insensitive so I've removed lower() completely. Will cover this case with unit tests later

msmoljan · 2018-12-18T18:40:23Z

logviewer/src/main/java/com/nodesagency/logviewer/data/database/DatabaseLogRepository.kt

@@ -74,4 +79,22 @@ internal class DatabaseLogRepository(
            .logDetailsDao()
            .getLogDetails(logEntryId)
    }
+
+
+    override fun getLogsFilteredBy(state: FilterState): DataSource.Factory<Int, LogEntry> {


It would be nice to cover this and getCategoriesByName with tests if you have some time. Since they are a bit smarter than the rest of the class, it would be good to assure they're performing 100% correctly. You can use LoggerTest.kt for reference. You can also reuse its code for in-memory DB creation etc.

The other methods are more trivial so I was lazy to add tests for those. Shame on me and I should have done it regardless, will make up for it at some point :)

levinzonr added 9 commits December 14, 2018 16:47

Filters view setup

99fac29

Tmp

9234a91

Merge branch 'develop' into feature/logsFiltering

e80252b

Added tag an message filters

cfc9e6b

State management, clean up

8532b87

Added severity filtering

ca16d48

Add Categories filtering

1568156

Added option for sharing database file

4198822

Button for disabling filters in logs

8d6f195

levinzonr requested a review from msmoljan December 17, 2018 16:04

msmoljan suggested changes Dec 18, 2018

View reviewed changes

msmoljan assigned msmoljan and levinzonr and unassigned msmoljan Dec 19, 2018

levinzonr added 8 commits December 19, 2018 14:41

Moved shareable dump uri copy logic data layer

3b10862

Added javadoc for Filter, Names adjustments

8148a97

Filters messages to strings.xml

9143a32

Messages adjustments

2d7bb89

Removed lower()

022da84

Started with db unit tests

227194f

Added unit tests filtering and some dao methods

e83775c

Added all logs sharing option, adjusted backing storage copy sharing

95f1abb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/logs filtering #24

Feature/logs filtering #24

levinzonr commented Dec 17, 2018

msmoljan left a comment

msmoljan Dec 18, 2018

levinzonr Dec 19, 2018 •

edited

msmoljan Dec 18, 2018

levinzonr Dec 19, 2018

msmoljan Dec 18, 2018

levinzonr Dec 19, 2018

msmoljan Dec 18, 2018

msmoljan Dec 18, 2018

msmoljan Dec 18, 2018

msmoljan Dec 18, 2018

levinzonr Dec 19, 2018

msmoljan Dec 18, 2018

levinzonr Dec 19, 2018

msmoljan Dec 18, 2018

levinzonr Dec 19, 2018

msmoljan Dec 18, 2018

levinzonr Dec 19, 2018

msmoljan Dec 18, 2018

levinzonr Dec 19, 2018

msmoljan Dec 18, 2018

levinzonr Dec 19, 2018

msmoljan Dec 18, 2018

levinzonr Dec 19, 2018

		@@ -0,0 +1,18 @@
		package com.nodesagency.logviewer.domain

		sealed class FilterState(

		@@ -18,4 +18,13 @@ internal interface LogEntryDao {
		@Insert
		fun insert(entry: LogEntry)

Feature/logs filtering #24

Are you sure you want to change the base?

Feature/logs filtering #24

Conversation

levinzonr commented Dec 17, 2018

msmoljan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levinzonr Dec 19, 2018 • 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

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

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

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

levinzonr Dec 19, 2018 •

edited