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

[WIP] Refactored around the FileAnnotationCache. #2557

Merged
merged 9 commits into from
Feb 21, 2017
Merged

[WIP] Refactored around the FileAnnotationCache. #2557

merged 9 commits into from
Feb 21, 2017

Conversation

LinusDietz
Copy link
Member

The Cache now works as intended, i.e., fully automatically instead of manual maintenance.

  • Tests created for changes
  • Manually tested changed features in running JabRef

@@ -1102,7 +1107,7 @@ public void stateChanged(ChangeEvent event) {


class DeleteAction extends AbstractAction {
public DeleteAction() {
DeleteAction() {
Copy link
Member

Choose a reason for hiding this comment

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

I think, checkstyle or codacy or some other toal moans because of package private. Nevertheless, I think, it is OK to reduce visibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems like codacy does not like default visibility at all


if(!tab.isInitialized) {

try {
Copy link
Member

Choose a reason for hiding this comment

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

indent? - Think, indent is wrong here.

* Adds pdf comments from all attached pdf files belonging to the entry selected in the main table and
* shows those from the first file in the comments tab
* Adds pdf annotations from all attached pdf files belonging to the entry selected in the main table and
* shows those from the first file in the file annotations tab
* @throws IOException
Copy link
Member

Choose a reason for hiding this comment

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

I would remove that, @throws javadoc, because it does not add any more information.

public List<ParsedFileField> getFilteredFileList() {
return FileField.parse(this.entry.getField(FieldName.FILE).get()).stream()
.filter(parsedFileField -> parsedFileField.getLink().toLowerCase().endsWith(".pdf"))
.filter(parsedFileField -> !parsedFileField.getLink().contains("www.")).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.

I'm not sure whether this line is really valid. Did you find a test case for that? I assume, a user put an URL in the file field. This should not be checked here, but at other places (integrity check, ...)

Copy link
Member Author

Choose a reason for hiding this comment

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

actually this is a one-to-one copy of what has been there before. I don't have the domain-specific insight if it makes sense.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Besides minor comments: looks good to me. Thank you for the effort!

@LinusDietz LinusDietz changed the title Refactored quite much around the FileAnnotationCache. [WIP Refactored quite much around the FileAnnotationCache. Feb 16, 2017
@LinusDietz LinusDietz changed the title [WIP Refactored quite much around the FileAnnotationCache. [WIP] Refactored quite much around the FileAnnotationCache. Feb 16, 2017
@LinusDietz LinusDietz changed the title [WIP] Refactored quite much around the FileAnnotationCache. Refactored around the FileAnnotationCache. Feb 16, 2017
@LinusDietz
Copy link
Member Author

Hey, I have tried to fix the issues from your review. Check my comments on the particular parts above.

@koppor koppor changed the title Refactored around the FileAnnotationCache. [WIP] Refactored around the FileAnnotationCache. Feb 19, 2017
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. Thank you for your contribution!

@tobiasdiez tobiasdiez merged commit 41d1551 into JabRef:master Feb 21, 2017
@LinusDietz LinusDietz deleted the refactor-fileAnnotations-caching branch February 21, 2017 17:48
Siedlerchr added a commit that referenced this pull request Feb 23, 2017
* upstream/master:
  Revert "Update gradle from 3.3 to 3.4"
  Localization tests (#2582)
  Update IEEEJournalList (#2579)
  Keyword - Special field synchronization (#2583)
  Update gradle from 3.3 to 3.4
  Check similarity of entry when using DOI retrieval with ArXiV (#2575)
  Add logic for new Sciencedirect pages (#2576)
  [WIP] Refactored around the FileAnnotationCache. (#2557)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants