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

Regex search #3287

Open
wants to merge 25 commits into
base: develop
Choose a base branch
from
Open

Regex search #3287

wants to merge 25 commits into from

Conversation

JJK96
Copy link
Contributor

@JJK96 JJK96 commented Jul 8, 2024

Describe the pull request content
I added a regular expression search option.

To do this, I had to update Apache Lucene 5 major versions (from 3.6 to 8.11). I tested several languages and searching still works, however testing by native speakers might be necessary. Depends on AndBible/jsword#15. After merging, it might be necessary to invalidate the indexes for all documents.

Screenshots

image

@JJK96 JJK96 requested a review from tuomas2 July 8, 2024 20:10
@@ -142,7 +143,7 @@ android {
/** these config values override those in AndroidManifest.xml. Can also set versionCode and versionName */
defaultConfig {
applicationId = applicationIdStandard
minSdk = 23
minSdk = 26
Copy link
Contributor

Choose a reason for hiding this comment

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

What requires this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, I'll check what breaks if I put it back

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MethodHandle.invoke and MethodHandle.invokeExact are only supported starting with Android O (--min-api 26): Lorg/tartarus/snowball/SnowballProgram;find_among([Lorg/tartarus/snowball/Among;I)I

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lucene requires this:
image

Copy link
Contributor

@tuomas2 tuomas2 Aug 5, 2024

Choose a reason for hiding this comment

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

This is rather conserning in my opinion. We will drop support from very big amount of mobile phones if we drop android 6 and 7. Maybe we need to release two versions, one for old phones and one for new phones if we will merge this...

  • I will check and analyze play store statistics.

Copy link
Contributor

Choose a reason for hiding this comment

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

About 2% of our users are using Android versions 6 / 7 / 7.1.

build.gradle.kts Outdated Show resolved Hide resolved
db/build.gradle.kts Outdated Show resolved Hide resolved
/** can we enable the main menu search button
*/
override val isSearchable: Boolean
get() = try { //TODO allow japanese search - japanese bibles use smartcn which is not available
!currentDocument!!.doesNotExist && "ja" != currentDocument!!.language.code
Copy link
Contributor

Choose a reason for hiding this comment

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

  • todo: check japanese documents

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 performed a quick check by copy-pasting part of the document and searching for it, which worked. But I don't speak/read Japanese, so I don't know if the results made any sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will try to consult my friend chatgpt. He speaks japanese for sure 😊

@tuomas2
Copy link
Contributor

tuomas2 commented Jul 15, 2024

  • implement invalidating old indices

@JJK96
Copy link
Contributor Author

JJK96 commented Aug 5, 2024

  • Bug: Regex search always searches whole bible, not the current book.

@JJK96
Copy link
Contributor Author

JJK96 commented Oct 14, 2024

@tuomas2 I have fixed all my open to-do's for this one. Let me know when you have finished the things you still wanted to check. Also, if you have feedback on my changes, let me know.

@tuomas2
Copy link
Contributor

tuomas2 commented Oct 17, 2024

@tuomas2 I have fixed all my open to-do's for this one. Let me know when you have finished the things you still wanted to check. Also, if you have feedback on my changes, let me know.

Thank you! It's now on top of my TODO list, probably next month having better time...

Copy link
Contributor

Choose a reason for hiding this comment

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

this file does not belong to this PR, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I did not mean to push that, indeed

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like identical to y1ot1nt1_chronological.properties

Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this.

@tuomas2 tuomas2 temporarily deployed to And Bible Builds November 14, 2024 13:49 — with GitHub Actions Inactive
@tuomas2 tuomas2 temporarily deployed to And Bible Builds November 14, 2024 14:24 — with GitHub Actions Inactive
@tuomas2 tuomas2 temporarily deployed to And Bible Builds November 14, 2024 14:35 — with GitHub Actions Inactive
@tuomas2
Copy link
Contributor

tuomas2 commented Nov 14, 2024

  • To be investigated: why production built apk is now 24.4 megabytes! Can it be reduced? There must be something unnecessary coming with new lucene libraries, I think. Before this change it was only 13.2 MB.

@timbze
Copy link
Contributor

timbze commented Nov 14, 2024

  • To be investigated: why production built apk is now 24.4 megabytes! Can it be reduced? There must be something unnecessary coming with new lucene libraries, I think. Before this change it was only 13.2 MB.

Android Studio has apk analyzer with which it's quite easy to see what is taking up the space

@tuomas2
Copy link
Contributor

tuomas2 commented Nov 14, 2024

Screenshot from 2024-11-14 16-49-39

Analyzers bundle a lot of new stuff, especially chinese and japanese analyzers.

@tuomas2
Copy link
Contributor

tuomas2 commented Nov 14, 2024

  • Crashes when indexing bible modules (like KJV or NASB)

@tuomas2
Copy link
Contributor

tuomas2 commented Nov 14, 2024

Leaving out these

//implementation("org.apache.lucene:lucene-analyzers-smartcn:8.11.2")
//implementation("org.apache.lucene:lucene-analyzers-kuromoji:8.11.2")

will save some space. But still it's big.

@tuomas2
Copy link
Contributor

tuomas2 commented Nov 14, 2024

I wish those japanese / chinese analyzers could be bundled as an extension package. Could investigate that option.

-->

seem to be possible. Something like this:


File jarFile = new File(getFilesDir(), "lucene-analyzers-smartcn-8.11.4.jar");
String optimizedDir = getDir("dex", Context.MODE_PRIVATE).getAbsolutePath();

DexClassLoader classLoader = new DexClassLoader(
    jarFile.getAbsolutePath(),
    optimizedDir,
    null,
    getClassLoader()
);

try {
    Class<?> analyzerClass = classLoader.loadClass("org.apache.lucene.analysis.cn.smart.SmartChineseAnalyzer");
    Object analyzerInstance = analyzerClass.newInstance();

    Method analyzeMethod = analyzerClass.getMethod("analyze", String.class);
    analyzeMethod.invoke(analyzerInstance, "测试文本");

} catch (Exception e) {
    e.printStackTrace();
}

So we could bundle these as add-on module. Need to build support for this in jsword/andbible.

BUT. That is probably disallowed by Google. Instead should use Play Feature Delivery. Which probably won't work outside of Google Play-enabled devices.

https://developer.android.com/guide/playcore/feature-delivery

@tuomas2 tuomas2 temporarily deployed to And Bible Builds November 14, 2024 15:16 — with GitHub Actions Inactive
@tuomas2
Copy link
Contributor

tuomas2 commented Nov 14, 2024

Can you @JJK96 try to figure out why it is crashing and fix it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Blockers
Development

Successfully merging this pull request may close these issues.

3 participants