Skip to content

Delete temp file on exit #12923

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ruarijupp
Copy link

Closes #11048

Describe the changes you have made here: what, where, why, ...

Added support for deleting temporary files on shutdown when using the --deleteWhenClosing argument. This fixes the issue where temp files created by the browser extension were not being removed after JabRef closed.

Changes made:

  • In JabKit.java → Added shutdown hook to delete temp files if provided.
  • In processArguments() → Added handling for --deleteWhenClosing argument.
  • Added unit test for ACMPortalFetcher with correct usage of withField.
  • Updated test assertion to use assertEquals instead of assertNotNull / assertFalse.

Mandatory checks

  • I own the copyright of the code submitted and I license it under the MIT license
  • Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (if change is visible to the user)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked 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 to the documentation repository.

ACMPortalFetcher fetcherSpy = spy(new ACMPortalFetcher());

BibEntry expectedEntry = new BibEntry();
expectedEntry.setField("title", "Machine Learning: A Probabilistic Perspective");
Copy link

Choose a reason for hiding this comment

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

When creating a new BibEntry object, 'withers' should be used instead of 'setField' to improve code readability and maintainability.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, trag-bot is right. Check other test code, there are examples

@Siedlerchr
Copy link
Member

Your pr has changes from another branch or unrelated, you shoudl fix that first

@ruarijupp
Copy link
Author

Your pr has changes from another branch or unrelated, you shoudl fix that first

Thanks for the heads up! Just merged in main and pushed again, should be all good now, let me know if anything else looks off.

Copy link

trag-bot bot commented Apr 13, 2025

@trag-bot didn't find any issues in the code! ✅✨

@ruarijupp
Copy link
Author

Just wanted to check, this seems unrelated to my changes (since I didn't touch .gitmodules or submodules at all).

Is there a recent change to main that I should pull in to resolve this? Or is this an infra thin

@jabref-machine
Copy link
Collaborator

JUnit tests are failing. You can see which checks are failing by locating the box "Some checks were not successful" on the pull request page. To see the test output, locate "Tests / Unit tests (pull_request)" and click on it.

You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.

try {Optional<String> deleteWhenClosingPath = Optional.empty();
JabKit.deleteWhenClosingPath = deleteWhenClosingPath;

for (String arg : args) {
Copy link
Member

Choose a reason for hiding this comment

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

Wrong formatting

*/
public URL getURLForQuery(QueryNode query) throws FetcherException {

/**
Copy link
Member

Choose a reason for hiding this comment

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

Misformatted

uriBuilder.addParameter("AllField", transformedQuery);
uriBuilder.addParameter("startPage", String.valueOf(pageNumber + 1)); // ACM uses 1-based page numbers

// Placeholder: empty result list (real fetching logic happens elsewhere)
Copy link
Member

Choose a reason for hiding this comment

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

Placeholder?

Maybe then add this notice to the doc of the metod?

ACMPortalFetcher fetcherSpy = spy(new ACMPortalFetcher());

BibEntry expectedEntry = new BibEntry();
expectedEntry.setField("title", "Machine Learning: A Probabilistic Perspective");
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, trag-bot is right. Check other test code, there are examples

@Siedlerchr Siedlerchr added the status: changes required Pull requests that are not yet complete label Apr 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: changes required Pull requests that are not yet complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Importing an entry with the browser extension doesn't delete temporary files
4 participants