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

Enable users to simultaneously search all SearchBasedFetchers #6504

Merged

Conversation

DominikVoigt
Copy link
Contributor

This PR enables users can send queries to all E-libraries simultaneously through the WebSearchPane.
Ref #369

It adds a new SearchBasedFetcher called CompositeSearchBasedFetcher that delegates the query to all of the fetchers it is composed of, and merges the fetched results.
In WebFetchers.getSearchBasedFetchers a CompositeSearchBasedFetcher is added that contains all SearchBasedFetchers.

If this PR gets approved an additional documentation entry has to be added.

  • 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.

…aries together in the Web Search Pane.

Signed-off-by: DominikVoigt <dominik.ingo.voigt@gmail.com>
Signed-off-by: DominikVoigt <dominik.ingo.voigt@gmail.com>
Signed-off-by: DominikVoigt <dominik.ingo.voigt@gmail.com>
Add linebreak for checkstyle.
Modify Testcase.

Signed-off-by: DominikVoigt <dominik.ingo.voigt@gmail.com>
Signed-off-by: DominikVoigt <dominik.ingo.voigt@gmail.com>
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 21, 2020
…sue.

Signed-off-by: DominikVoigt <dominik.ingo.voigt@gmail.com>
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

It would be nice if you could some more fetchers to your test and also test what happens when one fetcher returns an error

CHANGELOG.md Outdated Show resolved Hide resolved
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.

Thanks for your contribution. I like the idea!
For now only a initial impression after scrolling through the code: Did you test it in a running JabRef? I'm somewhat afraid that this quickly leads to Jabref being blocked by some databases since it issues quite a lot of requests. Moreover, if I understand the implementation correctly, then every fetcher is always asked which should lead to quite a huge list of returned entries (probably with perhaps many duplicates).

Co-authored-by: Christoph <cschwentker@gmail.com>
@DominikVoigt
Copy link
Contributor Author

DominikVoigt commented May 22, 2020

It would be nice if you could some more fetchers to your test and also test what happens when one fetcher returns an error

I replaced the tests with parameterized tests.

@DominikVoigt
Copy link
Contributor Author

Thanks for your contribution. I like the idea!

I'm happy about that!

For now only a initial impression after scrolling through the code: Did you test it in a running JabRef?

I tested it with a few queries and it worked so far ^^.

I'm somewhat afraid that this quickly leads to Jabref being blocked by some databases since it issues quite a lot of requests. Moreover, if I understand the implementation correctly, then every fetcher is always asked which should lead to quite a huge list of returned entries (probably with perhaps many duplicates).

One option might be to offer the user a way to configure which fetchers to include?
In my current implementation, I do not offer any post-processing yet. I think doing post-processing in the fetcher is not a great design idea, what do you think? :) Where should I implement the post-processing of the fetched entries?

Signed-off-by: DominikVoigt <dominik.ingo.voigt@gmail.com>
Add test case: empty set of fetchers

Signed-off-by: DominikVoigt <dominik.ingo.voigt@gmail.com>
Signed-off-by: DominikVoigt <dominik.ingo.voigt@gmail.com>
… in the Webfetcher.

Signed-off-by: DominikVoigt <dominik.ingo.voigt@gmail.com>
list.add(new DOAJFetcher(importFormatPreferences));
list.add(new IEEE(importFormatPreferences));
/* Disabled due to issue regarding Comparison: Title fields of the entries that otherwise are equivalent differ
* due to different JAXBElements.
Copy link
Member

Choose a reason for hiding this comment

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

Can you provide me the concreete example? Maybe, we need to fix the MedlineFetcher?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK:
When using the same instance of MedlineFetcher to fetch results directly or as part of the CompositeFetcher, certain BibEntries have differing titles due to something related to JAXBElements.
When searching for "Indistinguishable Photons from Deterministically" the direct use of the MedLine Fetcher returns a BibEntry with the title:

title = {Indistinguishable Photons from Deterministically Integrated Single Quantum Dots in Heterogeneous GaAs/Si, javax.xml.bind.JAXBElement@31b82e0f, N, javax.xml.bind.JAXBElement@27a09971, Quantum Photonic Circuits.}

The CompositeFetcher, on the other hand, returns a BibEntry with the title:

title = {Indistinguishable Photons from Deterministically Integrated Single Quantum Dots in Heterogeneous GaAs/Si, javax.xml.bind.JAXBElement@3289079a, N, javax.xml.bind.JAXBElement@32fa809f, Quantum Photonic Circuits.}

Both would be the same only differing by the JAXBElement Instance.

// list.add(new MedlineFetcher());

// Create different sized sets of fetchers to use in the composite fetcher.
for (int i = 1; i < list.size(); i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we do some different sub sets? Not always include the first one, but something like: Always +3 and then use the subsets matched by the respective bits?

6543210
0110001 --> list entry 5,4,0 are used

(Not sure what the others think of it)

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.

Code looks already very good to me! I've only two minor remarks.

@tobiasdiez tobiasdiez added the status: changes required Pull requests that are not yet complete label May 25, 2020
Add null test for CompositeSearchBasedFetcher.
Adapt most change requests

Signed-off-by: DominikVoigt <dominik.ingo.voigt@gmail.com>
@Siedlerchr
Copy link
Member

Siedlerchr commented May 26, 2020

JAXB Refs #6350 and #6302

Signed-off-by: DominikVoigt <dominik.ingo.voigt@gmail.com>
… getting created otherwise.

Signed-off-by: DominikVoigt <dominik.ingo.voigt@gmail.com>
@tobiasdiez tobiasdiez requested review from Siedlerchr and koppor May 27, 2020 20:52
@DominikVoigt DominikVoigt force-pushed the feature/add-option-to-fetch-from-all-sources branch from b278178 to 2c70b3f Compare May 27, 2020 21:59
DominikVoigt and others added 2 commits May 28, 2020 00:13
Remove Annotation from IEEETest.

Signed-off-by: DominikVoigt <dominik.ingo.voigt@gmail.com>
@koppor koppor merged commit 47846bf into JabRef:master May 27, 2020
@DominikVoigt DominikVoigt deleted the feature/add-option-to-fetch-from-all-sources branch July 27, 2020 20:52
koppor pushed a commit that referenced this pull request May 1, 2023
a985762505 Update environmental-and-engineering-geoscience.csl (#6512)
5118058ea0 Update norsk-henvisningsstandard-for-rettsvitenskapelige-tekster.csl (#6515)
e9830d3f5e Create polish-archives-of-internal-medicine.csl (#6399)
05ef543bd6 Update ieee.csl (#6511)
b6e6292e4b Update universite-de-bordeaux-ecole-doctorale-de-droit.csl (#6510)
af38aba0e9 Create la-nouvelle-revue-du-travail.csl (#6400)
4b23d7a03e Create north-pacific-anadromous-fish-commission-bulletin.csl (#6436)
77ea82a242 Create journal-of-dental-traumatology.csl (#6403)
af4578d1a7 Make magnetic-resonance-in-medicine.csl AMA dependent (#6433)
5467a4f901 Create medizinische-universitaet-innsbruck-vancouver.csl (#6484)
8a3c0a2b9b Update united-states-international-trade-commissio (#6487)
789267a9cb Update cardiff-university-harvard.csl (#6482)
252a5b5c08 Locators in palaeontology journal styles (#6496)
3d2bff0794 Update ecosistemas.csl (#6503)
199baca2c6 Bump nokogiri from 1.13.10 to 1.14.3 (#6504)
feffe61ae4 Update universite-du-quebec-a-montreal-etudes-litteraires-et-semiologie.csl (#6505)

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: a985762505418bd63c26a54c59b48e3ed7426953
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 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.

4 participants