Skip to content

Conversation

Yubo-Cao
Copy link
Collaborator

@Yubo-Cao Yubo-Cao commented Aug 25, 2025

Closes Yubo-Cao#6

This PR fixes the following error:

  1. IllegalStateException in search walkthrough
  2. IndexOutOfBounds and cannot close database during DialogService.showAndWait in the add group walkthrough

Steps to test

  1. The Search walkthrough should work properly even if multiple library tab is open
  2. The Add Group walkthrough should be more robust to unwanted user interaction. Specifically, you may launch the add group walkthrough and:
    a. Click on other group in the entry tab during the walkthrough without leading to error
    b. Close the add group dialog in the middle without leading to error

Mandatory checks

@Yubo-Cao Yubo-Cao requested a review from calixtus August 25, 2025 00:54
// The callee must guarantee the node to be attached is at least visible
// in the scene to begin with. If such exception is thrown, it indicates
// a bug in the caller side. Consider adjusting WalkthroughResolver.
throw new IllegalStateException("Failed to attach the node.");
Copy link

Choose a reason for hiding this comment

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

Exception is used for control flow rather than an exceptional state. The code should handle the case where node cannot be positioned as a normal flow path instead of throwing an exception.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The exception is intentional to catch programming error. A failed to detach state leads to undefined behavior—whether we should darken the entire window, or to highlight nothing, is questionable—but a sure way to lead to errors in subsequent calls to the Spotlight object.

@subhramit
Copy link
Member

Thanks for quick action!

@subhramit
Copy link
Member

For the submodules thing, see https://devdocs.jabref.org/code-howtos/faq.html#submodules

Siedlerchr
Siedlerchr previously approved these changes Aug 25, 2025
@Siedlerchr
Copy link
Member

You just need to run the openrewrite task, then it will be good

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 25, 2025
@jabref-machine
Copy link
Collaborator

Note that your PR will not be reviewed/accepted until you have gone through the mandatory checks in the description and marked each of them them exactly in the format of [x] (done), [ ] (not done yet) or [/] (not applicable).

@Siedlerchr Siedlerchr enabled auto-merge August 25, 2025 18:32
@Siedlerchr Siedlerchr added this pull request to the merge queue Aug 25, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 25, 2025
@Siedlerchr Siedlerchr merged commit 74f74a0 into JabRef:main Aug 25, 2025
1 check passed
Siedlerchr added a commit that referenced this pull request Sep 8, 2025
* upstream/main: (32 commits)
  Fix path (#13769)
  Mode aware consistency check (#13584)
  Refine JBang check (#13765)
  Add Language Server to the UI and add the integrity/consistency check (#13697)
  Fix/remove comment code (#13763)
  New Crowdin updates (#13760)
  Bump org.openrewrite.rewrite from 7.14.0 to 7.14.1 (#13757)
  Bump com.autonomousapps:dependency-analysis-gradle-plugin (#13756)
  Bump dev.langchain4j:langchain4j-bom from 1.2.0 to 1.3.0 in /versions (#13755)
  Bump jablib/src/main/resources/csl-locales from `fa56de1` to `e29c453` (#13754)
  Bump com.autonomousapps:dependency-analysis-gradle-plugin (#13753)
  Bump org.mockito:mockito-core from 5.18.0 to 5.19.0 in /versions (#13752)
  Bump actions/upload-pages-artifact from 3 to 4 (#13751)
  Migrate fetchers to Search.g4 ANTLR parser. (#13691)
  [Junie]: fix: resolve IllegalArgumentException for non-absolute URIs (#13669)
  Add auto-renaming of linked files on entry data change (#13295)
  Walkthrough additions (#13745)
  Switch from zulu to corretto (#13749)
  New Crowdin updates (#13747)
  Fix copy to (#13741)
  ...
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.

Exception at Walkthough "Search"

4 participants