-
Notifications
You must be signed in to change notification settings - Fork 199
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
Search results with statistics #2061
base: develop
Are you sure you want to change the base?
Conversation
…s-with-Statistics
Don't really like this display though.
It is not a good approach because it is generating the statistics twice.
Fixed highlighting words in a fuzzy or proximity search and code clean up.
|
This code needs plenty of love (I thought the label filtering code didn't need much work but you did a lot on it... so I shudder to think what there is for this). I don't have any problems with the code - it has been bug free for me over the last 6 months. Things to do:
|
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.
First batch of comments
@@ -94,6 +94,11 @@ | |||
android:configChanges="keyboardHidden|orientation|screenSize|locale" | |||
android:label="@string/search" | |||
/> | |||
<activity | |||
android:name="net.bible.android.view.activity.search.MySearchResults" |
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.
Isn't this temporary (2 search results activities)
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.
Maybe. I don't understand much about the manifest :)
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 mean, we have now SearchResults, and MySearchResults. I think SearchResults is deprecated by MySearchResults. End result should be that we remove old SearchResults and rename MySearchResults -> SearchResults.
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 am sure i had answered this somewhere else. Yes, MySearchResults is a duplicate. The old one could be removed.
@@ -232,7 +277,7 @@ class SearchControl @Inject constructor( | |||
const val TARGET_DOCUMENT = "TargetDocument" | |||
private const val STRONG_COLON_STRING = LuceneIndex.FIELD_STRONG + ":" | |||
private const val STRONG_COLON_STRING_PLACE_HOLDER = LuceneIndex.FIELD_STRONG + "COLON" | |||
const val MAX_SEARCH_RESULTS = 1000 | |||
const val MAX_SEARCH_RESULTS = 10000 |
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.
Quite a lot?
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 dont think we need to limit it at all. The most hits i got was about 4000 for some common words. The search itself is very fast. Even big searches in its current form were acceptable (but slow) for me. The complete search results are valuable now because of the stats. My suggestions it make it very large.
private val GENERAL_EPISTLES_COLOR = Color.rgb(0x67, 0xCC, 0x66) // changed 99 to CC to make a little clearer on dark background | ||
private val REVELATION_COLOR = Color.rgb(0xFE, 0x33, 0xFF) | ||
private val OTHER_COLOR = ACTS_COLOR | ||
|
||
public const val BOOK_GRID_FLOW_PREFS = "book_grid_ltr" | ||
public const val BOOK_GRID_FLOW_PREFS_GROUP_BY_CATEGORY = "book_grid_group_by_category" | ||
private const val TAG = "GridChoosePassageBook" | ||
|
||
fun getBookTextColor(bookNo: Int): 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.
Why here? Not used in GridChoosePassageBook activity.
Perhaps need to move all these color-related code somewhere else, if they are used in multiple places.
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 recall what i did here. But yes, they are now used in two places and i have other places i would like to use them as well. I think i wrote some functions to get what i wanted and added them somewhere. And i seem to recall not knowing where the best place to put them would be.
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 think you have moved this and i have incorporated that change.
private const val bookTabPosition = 1 | ||
private const val wordTabPosition = 2 | ||
|
||
class MyTimer(val name: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.
- remove debug code before merge
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.
Done
supportActionBar?.setDisplayHomeAsUpEnabled(true) | ||
isScriptureResultsCurrentlyShown = searchControl.isCurrentlyShowingScripture | ||
|
||
GlobalScope.launch { |
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.
TODO
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 dont know how to run this in the background sorry.
} | ||
} | ||
|
||
private suspend fun prepareResults() { |
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.
most of stuff in Main, only fetching in IO
} | ||
isOk = true | ||
fetchResultsTimer.stop(true) | ||
} catch (e: Exception) { |
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.
too wide catch
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 sure what to narrow it to.
getString(R.string.search_result_count, mSearchResultsHolder!!.size()) | ||
} | ||
withContext(Dispatchers.Main) { | ||
Toast.makeText(this@MySearchResults, msg, Toast.LENGTH_SHORT).show() |
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.
can use ToastEvent
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.
Done i think.
val bookOrdinal = ((key as Verse).book as BibleBook).ordinal | ||
var mBibleBook = BibleBook.values()[bookOrdinal] | ||
var bookNameLong = versification.getLongName(mBibleBook) // key.rootName | ||
val bookStat = bookStatistics.firstOrNull{it.book == bookNameLong} |
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 looks like bookStatistics and other statistics containers could be maps, which is more efficient
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.
Outdated?
Yes, I wanted to keep the old way so i could easily go back and look
at how it worked. But it is not needed.... except if someone hates the new
search and just wants the simple old search results back. If the results
with Statistics works as fast as the old search then there is no need for
both.
|
I took new strings from here to develop already, so that this can be included easily even after 4.1 release if this is not ready before that. I will leave this now waiting for the mentioned improvements (anyone is free to contribute). |
Closes #2049