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

JAMES-4100 Improve Search Snippet display #2583

Merged

Conversation

hungphan227
Copy link
Contributor

No description provided.

Copy link
Contributor

@Arsnael Arsnael left a comment

Choose a reason for hiding this comment

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

Read it, nothing to add

@quantranhong1999
Copy link
Contributor

@Arsnael
Copy link
Contributor

Arsnael commented Jan 8, 2025

@chibenwa
Copy link
Contributor

chibenwa commented Jan 8, 2025

Have you tried

StringUtils.remove(s, '>);

Or

CharMatcher.is('>').removeFrom(s)

?

@hungphan227
Copy link
Contributor Author

Have you tried

StringUtils.remove(s, '>);

Or

CharMatcher.is('>').removeFrom(s)

?

Don't you want to keep at least one character after removing?

@chibenwa
Copy link
Contributor

chibenwa commented Jan 8, 2025

I do not know: how would it look like in the end user display?

@hungphan227
Copy link
Contributor Author

I do not know: how would it look like in the end user display?

Example:
Input: >>>>>>>>>> append contentA to >>> inbox >>>>>>
Result: > append contentA to > inbox >

(contentA is the highlighted part)

@chibenwa
Copy link
Contributor

chibenwa commented Jan 8, 2025

I wonder if append contentA to INBOX wouldn't be better...

@hungphan227
Copy link
Contributor Author

hungphan227 commented Jan 8, 2025

I wonder if append contentA to INBOX wouldn't be better...

The solution for this is not just StringUtils.remove(s, '>);. We should not remove > from <mark> tag of search snippet.

@Arsnael
Copy link
Contributor

Arsnael commented Jan 13, 2025

@hungphan227 Status of this?

Tests still not fixed I believe => https://ci-builds.apache.org/job/james/job/ApacheJames/job/PR-2583/1/testReport/

@hungphan227
Copy link
Contributor Author

Actually I don't know what I should do.

Either removing all > or keeping just one >, there is no way to make it simple like StringUtils.remove(s, '>);. I know only two ways: either using regex or a loop to iterate through the string.

Therefore, I suggest closing this ticket and leave things as before. Anyway removing > is not really important.

@Arsnael
Copy link
Contributor

Arsnael commented Jan 13, 2025

Anyway removing > is not really important.

An end user might disagree with you on that I think :)

Honestly I get that removing entirely the trailings > is tricky and might require a non efficient process as you need to be careful with initial > potentially present in the message (and you don't want to break the <mark> tags)

I would be ok with the initial solution, aka folding >>>>> into > but need to fix current breaking related tests.

@chibenwa what are your thoughts here?

@chibenwa
Copy link
Contributor

Ok

Copy link
Contributor

@Arsnael Arsnael left a comment

Choose a reason for hiding this comment

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

LGTM but did you try to run locally related tests though? Would be surprised that everything is green :)

@hungphan227
Copy link
Contributor Author

hungphan227 commented Jan 15, 2025

LGTM but did you try to run locally related tests though? Would be surprised that everything is green :)

Fixed in mailbox/lucene/src/main/java/org/apache/james/mailbox/lucene/search/LuceneSearchHighlighter.java

Sth wrong with CI. Need to restart CI

@Arsnael
Copy link
Contributor

Arsnael commented Jan 15, 2025

Sth wrong with CI. Need to restart CI

=> #2599 is what's wrong with the CI :)

@chibenwa
Copy link
Contributor

Can we have a look at how it looks like with the frontend?

@hungphan227 hungphan227 force-pushed the JAMES-4100-Improve-Search-Snippet-display branch from cc14340 to 5089a3c Compare January 20, 2025 09:39
@hungphan227
Copy link
Contributor Author

Updated to remove before indexing

@hungphan227 hungphan227 force-pushed the JAMES-4100-Improve-Search-Snippet-display branch from 5e4df50 to bf8d881 Compare January 21, 2025 03:16
@hungphan227
Copy link
Contributor Author

03:21:52,863 [ERROR] Failed to execute goal org.apache.maven.plugins:maven-remote-resources-plugin:1.6.0:process (default) on project apache-mailet-ai: Execution default of goal org.apache.maven.plugins:maven-remote-resources-plugin:1.6.0:process failed: Could not acquire lock(s)

@Arsnael could you help me restart?

@Arsnael
Copy link
Contributor

Arsnael commented Jan 22, 2025

@chibenwa chibenwa merged commit a6f9f32 into apache:master Jan 22, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants