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

Improve things arround change detection #5770

Merged
merged 5 commits into from
Dec 20, 2019
Merged

Improve things arround change detection #5770

merged 5 commits into from
Dec 20, 2019

Conversation

tobiasdiez
Copy link
Member

Another shot at fixing #4877. The changes in this PR might be already sufficient, let's see...

No time for extended PR description, see commit messages for changes. Last commit contains the actual "fix", the other ones are cosmetic changes.

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 19, 2019
throws IOException {
ParserResult result = new BibtexImporter(importFormatPreferences, fileMonitor).importDatabase(fileToOpen.toPath(),
ParserResult result = new BibtexImporter(importFormatPreferences, fileMonitor).importDatabase(fileToOpen,
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change it from path to file?

Copy link
Member

Choose a reason for hiding this comment

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

This question is asked, because java.nio.Path should be preferred over java.io.File. We are aware in the code that this causes some conversion issues, however, I think, we should keep to move forward here. (Is this article still the best google result? --> https://jaxenter.de/java-nio-file-zeitgemases-arbeiten-mit-dateien-2581)

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually converted it to a Path, the method now accepts a Path variable and not a File

public static ParserResult loadDatabase(String name, ImportFormatPreferences importFormatPreferences, FileUpdateMonitor fileMonitor) {
File file = new File(name);
LOGGER.info("Opening: " + name);
LOGGER.debug("Opening: " + name);
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't "Opening {}", name work? - I think, this is the style one does in Java, because otherwise the debug string is always calculated even if the statement is not set.

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't that micro optimization?

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.

Think, the changes need to be tried out.

"Someone" should write tests emulating external modifications on the file in a parallel thread (or are we already having those kinds of tests?)

@tobiasdiez
Copy link
Member Author

I agree writing tests would be a good idea. However that requires major refactoring beforehand as the problematic code is still tightly coupled to the UI 😣

@tobiasdiez tobiasdiez merged commit 6431099 into master Dec 20, 2019
@tobiasdiez tobiasdiez deleted the fixChangeDetect branch December 20, 2019 12:47
Siedlerchr added a commit that referenced this pull request Dec 20, 2019
# By Tobias Diez (11) and others
# Via GitHub (1) and Tobias Diez (1)
* upstream/master: (29 commits)
  Improve things arround change detection (#5770)
  Revert "Update to most recent journal abbreviation list" (#5769)
  Various fixes to the dark theme (#5764)
  Bump mockito-core from 3.2.0 to 3.2.4 (#5760)
  Bump classgraph from 4.8.58 to 4.8.59 (#5761)
  Improve dependency update rules
  Update jpackage to build 27 (#5758)
  Persistent column sortorder (#5730)
  Fix medline fetcher/importer when using installer (#5752)
  New Crowdin translations (#5751)
  Bump byte-buddy-parent from 1.10.4 to 1.10.5 (#5750)
  Fix checkstyle
  Fix filename
  Update to most recent journal abbreviation list
  Remove obsolete string
  Revert "Switch back to development"
  Switch back to development
  Next development cycle
  Release 5.0-beta (#5684)
  Fix multiple entries allowed in crossref (issue #5284) (#5724)
  ...

# Conflicts:
#	src/main/java/org/jabref/gui/collab/ChangeDisplayDialog.java
#	src/main/java/org/jabref/gui/collab/EntryChangeViewModel.java
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.

3 participants