Skip to content
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

Draft
wants to merge 50 commits into
base: develop
Choose a base branch
from
Draft

Conversation

tuomas2
Copy link
Contributor

@tuomas2 tuomas2 commented Jan 19, 2022

Closes #2049

It is not a good approach because it is generating the statistics twice.
@tuomas2
Copy link
Contributor Author

tuomas2 commented May 7, 2022

  • Search activity does not have defaults set (Whole Bible, All words should be set by default)

@agrogers
Copy link
Contributor

agrogers commented May 7, 2022

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:

  • Do the search in a different thread. At present it locks the interface so long searches are too slow.
  • Display the results progressively. The first results can be shown almost immediately because the search itself is fast. It is the retrieval of the entire verse and the parsing of those verses that is slow.
  • Maybe find a better way to parse the results. Parsing is needed to work out the word stats. That is, it needs to look at the text of every verse, search for the Strongs number (or word), work out what words it applies to, create a list of those words and count their uses etc
  • The 'Verses' and 'Books' totals can be displayed immediately but the 'Words' total must be delayed until all the verses have been parsed. So indicate that the Word total is incomplete (perhaps '1+' which updates as the words are found)
  • 'Show key words only' is not implemented. A strongs search will often show a LOT of word variations. My idea was to consolidate those words as much as possible. The plan was to look through the list for all single words longer than 3 characters. Then, if that single word is used elsewhere, collapse the other hits into that word. For example, H7641 gives 'among the ears of grain, branches, ears, ears of grain, of grain, the ears'. My approach would reduce the results to 'ears, branches, of grain' which is more useful from a statistics perspective. Some strongs search results give a crazy long list of words. I feel this would give reasonable results 90% of the time.

Copy link
Contributor Author

@tuomas2 tuomas2 left a 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"
Copy link
Contributor Author

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)

Copy link
Contributor

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 :)

Copy link
Contributor Author

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.

Copy link
Contributor

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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quite a lot?

Copy link
Contributor

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 {
Copy link
Contributor Author

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.

Copy link
Contributor

@agrogers agrogers May 7, 2022

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.

Copy link
Contributor

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){
Copy link
Contributor Author

@tuomas2 tuomas2 May 7, 2022

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

Copy link
Contributor

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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO

Copy link
Contributor

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() {
Copy link
Contributor Author

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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

too wide catch

Copy link
Contributor

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()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

can use ToastEvent

Copy link
Contributor

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}
Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Outdated?

@agrogers
Copy link
Contributor

agrogers commented May 20, 2022 via email

@tuomas2 tuomas2 marked this pull request as draft May 26, 2022 08:16
tuomas2 added a commit that referenced this pull request May 26, 2022
@tuomas2
Copy link
Contributor Author

tuomas2 commented May 26, 2022

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).

@agrogers
Copy link
Contributor

I have implemented the 'Show key words only' now. Done very little testing on it. Will do that over the next few weeks. It's rudimentary but seems to give good results.
image
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting for others
Development

Successfully merging this pull request may close these issues.

New search statistics display proposal
2 participants