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

Fix #3292: annotations are now automatically refreshed #3325

Merged
merged 9 commits into from
Oct 21, 2017

Conversation

tobiasdiez
Copy link
Member

This PR aims to fix #3292 by removing the refresh button completely and monitoring the pdf files automatically for changes. In the progress of the implementation a few refactorings were made:

  • Extract the file monitoring functionality from FileUpdateMonitor to a new class and convert the timestamp-based method to the new nio watcher interface.
  • Extract the conflict handling code by external bib file changes from the base panel to a new class DatabaseChangeMonitor, where also some of the old code from FileUpdateMonitor now finds a new home. This class is still a bit ugly but in my opinion better than before. I hope that the conflict handling still works as expected. I tested it but might have missed some special cases/configurations.

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
  • If you changed the localization: Did you run gradle localizationUpdate?

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 20, 2017
@lenhard
Copy link
Member

lenhard commented Oct 20, 2017

The changes are a bit hard to review, but I have tested it locally and the annotations are refreshed as desired.

I have another point, though, one that might entail some work. Since you are already touching this code, do you see a change of refactoring the complete collab package, so that it fits better to our architecture? If we just take the current dependencies in the package, then the whole thing would basically go into gui. This already would be an improvement, because the source code packages in root would already look a little more like where we want to go.

But to be honest, I do not get why file monitoring functionality should belong into the gui part. At least part of that should be extractable into a non-GUI part and should be movable into logic. If I look at ChangeScanner, then basically everything from line 164 downwards is GUI-independent code. I also don't see why Change and its subclasses should be so heavily tied to UI code. As far as I can see this is only, because the code is tied so strongly to the undo framework. Couldn't we extract that part and move the Change classes to a deeper layer?

@tobiasdiez
Copy link
Member Author

Ok, I'll merge this now and will have a look at the collab package later. I second your concerns @lenhard, much of the code their should actually reside in logic. This would also help @Siedlerchr in his endeavor towards sharelatex integration.

@tobiasdiez tobiasdiez merged commit a7bf676 into master Oct 21, 2017
@tobiasdiez tobiasdiez deleted the fixAnnotationRefresh branch October 21, 2017 11:38
Siedlerchr added a commit that referenced this pull request Oct 22, 2017
* upstream/master:
  Initializing EntryEditor Tabs on focus (#3331)
  Fix #3292: annotations are now automatically refreshed (#3325)
  Change integrity message for names depending on database mode (#3330)
  Fix location bundle with fast access (#3327)
  Small code cleanup
  Fix "path not found" exception
  Small fix in drag and drop handler of linked files (#3328)

# Conflicts:
#	src/main/java/org/jabref/gui/DefaultInjector.java
Siedlerchr added a commit that referenced this pull request Oct 25, 2017
* upstream/master:
  Initializing EntryEditor Tabs on focus (#3331)
  Fix #3292: annotations are now automatically refreshed (#3325)

# Conflicts:
#	CHANGELOG.md
Siedlerchr added a commit that referenced this pull request Oct 27, 2017
* upstream/master: (31 commits)
  Source tab entry duplication (#3360)
  Use CITE_COMMENT not only for external latex editors but also for cop… (#3351)
  Updating with new translations (#3348)
  Upgrade error-prone (#3350)
  Jabref_pt_BR partially updated (#3349)
  Used late initialization for context menus (#3340)
  Fix NPE when calling with bib file as cmd argument (#3343)
  update mockito-core from 2.10.0 -> 2.11.0 (#3338)
  Remove underscore in Localized messages (#3336)
  Localisation: French: new entries translated (#3337)
  Refactoring: Lazy init of all editor tabs (#3333)
  Initializing EntryEditor Tabs on focus (#3331)
  Fix #3292: annotations are now automatically refreshed (#3325)
  Change integrity message for names depending on database mode (#3330)
  Fix location bundle with fast access (#3327)
  Small code cleanup
  Fix "path not found" exception
  Small fix in drag and drop handler of linked files (#3328)
  Fix NPE in MainTable (#3318)
  Increase relative size of abstract field in editor (#3320)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Refresh button in file annotations tab does not refresh the contents
3 participants