-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add local search history to the community list screen #871
Conversation
Uploaded screen cap of new functionality |
Can you add a option to disable saving searches, I can imagine that not everyone wants their searches to be saved. |
Based on the screen capture, I think searches should be listed in reverse-chronological order. The last thing I searched should be the first item in history. |
Sorted searches in reverse order, added setting to control whether searches are saved or not. screen cap screen-20230624-121448.2.mp4 |
If you disable the saved searches does it delete the ones it had saved already? |
Updated to clear search history when the option is disabled. |
app/src/main/java/com/jerboa/ui/components/community/list/CommunityListActivity.kt
Outdated
Show resolved
Hide resolved
5bc65c0
to
13a9e0c
Compare
This change adds a search history Dao/Repository/ViewModel so that we can save user searches in Room, and display them on the community list screen. Users can click the history item to search it, or click the "X" button to delete it.
Also, do not insert search history items when saveSearchHistory is false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cant actually run this, causes crash at startup see stacktrace
stack trace
E (1) near "timestamp": syntax error in "CREATE TABLE IF NOT EXISTS SearchHistory( history TEXT PRIMARY KEY NOT NULL timestamp INTEGER NOT NULL )" 2023-06-28 06:01:10.690 6138-6138 SQLiteLog com.jerboa.debug E (1) near "timestamp": syntax error in "CREATE TABLE IF NOT EXISTS SearchHistory( history TEXT PRIMARY KEY NOT NULL timestamp INTEGER NOT NULL )" FATAL EXCEPTION: main Process: com.jerboa.debug, PID: 6138 java.lang.RuntimeException: Unable to start activity ComponentInfo{com.jerboa.debug/com.jerboa.MainActivity}: android.database.sqlite.SQLiteException: near "timestamp": syntax error (code 1 SQLITE_ERROR): , while compiling: CREATE TABLE IF NOT EXISTS SearchHistory( history TEXT PRIMARY KEY NOT NULL timestamp INTEGER NOT NULL ) at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:3635) at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:3792) at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:103) at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:135) at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:95) at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2210) at android.os.Handler.dispatchMessage(Handler.java:106) at android.os.Looper.loopOnce(Looper.java:201) at android.os.Looper.loop(Looper.java:288) at android.app.ActivityThread.main(ActivityThread.java:7839) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1003) Caused by: android.database.sqlite.SQLiteException: near "timestamp": syntax error (code 1 SQLITE_ERROR): , while compiling: CREATE TABLE IF NOT EXISTS SearchHistory( history TEXT PRIMARY KEY NOT NULL timestamp INTEGER NOT NULL ) at android.database.sqlite.SQLiteConnection.nativePrepareStatement(Native Method) at android.database.sqlite.SQLiteConnection.acquirePreparedStatement(SQLiteConnection.java:1047) at android.database.sqlite.SQLiteConnection.prepare(SQLiteConnection.java:654) at android.database.sqlite.SQLiteSession.prepare(SQLiteSession.java:590) at android.database.sqlite.SQLiteProgram.(SQLiteProgram.java:62) at android.database.sqlite.SQLiteStatement.(SQLiteStatement.java:34) at android.database.sqlite.SQLiteDatabase.executeSql(SQLiteDatabase.java:1920) at android.database.sqlite.SQLiteDatabase.execSQL(SQLiteDatabase.java:1842) at androidx.sqlite.db.framework.FrameworkSQLiteDatabase.execSQL(FrameworkSQLiteDatabase.kt:246) at com.jerboa.db.AppDBKt$MIGRATION_17_18$1.migrate(AppDB.kt:527) at androidx.room.RoomOpenHelper.onUpgrade(RoomOpenHelper.kt:91) at androidx.sqlite.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.onUpgrade(FrameworkSQLiteOpenHelper.kt:253) at android.database.sqlite.SQLiteOpenHelper.getDatabaseLocked(SQLiteOpenHelper.java:416) at android.database.sqlite.SQLiteOpenHelper.getWritableDatabase(SQLiteOpenHelper.java:316) at androidx.sqlite.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.getWritableOrReadableDatabase(FrameworkSQLiteOpenHelper.kt:232) at androidx.sqlite.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.innerGetDatabase(FrameworkSQLiteOpenHelper.kt:190) at androidx.sqlite.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.getSupportDatabase(FrameworkSQLiteOpenHelper.kt:151) at androidx.sqlite.db.framework.FrameworkSQLiteOpenHelper.getWritableDatabase(FrameworkSQLiteOpenHelper.kt:104) at androidx.room.RoomDatabase.inTransaction(RoomDatabase.kt:638) at androidx.room.RoomDatabase.assertNotSuspendingTransaction(RoomDatabase.kt:457) at com.jerboa.db.AccountDao_Impl.getAllSync(AccountDao_Impl.java:289) at com.jerboa.db.AccountRepository.getAllSync(AppDB.kt:227) at com.jerboa.db.AccountViewModel.(AppDB.kt:610) at com.jerboa.db.AccountViewModelFactory.create(AppDB.kt:634) at androidx.lifecycle.ViewModelProvider$Factory.create(ViewModelProvider.kt:83) at androidx.lifecycle.ViewModelProvider.get(ViewModelProvider.kt:187) at androidx.lifecycle.ViewModelProvider.get(ViewModelProvider.kt:153) at androidx.lifecycle.ViewModelLazy.getValue(ViewModelLazy.kt:53) at androidx.lifecycle.ViewModelLazy.getValue(ViewModelLazy.kt:35) at com.jerboa.MainActivity.getAccountViewModel(MainActivity.kt:117) at com.jerboa.MainActivity.onCreate(MainActivity.kt:128) at android.app.Activity.performCreate(Activity.java:8051) at android.app.Activity.performCreate(Activity.java:8031) at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1329) at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:3608) ... 12 morehi
CREATE TABLE IF NOT EXISTS SearchHistory(
history TEXT PRIMARY KEY NOT NULL
timestamp INTEGER NOT NULL
)
its missing a comma between history and timestamp
app/src/main/java/com/jerboa/ui/components/settings/lookandfeel/LookAndFeelActivity.kt
Outdated
Show resolved
Hide resolved
This should be working now, thanks for catching this. |
It still crashes at startup but for a different reason this time. The best way to test the migration is to install a version with a DB behind the current. and then install the new app. The stacktrace now isProcess: com.jerboa.debug, PID: 12543 And this is because in your migration your column is named history. But inside your entity the column has name "text". |@Entity
data class SearchHistory(
@PrimaryKey @ColumnInfo(name = "text") val text: String, // Should be history or migration should be text
@ColumnInfo(name = "timestamp") val timestamp: Long,
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a lot of work, because you didn't look at how the other tables were done, and tried to build it from scratch. The SearchHistory table needs some work, as well as its own viewmodel like the other two tables, that can be passed into activities as necessary.
app/src/main/java/com/jerboa/ui/components/community/list/CommunityListViewModel.kt
Outdated
Show resolved
Hide resolved
Sounds good, I'll add another change shortly to make search history its own
view model and address the rest of the feedback.
One question, I added the timestamp column because there was a request to
display history entries sorted in reverse chronological order. Is this
still desired? If not I can remove that column.
…On Mon, Jul 3, 2023, 6:52 AM Dessalines ***@***.***> wrote:
***@***.**** requested changes on this pull request.
This needs a lot of work, because you didn't look at how the other tables
were done, and tried to build it from scratch. The SearchHistory table
needs some work, as well as its own viewmodel like the other two tables,
that can be passed into activities as necessary.
------------------------------
In app/src/main/java/com/jerboa/db/AppDB.kt
<#871 (comment)>:
> @@ -142,8 +149,30 @@ val APP_SETTINGS_DEFAULT = AppSettings(
usePrivateTabs = false,
secureWindow = false,
blurNSFW = true,
+ saveSearchHistory = true,
+)
+
***@***.***
+data class SearchHistory(
+ @PrimaryKey @ColumnInfo(name = "text") val text: String,
No, use auto-gen'd primary keys like the other example.
2ndly, don't name these columns names which might be protected keywords in
sql or kotlin. Name it search_term or something.
3rd: These search terms should almost certainly be attached to an account,
so have a foreign key link to account.id
------------------------------
In app/src/main/java/com/jerboa/db/AppDB.kt
<#871 (comment)>:
> @@ -142,8 +149,30 @@ val APP_SETTINGS_DEFAULT = AppSettings(
usePrivateTabs = false,
secureWindow = false,
blurNSFW = true,
+ saveSearchHistory = true,
+)
+
***@***.***
+data class SearchHistory(
+ @PrimaryKey @ColumnInfo(name = "text") val text: String,
+ @ColumnInfo(name = "timestamp") val timestamp: Long,
This column isn't necessary, the inserts are going to be in order anyway
by the primary key.
------------------------------
In app/src/main/java/com/jerboa/db/AppDB.kt
<#871 (comment)>:
> @@ -173,6 +202,9 @@ interface AppSettingsDao {
@query("SELECT * FROM AppSettings limit 1")
fun getSettings(): LiveData<AppSettings>
+ @query("SELECT * FROM AppSettings limit 1")
Why did this get added? The livedata is already above this.
------------------------------
In app/src/main/java/com/jerboa/db/AppDB.kt
<#871 (comment)>:
> )
***@***.***
+interface SearchHistoryDao {
+ @query("SELECT * FROM SearchHistory")
+ fun history(): Flow<List<SearchHistory>>
Use LiveData like the other examples. Don't code from scratch: use the
conventions currently here.
------------------------------
In app/src/main/java/com/jerboa/db/AppDB.kt
<#871 (comment)>:
> @@ -224,10 +256,26 @@ class AccountRepository(private val accountDao: AccountDao) {
}
}
+class SearchHistoryRepository(
+ private val searchHistoryDao: SearchHistoryDao,
+ private val appSettingsDao: AppSettingsDao,
+) {
+ fun history(): Flow<List<SearchHistory>> = searchHistoryDao.history()
+
+ suspend fun insert(item: SearchHistory) {
+ if (appSettingsDao.settings().first().saveSearchHistory) {
Why is this necessary? It should be connected to an account, not the
appsettings, so it'll already have the account.id in the form.
There's also no need to do a check on this insert. If you have an
appsetting to disable search, that should be done outside of / before this
SQL logic.
------------------------------
In app/src/main/java/com/jerboa/db/AppDB.kt
<#871 (comment)>:
> @@ -242,6 +290,7 @@ class AppSettingsRepository(
@workerthread
suspend fun update(appSettings: AppSettings) {
appSettingsDao.updateAppSettings(appSettings)
+ if (!appSettings.saveSearchHistory) searchHistoryDao.clear()
IMO this isn't necessary, but its up to you. The saved search history
doesn't really need to clear anything, it just needs to visually not show
it.
------------------------------
In app/src/main/java/com/jerboa/db/AppDB.kt
<#871 (comment)>:
> @@ -469,14 +518,32 @@ val MIGRATION_16_17 = object : Migration(16, 17) {
}
}
+val MIGRATION_17_18 = object : Migration(17, 18) {
+ override fun migrate(database: SupportSQLiteDatabase) {
+ database.execSQL(UPDATE_APP_CHANGELOG_UNVIEWED)
+ database.execSQL(
+ "ALTER TABLE AppSettings add column save_search_history INTEGER NOT NULL default 1",
+ )
+ database.execSQL(
+ """
+ CREATE TABLE IF NOT EXISTS SearchHistory(
+ history TEXT PRIMARY KEY NOT NULL,
See above. Primary key issue, column names, and unecessary timestamp
column.
------------------------------
In app/src/main/java/com/jerboa/MainActivity.kt
<#871 (comment)>:
> }
class MainActivity : AppCompatActivity() {
private val siteViewModel by viewModels<SiteViewModel>()
+ private val communityListViewModel by viewModels<CommunityListViewModel>() {
+ CommunityListViewModelFactory((application as JerboaApplication).searchHistoryRepository)
Why did you not make the searchhistory a viewmodel like the other tables?
There's no reason to try to bundle it into another one.
------------------------------
In
app/src/main/java/com/jerboa/ui/components/community/list/CommunityListViewModel.kt
<#871 (comment)>:
>
-class CommunityListViewModel : ViewModel(), Initializable {
- override var initialized by mutableStateOf(false)
+class CommunityListViewModelFactory(private val repository: SearchHistoryRepository) :
No, just make the searchhistory its own viewmodel like the other examples,
and pass in either the viewmodel, or the
searchHistoryViewModel.observeAsState() into the activities.
—
Reply to this email directly, view it on GitHub
<#871 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGSOE3OUU3CPFQ2NJAKKWTXOLFDNANCNFSM6AAAAAAZSTA66M>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
You can just use the primary key for sorting (most recent at the top, or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is also missing the exported 20 schema
- Moved SearchHistoryViewModel to separate file - Updated version to 21
) | ||
database.execSQL( | ||
"CREATE INDEX index_SearchHistory_account_id ON SearchHistory(account_id)", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better, thanks.
""" | ||
CREATE TABLE IF NOT EXISTS SearchHistory( | ||
id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, | ||
account_id INTEGER, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
account_id should also be not null, no? And it'd probably be clearer to move the references / fk relation into this line.
suspend fun insert(item: SearchHistory) | ||
|
||
@Query("DELETE FROM SearchHistory WHERE account_id IS :accountId AND search_term = :searchTerm") | ||
suspend fun delete(accountId: Int?, searchTerm: String) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
account_id should be required.
name = "account_id", | ||
index = true, | ||
) | ||
val accountId: Int?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, not optional.
private val searchHistoryDao: SearchHistoryDao, | ||
) { | ||
fun history(): LiveData<List<SearchHistory>> = searchHistoryDao.history() | ||
.map { history -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't sort / distinct in code, sort using the @Query
in sql.
@@ -49,6 +57,8 @@ fun CommunityListActivity( | |||
siteViewModel: SiteViewModel, | |||
blurNSFW: Boolean, | |||
drawerState: DrawerState, | |||
appSettingsViewModel: AppSettingsViewModel, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems pointless to bring in the entire appSettingsViewModel here. Just add saveSearchHistory
as a param.
@@ -60,9 +70,20 @@ fun CommunityListActivity( | |||
communityListViewModel.setCommunityListFromFollowed(siteViewModel) | |||
} | |||
|
|||
val saveSearchHistory by remember { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary.
val searchHistory by remember(account) { | ||
searchHistoryViewModel.searchHistory | ||
.map { history -> history.filter { it.accountId == account?.id } } | ||
}.observeAsState() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought there was a function that takes in account_id
to filter by that already? If not, one needs to be added.
val appSettingsRepository by lazy { | ||
AppSettingsRepository(database.appSettingsDao(), database.searchHistoryDao()) | ||
} | ||
val searchHistoryRepository by lazy { SearchHistoryRepository(database.searchHistoryDao()) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just add this new one alongside the other two, there's no need to embed them.
""", | ||
) | ||
database.execSQL( | ||
"CREATE INDEX index_SearchHistory_account_id ON SearchHistory(account_id)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consistent naming: idx_search_history_account
Stale PR. I can re-open if necessary. |
This change adds a search history Dao/Repository/ViewModel so that we can save user searches in Room, and display them on the community list screen. Users can click the history item to search it, or click the "X" button to delete it.
This is for issue #811