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.

Sign up for GitHub

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 for issue #7719: The "Aux file" on "Edit group" doesn't support relative sub-directories path #7728

Closed
wants to merge 8 commits into from
Closed

Fix for issue #7719: The "Aux file" on "Edit group" doesn't support relative sub-directories path #7728

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a779428
fix for issue #7719, user should input absolute path
devinluo27 May 10, 2021
5f11f2d
fix for issue #7719, v1
devinluo27 May 10, 2021
57f1861
fix for issue #7719, v3
devinluo27 May 20, 2021
89c95fa
Update src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java
koppor May 20, 2021
77f647b
Merge branch 'main' into fix_for_7719
koppor May 20, 2021
887e31b
Merge branch 'fix_for_7719' of https://github.com/Aloofwisdom/jabref …
devinluo27 May 21, 2021
aae330f
fixes for 7719, reject if no LatexGroupPath is set
devinluo27 May 21, 2021
e7c8131
fixes for 7719, delete a comment
devinluo27 May 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,10 +335,12 @@ public AbstractGroup resultConverter(ButtonType button) {
FieldFactory.parseField(autoGroupPersonsFieldProperty.getValue().trim()));
}
} else if (typeTexProperty.getValue()) {
// issue 7719: add workingDir to filepath if it is relative
Path inputPath = preferencesService.getWorkingDir().resolve(Path.of(texGroupFilePathProperty.getValue().trim()));
resultingGroup = TexGroup.create(
groupName,
groupHierarchySelectedProperty.getValue(),
Path.of(texGroupFilePathProperty.getValue().trim()),
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for looking at the issue!

Perhaps the changes should be added with

jabref/src/main/java/org/jabref/model/groups/TexGroup.java

Lines 134 to 143 in 807f85e

private Path expandPath(Path path) {
List<Path> fileDirectories = getFileDirectoriesAsPaths();
return FileHelper.find(path.toString(), fileDirectories).orElse(path);
}
private List<Path> getFileDirectoriesAsPaths() {
return metaData.getLatexFileDirectory(user)
.map(List::of)
.orElse(Collections.emptyList());
}

instead. Wouldn't they break paths relative to the "latex directory" otherwise? (refs #4792)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, thanks for your suggestion. Do you mean that it is better to add the preferencesService.getWorkingDir() to file path inexpandPath(Path path) method instead of in jabref/src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java. Could you explain it a little further?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my modification, this.filePath is always absolute. I thinks it is ok with FileHelper.find(path.toString(), fileDirectories).orElse(path); .

Copy link
Member

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 May 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Quite frankly I am not really sure what is going on with getWorkingDir(), but the "working dir" is changed if you open a different library which is strange behavior.

I think you are right about it not breaking the "latex directory". There is a bug in the texGroupFilePathValidator that makes it not work as I'd expect (that is unrelated to your changes).

Do you mean that it is better to add the preferencesService.getWorkingDir() to file path inexpandPath(Path path) method instead of in jabref/src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java. Could you explain it a little further?

I think so. I'll try to think a bit more about it, but I would prefer if someone in the future will be able to use the TexGroup.expandPath method (or a similar method) to fix the bug in texGroupFilePathValidator. That the path to a valid absolute or relative .aux-file is created in the same method (DRY).

Copy link
Member

Choose a reason for hiding this comment

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

What are your thoughts about using the currentDatabase.getDatabasePath()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I print "working dir" out and it always turns out to be the home dir for current user(Mac OS).

I have tried some input and find that currentDatabase.getDatabasePath() is empty. I am not quite sure if metaData.getLatexFileDirectory(user) and currentDatabase.getDatabasePath() are the same. I think explicitly setting currentDatabase.getDatabasePath() to working dir doesn't sound good as well.

There is also another way to fix it. Remove the automatically added "working dir" in texGroupFilePathValidator and always let users input a absolute path or use the browser button. This can make it simple, since probably getWorkingDir()'s behaviour is wired.

Copy link
Member

Choose a reason for hiding this comment

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

I am not quite sure if metaData.getLatexFileDirectory(user) and currentDatabase.getDatabasePath() are the same.

  • getLatexFileDirectory can be manually set by the user and is intended to allow relative paths to a user-specified directory (the menu Library -> Library properties) for .aux files
  • getDatabasePath should give you the path to the .bib file. It should be used by the "Reveal in file explorer" function. (From the JavaDoc) Get the path where this database was last saved to or loaded from, if any.

I think explicitly setting currentDatabase.getDatabasePath() to working dir doesn't sound good as well.

Agreed.

There is also another way to fix it. Remove the automatically added "working dir" in texGroupFilePathValidator and always let users input a absolute path or use the browser button. This can make it simple, since probably getWorkingDir()'s behaviour is wired.

I think you are right in that the "working dir" should be removed from the texGroupFilePathValidator. Perhaps its behavior can be made more aligned with

jabref/src/main/java/org/jabref/gui/groups/GroupDialogViewModel.java

Lines 422 to 431 in 84c8849

public void texGroupBrowse() {
FileDialogConfiguration fileDialogConfiguration = new FileDialogConfiguration.Builder()
.addExtensionFilter(StandardFileType.AUX)
.withDefaultExtension(StandardFileType.AUX)
.withInitialDirectory(preferencesService.getWorkingDir()).build();
dialogService.showFileOpenDialog(fileDialogConfiguration)
.ifPresent(file -> texGroupFilePathProperty.setValue(
FileUtil.relativize(file.toAbsolutePath(), getFileDirectoriesAsPaths()).toString()
));
}

It seems like there is a wish to be able to use relative paths, which were the intent of the latex file directory. What are your thoughts about attempting to get it working again and updating the documentation?
I don't know if there is a "default" directory it would make sense to look for .aux files in, but perhaps it isn't too much to ask of a user to specify that directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have set the base for relative filePath as getLatexFileDirectory() and I have try some test cases manually and it can work. Point it out if there is any problem.

inputPath,
new DefaultAuxParser(new BibDatabase()),
Globals.getFileUpdateMonitor(),
currentDatabase.getMetaData());
Expand Down
1 change: 1 addition & 0 deletions src/main/java/org/jabref/gui/util/DefaultFileUpdateMonitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ private void notifyAboutChange(Path path) {
public void addListenerForFile(Path file, FileUpdateListener listener) throws IOException {
if (isActive()) {
// We can't watch files directly, so monitor their parent directory for updates
// toAbsolutePath() will add the path to Jabref as home directory to file, if file is not a absolute path
Path directory = file.toAbsolutePath().getParent();
directory.register(watcher, StandardWatchEventKinds.ENTRY_CREATE, StandardWatchEventKinds.ENTRY_MODIFY);
listeners.put(file, listener);
Expand Down