-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Fix issue #11189 part 00 refactor citation relation tab logic #11845
base: main
Are you sure you want to change the base?
Fix issue #11189 part 00 refactor citation relation tab logic #11845
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
The tool reviewdog already placed comments on GitHub to indicate the places. See the tab "Files" in you PR.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.
You can check review dog's comments at the tab "Files changed" of your pull request.
9fe8522
to
cbe9e96
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
The tool reviewdog already placed comments on GitHub to indicate the places. See the tab "Files" in you PR.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.
You can check review dog's comments at the tab "Files changed" of your pull request.
cbe9e96
to
8231340
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code currently does not meet JabRef's code guidelines.
We use OpenRewrite to ensure "modern" Java coding practices.
The issues found can be automatically fixed.
Please execute the gradle task rewriteRun
, check the results, commit, and push.
You can check the detailed error output by navigating to your pull request, selecting the tab "Checks", section "Tests" (on the left), subsection "OpenRewrite".
8231340
to
33967c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code currently does not meet JabRef's code guidelines.
We use OpenRewrite to ensure "modern" Java coding practices.
The issues found can be automatically fixed.
Please execute the gradle task rewriteRun
, check the results, commit, and push.
You can check the detailed error output by navigating to your pull request, selecting the tab "Checks", section "Tests" (on the left), subsection "OpenRewrite".
33967c2
to
592d4d7
Compare
d94f4d3
to
3155242
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add code explanations to the PR
src/main/java/org/jabref/gui/entryeditor/citationrelationtab/BibEntryRelationsCache.java
Show resolved
Hide resolved
src/main/java/org/jabref/gui/entryeditor/citationrelationtab/BibEntryRelationsRepository.java
Show resolved
Hide resolved
src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java
Show resolved
Hide resolved
src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java
Show resolved
Hide resolved
src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java
Show resolved
Hide resolved
src/main/java/org/jabref/logic/citation/repository/LRUBibEntryRelationsCache.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/citation/repository/LRUBibEntryRelationsCache.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/citation/repository/LRUBibEntryRelationsRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/citation/service/SearchCitationsRelationsService.java
Outdated
Show resolved
Hide resolved
Please no force push if not needed. All commits will be squashed when merged |
* Move repository, cache, and fetcher to logic package * Move citations model to model/citations/semanticscholar package
* Introduce service layer * Rename LRU cache implementation * Add tests helpers for repository
* Move logic from repository to service * Refactor repositories * Update tab configuration
3155242
to
18db75e
Compare
Sorry, just re-based main branch locally. |
…lation-tab-logic # Conflicts: # src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general looks good. Some minor comments.
Sorry for delay. Please go ahead with everything.
src/main/java/org/jabref/logic/citation/service/SearchCitationsRelationsService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java
Show resolved
Hide resolved
src/test/java/org/jabref/logic/citation/service/SearchCitationsRelationsServiceTest.java
Outdated
Show resolved
Hide resolved
Small other comments - IntelliJ proposed to extract a method
in the tests - maybe you can also include that. |
@alexandre-cremieux Please pull before you continue working on it - I merged |
src/main/java/org/jabref/logic/citation/repository/LRUBibEntryRelationsRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/citation/service/SearchCitationsRelationsService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/citation/service/SearchCitationsRelationsService.java
Outdated
Show resolved
Hide resolved
Thanks for the review and the merge. I will resume the work on this branch and apply the changes. |
@alexandre-cremieux Sorry for the merge conflicts - can you handle them? I was always happy with IntelliJ's "resolve merge conflicts" dialog. Hope, it works in this case, too. |
Hello @koppor . Seems that we have new conflicts to resolve to be able to merge main. But I will do that when the feature will be fully developed. Was quite busy last month, I resumed the work this week. PR comments were addressed. |
… exhausted * Remove the isForceUpdate boolean * User is still able to trigger the fetch if an error occurs
Code update:
|
* Instantiate service in JabRefGui * Inject service in EntryEditor
Hello @koppor. Now, only preference for the MV store TTL time is missing. I thought about adding it under
Default value would be 7. It would be added under a new category: |
Hello @koppor Could you please help me a bit here ? The single missing part is the configuration that you asked for in December. Could you please explain me how to implement this: new tab in Preferences or adding it into existing one (like Web search), naming of the parameter for the user, what about the localization, etc ? Otherwise, could you please let me know if I misunderstood something ? Thank in advance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.
In case of issues with the import order, double check that you activated Auto Import.
You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.
In case of issues with the import order, double check that you activated Auto Import.
You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.
In case of issues with the import order, double check that you activated Auto Import.
You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports.
32aa56a
to
4956095
Compare
4956095
to
ea36f1a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answered comments.
progress, | ||
fetchedList, | ||
observableList | ||
)) | ||
.onFailure(exception -> { | ||
LOGGER.error("Error while fetching citing Articles", exception); | ||
hideNodes(abortButton, progress, importButton); | ||
listView.setPlaceholder(new Label(Localization.lang("Error while fetching citing entries: %0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@InAnYan This was here before I touched this code. I am leaving this for now in this PR as this out of the scope.
src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java
Show resolved
Hide resolved
LOGGER.error("Error while fetching citations for entry {}", cited.getTitle(), e); | ||
} | ||
} | ||
return this.relationsRepository.readCitations(cited); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends. Normally:
- If the store was empty and nothing was fetch in any case (empty fetch, or error) then yes => empty list
- If the store was not empty and nothing was fetch after a successful fetch => the store will be erased and the returned collection will be empty
- If the store was not empty and an error occurs while fetching => will return the content of the store
MVStoreBibEntryRelationDAO(Path path, String mapName) { | ||
this.mapName = mapName; | ||
this.insertionTimeStampMapName = mapName + "-insertion-timestamp"; | ||
this.storeConfiguration = new MVStore.Builder().autoCommitDisabled().fileName(path.toAbsolutePath().toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank, me also usually. Hopefully, it is a proof that I am not yet a machine 🤖 😄 🌹
|
||
MVStoreBibEntryRelationDAO(Path path, String mapName) { | ||
this.mapName = mapName; | ||
this.insertionTimeStampMapName = mapName + "-insertion-timestamp"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
.orElse(true); | ||
} | ||
|
||
private static class BibEntrySerializer extends BasicDataType<BibEntry> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @InAnYan,
Please do not take this comment in account. You were right, ad-hoc serialization does not seem to be necessary. It should possible to apply existing logic provided by BibDatabaseWriter' and 'BibTexParser
classes.
This is the complicated part:
The BibEntry is used here in fact not to represent BibEntry but something else, a citation/reference fetched from SemanticsScholar's API. The output of the API is not a BibEntry, but a Citation or a Reference and the fetcher (JabRef) convert it to BibEntry. A BibEntry is larger in memory than a citation/reference. So to avoid using more space than needed the serializer is adapted to this use case.
Also, until the Citation/Reference is inserted into the bib, the user does not have access to as much information than for a BibEntry accessible from a bib, or even cannot edit it.
I had this discussion with @koppor. I proposed to change the type of the Citation/Reference to something else and only transform it into a BibEntry when necessary (search or insertion into the bib). I thought it would simplify the code but it seems that this is not really needed.
Why did I propose those changes: (all the discussion is in the PR)
* Managing citations/references in this context is another domain than managing a BibEntry
* Interpreting DDD here (sorry, sometimes I am dogmatic), as it is not the same domain then it seems normal to not use the same model (as we are not representing the same thing)
* Avoiding mixing the concerns between citation/relation could (maybe should) reduce the ramp-up for the developer on this use case in the future
* Managing duplicates in the citations/relations stores would be easier from my point of view because it would only rely on the DOI for equality tests (I am saying this but maybe it is already the case with BibEntry ?!?) - with another model you can decide to switch to a more suitable ID (as DDD encourage it).
I let you see with @koppor. I am still willing to do those changes: create a specific model for citations/references and use this one for fetching, storage, and view in citations tabs. But convert it to BibEntry only when needed (search/insertion from/into the bib).
However, if you have a plan that implies to store it as a full BibEntry then please let me know and please indicate me where to find the proper serializer (so I do not re-invent the wheel).
private static String toString(BibEntry entry) { | ||
return String.join( | ||
FIELD_SEPARATOR, | ||
entry.getTitle().orElse("null"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Than "null" would be serialized (and deserialized).
private static BibEntry fromString(String serializedString) { | ||
var fields = serializedString.split(FIELD_SEPARATOR); | ||
BibEntry entry = new BibEntry(); | ||
extractFieldValue(fields[0]).ifPresent(title -> entry.setField(StandardField.TITLE, title)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because there are the only ones available for a citations or reference.
progress, | ||
fetchedList, | ||
observableList | ||
)) | ||
.onFailure(exception -> { | ||
LOGGER.error("Error while fetching citing Articles", exception); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@InAnYan This was here before I touched this code. I am leaving this for now in this PR as this out of the scope.
Still missing:
|
TODO here:
After this, the PR should be ready for last review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.
In case of issues with the import order, double check that you activated Auto Import.
You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports.
e56497e
to
16cfd2c
Compare
…erialization error (JabRef#11189)
Ready for review. Normally everything should be there. |
High level description
This contributions aims to provide a local temporary local storage for citations relations using MVStore. Please see #11189 for more information.
Implementation details
No new dependency added.
Caching strategy
The implemented caching logic is following a 2 levels side-caching strategy with:
The interaction between the two levels of cache is backed by a responsibility chain pattern. This helps to:
Search high level logic
Serialization
MVStoreDAO uses the canonical representation to serialize a BibEntry and the
BibtexParser
to deserialize it.Configuration
Citations Relations
tab using the JabRef IoC providercitations
folder under JabRef's app directorySetting translation
The the local storage TTL setting message has been traduced in:
Refactoring
This contributions simplifies the citations/references fetching and caching logic by introducing two layers:
This should help to make this feature more extendable without modifying orchestration logic following open/close principle.
Screen shots
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if applicable)