Skip to content

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

Merged
Siedlerchr merged 6 commits into
JabRef:masterfrom
systemoperator:fix-texgroup-autoupdate
Feb 5, 2021
Merged

Fixes the auto-update problem with TexGroups on Ubuntu Linux and makes the detection of file modifications more reliable#7412
Siedlerchr merged 6 commits into
JabRef:masterfrom
systemoperator:fix-texgroup-autoupdate

Conversation

@systemoperator

@systemoperator systemoperator commented Feb 2, 2021

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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

systemoperator commented Feb 2, 2021

Copy link
Copy Markdown
Contributor Author

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

systemoperator commented Feb 2, 2021

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@tobiasdiez tobiasdiez left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@k3KAW8Pnf7mkmdSMPHz27

Copy link
Copy Markdown
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
Copy Markdown
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

calixtus commented Feb 4, 2021

Copy link
Copy Markdown
Member

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/

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 left a comment

Copy link
Copy Markdown
Member

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