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.

Sign up for GitHub

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

Jump to bottom

Fix threading cleanup in performSearch #7672

Merged
merged 5 commits into from
Apr 27, 2021
Merged

Fix threading cleanup in performSearch #7672

merged 5 commits into from
Apr 27, 2021

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Apr 26, 2021 •
edited
Loading

Fixes #7606
Fixes #1018

  • Change in CHANGELOG.md described in a way that is understandable for the average user (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.

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 26, 2021
tobiasdiez
tobiasdiez requested changes Apr 26, 2021
View reviewed changes
src/main/java/org/jabref/gui/importer/ImportEntriesViewModel.java Outdated
// store the complete parser result (to import groups, ... later on)
this.parserResult = parserResult;

List<BibEntry> resultEntries = parserResult.getDatabase().getEntries().stream().map(cleanup::doPostCleanup).collect(Collectors.toList());
Copy link
Member

Choose a reason for hiding this comment

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

An idea would be to move this cleanup to the ImportHandler for consistency reasons.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is already part of the importHandler for importing external files

jabref/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java

Lines 165 to 169 in bb011c9

public void importEntries(List<BibEntry> entries) {
ImportCleanup cleanup = new ImportCleanup(bibdatabase.getMode());
cleanup.doPostCleanup(entries);
bibdatabase.getDatabase().insertEntries(entries);

Copy link
Member

Choose a reason for hiding this comment

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

Okay, then I don't get why you added it in the dialog. The buildImportHandlerThenImportEntries method uses the import handler to import entries.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah thanks, then it can be removed here. Totally overlooked that this is called.

src/main/java/org/jabref/gui/importer/ImportEntriesViewModel.java Outdated
@@ -52,6 +55,7 @@
private final PreferencesService preferences;
private final BibEntryTypesManager entryTypesManager;

private final ImportCleanup cleanup = new ImportCleanup(BibDatabaseMode.BIBTEX);
Copy link
Member

Choose a reason for hiding this comment

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

If one uses the user database, then this would immediately take care of #1018 as well, right?

src/main/java/org/jabref/logic/importer/fetcher/CompositeSearchBasedFetcher.java Show resolved Hide resolved
tobiasdiez
tobiasdiez approved these changes Apr 27, 2021
View reviewed changes
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

LGTM

src/main/java/org/jabref/gui/importer/ImportEntriesViewModel.java Outdated
@@ -51,6 +52,7 @@
private final ObservableList<BibEntry> entries;
private final PreferencesService preferences;
private final BibEntryTypesManager entryTypesManager;
private ImportCleanup cleanup;
Copy link
Member

Choose a reason for hiding this comment

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

This is no longer required now.

@Siedlerchr Siedlerchr merged commit d320975 into main Apr 27, 2021
@Siedlerchr Siedlerchr deleted the fixThreadingIn branch April 27, 2021 16:44
Siedlerchr added a commit that referenced this pull request Apr 28, 2021
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into fixCleanup
0ed2966 
* upstream/main:
  Fix threading cleanup in performSearch (#7672)
Siedlerchr added a commit that referenced this pull request May 4, 2021
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into udpatejaxb
4c8f270 
* upstream/main: (354 commits)
  Fix ScienceDirect fetcher (#7684)
  Refactor NoBibTexFieldCheckerTest to increase mutation coverage (#7697)
  Update Gradle from 6.8.3 to 7.0 (#7619)
  Fixes #7305: the RFC fetcher is not compatible with the draftFix for issue 7305 (#7674)
  Refactoring existing unit tests (#7693)
  cover boundary cases & add more unit tests (#7694)
  Bump classgraph from 4.8.104 to 4.8.105 (#7688)
  Bump java-diff-utils from 4.9 to 4.10 (#7692)
  Fix arXiv fetcher tests (#7686)
  Make key for ScienceDirect configurable (#7683)
  migration of timestamp (#7671)
  Fix CCSB and DOAJ (#7426)
  [Bot] Update CSL styles (#7680)
  MS Office XML: Export month name (#7677)
  linkfix (#7678)
  readd fix (#7675)
  Fix threading cleanup in performSearch (#7672)
  add missing changelog
  delete bug fix (#7580)
  Add more unit tests to three gui classes  (#7636)
  ...

# Conflicts:
#	build.gradle
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobiasdiez tobiasdiez tobiasdiez approved these changes

@DominikVoigt DominikVoigt Awaiting requested review from DominikVoigt

Assignees
No one assigned
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
No milestone
2 participants
@Siedlerchr @tobiasdiez