Convert absolute paths to relative paths in Recent Files #14464
Conversation
Removed commented-out section for relative/absolute helpers.
github-actions
bot
added
the
status: changes-required
github-actions
bot
removed
the
status: changes-required
|
Thank you for your initial code. Please add missing tests. |
calixtus
added
the
status: changes-required
github-actions
bot
added
status: changes-required
github-actions
bot
removed
the
status: changes-required
koppor
left a comment
koppor
left a comment
There was a problem hiding this comment.
Code comments need to be added on the absolute/relative thing. There is no code comment about the where relative and absolute paths are stored etc.
I miss handling of #3590 (comment) - and your PR comment doesn't say anything about this.
Thus this PR reads like a quick fix than a well-through-through thing? I also miss screenshots.
| if (absolutePath == null) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
When can this happen? Maybe annotate paramter as @NonNull?
| try { | ||
| return workingDir.relativize(absolutePath); | ||
| } catch (Exception e) { | ||
| return absolutePath; // fallback |
There was a problem hiding this comment.
Do a LOGGER.error, because this should be fixed maybe?
| private Path toAbsolute(Path storedPath) { | ||
| if (storedPath == null) { | ||
| return null; | ||
| } | ||
| Path workingDir = Path.of("").toAbsolutePath(); | ||
| if (!storedPath.isAbsolute()) { |
| /** | ||
| * Adds the file to the top of the list. If it already is in the list, it is merely moved to the top. | ||
| * Adds the file to the top of the list. If it already is in the list, it is | ||
| * merely moved to the top. | ||
| */ |
There was a problem hiding this comment.
Not sure why the reformatting here?
| public void newFile(Path file) { | ||
| removeItem(file); | ||
| this.addFirst(file); | ||
| this.addFirst(toRelative(file.toAbsolutePath())); |
| private Path toRelative(Path absolutePath) { | ||
| Path workingDir = Path.of("").toAbsolutePath(); | ||
| try { | ||
| return workingDir.relativize(absolutePath); | ||
| } catch (Exception e) { | ||
| return absolutePath; // fallback | ||
| } | ||
| } | ||
|
|
||
| private Path toAbsolute(Path storedPath) { | ||
| Path workingDir = Path.of("").toAbsolutePath(); | ||
| if (!storedPath.isAbsolute()) { | ||
| return workingDir.resolve(storedPath).normalize(); | ||
| } | ||
| return storedPath; | ||
| } |
There was a problem hiding this comment.
This is duplicated code.
Strong indication that used at too many places.
Maybe you can use org.jabref.logic.util.io.FileUtil#relativize(java.nio.file.Path, java.util.List<java.nio.file.Path>) ?
github-actions
bot
added
the
status: changes-required
| return new FileHistory(new ArrayList<>(list)); | ||
| } | ||
|
|
||
| private Path toRelative(Path absolutePath) { |
There was a problem hiding this comment.
This definetitely needs a test
4 participants
Closes #3590
This PR updates the Recent Files feature so that stored file paths are converted from absolute paths to project-relative paths. This fixes issues where Recent Files breaks when the project folder is moved or shared across machines. This improves portability and consistency for users working in multiple environments.
Steps to test
.bibfile located inside a folder (e.g., a project directory)..bibfile using the relative path.Mandatory checks
CHANGELOG.mdin a way understandable for the average user (if visible)