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

Fixes the auto-update problem with TexGroups on Ubuntu Linux and makes the detection of file modifications more reliable #7412

Merged
merged 6 commits into from
Feb 5, 2021

Conversation

systemoperator
Copy link
Contributor

@systemoperator systemoperator commented Feb 2, 2021

Fixes #7173 (comment) in #7173.

On Ubuntu Linux the auto-update functionality does not work with TexGroups, when the associated .aux file gets updated externally.

This is a quick fix for this issue, which may still be improved. Only one of both files needs to be changed in order to fix it.

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

@systemoperator systemoperator changed the title Fixes the auto-update problem with TexGroup's on Ubuntu Linux Fixes the auto-update problem with TexGroups on Ubuntu Linux Feb 2, 2021
@tobiasdiez
Copy link
Member

Thanks for your PR. Can you please explain what the problem is/was and why your fix works? It looks like you only have a relative file in your tex group and want to resolve this path relative to the latex folder, is this correct?

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Sponsor Member

If the problem is that it is a relative path, perhaps this would be another location where the code could be added?

Path.of(texGroupFilePathProperty.getValue().trim()),

@systemoperator
Copy link
Contributor Author

systemoperator commented Feb 2, 2021

Basically, the path for the aux-file is created for the wrong base directory.

Path path = Path.of(tok.nextToken());

creates a Path object path="main.aux". When converting it to its absolute path (path.toAbsolutePath()), it uses the working directory of JabRef and not for the directory of the opened library, which results in a non-existing file-path.

Later, the "file-monitor" detects the modified file main.aux with its correct absolute path, but when it tries to inform the registered listeners at

listeners.get(path).forEach(FileUpdateListener::fileUpdated);

no proper entry for this file is found in the keys of the map, thus no listener gets informed.

If TexGroup.java would be used, then there would still exist some wrong paths in the code. Thus, I would drop the changes in TexGroup.java and choose GroupsParser.java instead.

@k3KAW8Pnf7mkmdSMPHz27 You are right. In this case, when the library is simply opened from within JabRef, your mentioned code line is not executed, but if one would convert an existing group to a tex-group, this should be a problem as well.

@systemoperator
Copy link
Contributor Author

systemoperator commented Feb 2, 2021

Changing

fileMonitor.addListenerForFile(filePath, group);

to

fileMonitor.addListenerForFile(group.filePath, group);

and ignoring the other fixes, should fix both cases (I. simply opening the library and II. changing a group to a TexGroup)

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Sponsor Member

I am not up-to-date on how changing the group type works in practice, but it seems like a good solution to the relativized path issue.

@tobiasdiez
Copy link
Member

Thanks for the explanation! #7412 (comment) looks like the right approach to me. In addition, I would suggest to add a method getFilePathResolved() to TexGroup that simply returns the filepath object.

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Sponsor Member

Basically, I agree with @tobiasdiez, same regarding #7173 (comment).

@systemoperator systemoperator changed the title Fixes the auto-update problem with TexGroups on Ubuntu Linux Fixes the auto-update problem with TexGroups on Ubuntu Linux and makes the detection of file modifications more reliable Feb 2, 2021
@systemoperator
Copy link
Contributor Author

I have tested this fix for loading a library, for changing a group to a TexGroup and with different editors each. It works great for Ubuntu.

@k3KAW8Pnf7mkmdSMPHz27 Could you check for safety reasons, that your library on macOS is still getting updated with this version?

tobiasdiez
tobiasdiez previously approved these changes Feb 3, 2021
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!

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Sponsor Member

@k3KAW8Pnf7mkmdSMPHz27 Could you check for safety reasons, that your library on macOS is still getting updated with this version?

The library is still getting updated 👍 😃

@Siedlerchr
Copy link
Member

Would be nice if someone can test this under Windows as well, because Windows and Linux behave differently regarding file watching events/file system operations

@calixtus
Copy link
Member

calixtus commented Feb 4, 2021

Just a general thing: Please try to shorten your PR titles and commit messages and keep them meaningful, so after months you still understand, what you did. See https://www.freecodecamp.org/news/writing-good-commit-messages-a-practical-guide/

Copy link
Sponsor Member

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 left a comment

Choose a reason for hiding this comment

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

I don't have a full Windows 10 setup, but the relative path works when manually editing the .aux file. There is an issue in that TexGroup.create doesn't throw an exception if the file doesn't exist but it is not related to this PR.

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 5, 2021
@Siedlerchr Siedlerchr merged commit 93ad499 into JabRef:master Feb 5, 2021
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.

5 participants