Skip to content

Conversation

turhantolgaunal
Copy link
Contributor

@turhantolgaunal turhantolgaunal commented Sep 1, 2025

Followup to #13691

Updated the websearch field to be highlighted in accordance with the ANTLR based parser, rather than the old lucene one.

Steps to test

Writing a query to the web search field should turn it red if the query doesn't obey the search syntax rules.

Mandatory checks

  • I own the copyright of the code submitted and I license it under the MIT license
  • I manually tested my changes in running JabRef (always required)
  • [/] I added JUnit tests for changes (if applicable)
  • [/] I added screenshots in the PR description (if change is visible to the user)
  • [/] I described the change in CHANGELOG.md in a way that is understandable for the average user (if change is visible to the user)
  • I checked the user documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request updating file(s) in https://github.com/JabRef/user-documentation/tree/main/en.

int line = offendingToken.getLine();
int charPositionInLine = offendingToken.getCharPositionInLine() + 1;

String errorMessage = "Invalid syntax at line %d, position %d".formatted(line, charPositionInLine);
Copy link

Choose a reason for hiding this comment

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

The error message string is not using sentence case as required for UI text. It should start with a capital letter and maintain consistent casing with other UI messages.

try {
parser.parse(queryText, NO_EXPLICIT_FIELD);
SearchLexer lexer = new SearchLexer(CharStreams.fromString(queryText));
lexer.removeErrorListeners(); // no infos on file system
Copy link

Choose a reason for hiding this comment

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

Comment provides no additional information beyond what is obvious from the code itself. Such trivial comments should be removed according to the commenting guidelines.

@turhantolgaunal turhantolgaunal marked this pull request as ready for review September 4, 2025 17:38
int charPositionInLine = offendingToken.getCharPositionInLine() + 1;

String errorMessage = "Invalid syntax at line %d, position %d".formatted(line, charPositionInLine);
return ValidationMessage.error(Localization.lang(errorMessage));
Copy link
Member

Choose a reason for hiding this comment

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

Please reuse the existing l10n: Localization.lang("Invalid query element '%0' at position %1,line, charPositioninLIne)...

@koppor koppor mentioned this pull request Sep 8, 2025
5 tasks
@Test
void queryConsistingOfInvalidDOIIsValid() {
viewModel.queryProperty().setValue("101.1007/JHEP02(2023)082");
// There is currently no interpretation of nearly-valid identifiers, therefore, this is concidered as "regular" search term
Copy link

Choose a reason for hiding this comment

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

Misspelled word in comment ('concidered' should be 'considered'). Variable names and comments should be correctly spelled to maintain code quality.

@koppor koppor added the status: awaiting-second-review For non-trivial changes label Sep 8, 2025
void queryConsistingOfInvalidDOIIsValid() {
viewModel.queryProperty().setValue("101.1007/JHEP02(2023)082");
assertFalse(viewModel.queryValidationStatus().validProperty().getValue());
// There is currently no interpretation of nearly-valid identifiers, therefore, this is concidered as "regular" search term
Copy link

Choose a reason for hiding this comment

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

The comment merely restates what the code does and contains a spelling error ('concidered'). Comments should provide additional information not obvious from the code.

@Siedlerchr Siedlerchr added this pull request to the merge queue Sep 8, 2025
Merged via the queue into JabRef:main with commit 0eb694b Sep 8, 2025
39 of 46 checks passed
Siedlerchr added a commit that referenced this pull request Sep 8, 2025
* upstream/main: (54 commits)
  Split relativizeSymlinks parameterized tests in separate tests (#13782)
  Update the search syntax highlight for web search (#13801)
  Chore(deps): Bump ai.djl:bom from 0.33.0 to 0.34.0 in /versions (#13833)
  Fix typos in CHANGELOG.md (#13826)
  Chore(deps): Bump com.konghq:unirest-modules-gson in /versions (#13831)
  Chore(deps): Bump org.gradlex:extra-java-module-info in /build-logic (#13830)
  Chore(deps): Bump org.apache.logging.log4j:log4j-to-slf4j in /versions (#13832)
  Chore(deps): Bump io.zonky.test.postgres:embedded-postgres-binaries-bom (#13834)
  Chore(deps): Bump jablib/src/main/resources/csl-locales (#13829)
  Chore(deps): Bump jablib/src/main/resources/csl-styles (#13827)
  Chore(deps): Bump jablib/src/main/abbrv.jabref.org (#13828)
  add: CAYW endpoint formats (#13785)
  New Crowdin updates (#13823)
  chore(deps): update dependency org.kohsuke:github-api to v2.0-rc.5 (#13822)
  Add support for automatic ICORE conference ranking lookup [#13476] (#13699)
  New Crowdin updates (#13820)
  Initialize search bar auto-completion with real database context (no tab switch needed) (#13816)
  Fixes #13274: Allow cygwin-paths on Windows (#13297)
  Refine "REDACTED" replacement of API key value in web fetcher search URL (#13814)
  changed ISSNCleanup into NormalizeIssn, refactored respective tests #13748 (#13767)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting-second-review For non-trivial changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants