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

Fixes exception in preview using regexp search and regexp search without specified field #7073

Merged
merged 12 commits into from
Nov 21, 2020

Conversation

k3KAW8Pnf7mkmdSMPHz27
Copy link
Sponsor Member

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 commented Nov 4, 2020

Fixes #6777 . The issue originates in how the JavaScript regexp is created in PreviewViewer#highlightSearchPattern.

  1. Fixes the use of regexp while searching for one term (e.g., Liu.*)
  2. Fixes exception in the preview when forward slash is used in the regexp
  • Change in CHANGELOG.md described (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 marked this pull request as ready for review November 6, 2020 21:53
@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 changed the title [WIP] Fixes exception in preview using regexp search Fixes exception in preview using regexp search Nov 6, 2020
@k3KAW8Pnf7mkmdSMPHz27
Copy link
Sponsor Member Author

Is there a good spot to put test cases for this issue?

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 changed the title Fixes exception in preview using regexp search Fixes exception in preview using regexp search and regexp search without specified field Nov 6, 2020
@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 marked this pull request as draft November 7, 2020 18:26
@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 marked this pull request as ready for review November 8, 2020 21:54
@k3KAW8Pnf7mkmdSMPHz27
Copy link
Sponsor Member Author

This PR can be made better by some larger reformatting but there might not be a point to doing it given that the code should be completely replaced by Lucene.

@Siedlerchr
Copy link
Member

I think it's not too important to have the highlighting that accurate for each complicated regex and yeah in the longterm we plan to use/convert to lucence. And thanks for finding the issue with the grammar based search + the regex!

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Nov 8, 2020
@Siedlerchr
Copy link
Member

As you are at the search code and as you also worked on the unicode normalization, can you maybe check that issue here as well? #6815

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Sponsor Member Author

As you are at the search code and as you also worked on the unicode normalization, can you maybe check that issue here as well? #6815

Sure, will do !

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Sponsor Member Author

I think it's not too important to have the highlighting that accurate for each complicated regex and yeah in the longterm we plan to use/convert to lucence.

I agree, and I haven't really looked into this in enough detail to contribute. There are definitively still going to be other issues after this PR. It just aims to address the most obvious cases of the / exception.

And thanks for finding the issue with the grammar based search + the regex!

Thank you for the sanity check and help, it is both appreciated and saves me time ^^

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

At first, LGTM.

Minor issue with a magic string replacmenet 😇

May I ask whether its possible to add a test case for the fixed functionality?

src/main/java/org/jabref/gui/preview/PreviewViewer.java Outdated Show resolved Hide resolved
@k3KAW8Pnf7mkmdSMPHz27
Copy link
Sponsor Member Author

May I ask whether its possible to add a test case for the fixed functionality?

I'll look into it tomorrow. I don't have any experience writing tests for this type of code but there should be some "template" I can use elsewhere.

@Siedlerchr
Copy link
Member

Regarding tests, you are lucky, as there is already one: https://github.com/JabRef/jabref/blob/master/src/test/java/org/jabref/model/search/rules/ContainBasedSearchRuleTest.java
(Please also refactor the BibEntry to "withField" )

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 marked this pull request as draft November 10, 2020 23:46
@k3KAW8Pnf7mkmdSMPHz27
Copy link
Sponsor Member Author

k3KAW8Pnf7mkmdSMPHz27 commented Nov 13, 2020

I keep running into the same issue with creating a PreviewViewerTest.

BibDatabaseContext databaseContext = new BibDatabaseContext();
StateManager stateManager = new StateManager();
stateManager.setSearchQuery(new SearchQuery("test", false, false));

PreviewViewer pw = new PreviewViewer(databaseContext, null, stateManager);

As soon as I call the constructor I get a java.lang.ExceptionInInitializerError (i.e., before it enters the constructor)

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 marked this pull request as ready for review November 16, 2020 13:22
@Siedlerchr
Copy link
Member

Siedlerchr commented Nov 21, 2020

If you want to test something on the javafx thread, you need to use the testfx extensions and declare it as GUI test:
See as an example the SourceTabTest under gui.entryeditor.
But that is complicated. I would rather merge this one as it is now. Feel free to create another PR with a gui test

@Siedlerchr Siedlerchr merged commit 6f35c36 into JabRef:master Nov 21, 2020
@k3KAW8Pnf7mkmdSMPHz27
Copy link
Sponsor Member Author

@Siedlerchr @koppor thank you both for your reviews! They are very appreciated!


If you want to test something on the javafx thread, you need to use the testfx extensions and declare it as GUI test:
See as an example the SourceTabTest under gui.entryeditor.
But that is complicated. I would rather merge this one as it is now. Feel free to create another PR with a gui test

Great! In that case, I'll put adding a test case for this issue on my todo-list for later.
Thank you for linking SourceTabTest, now I know where to start digging 😃
I suspect I'll have opportunities to practice writing GUI tests soon X)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

regexp search throws exception when entering slash
3 participants