Fix #10559: Improve Image Handling and Align with Expected Behavior #12819
Conversation
![trag-bot[bot]](https://avatars.githubusercontent.com/in/856808?s=60&v=4){kind=small}
| try { | ||
| Files.createDirectories(imageFolder); | ||
| } catch (IOException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
Using printStackTrace() for error handling is not appropriate for production code. Should use proper logging mechanism to handle exceptions and provide meaningful error messages.
| Files.createDirectories(imageFolder); | ||
| } catch (IOException e) { | ||
| e.printStackTrace(); | ||
| return null; |
There was a problem hiding this comment.
Methods returning null violate modern Java practices. Should use Optional to handle absence of value instead of returning null.
| event.consume(); | ||
| } | ||
|
|
||
| private boolean isImageFile(File file) { |
There was a problem hiding this comment.
Can you extract this method into FileUtils like this: https://github.com/JabRef/jabref/blob/main/src/main/java/org/jabref/logic/util/io/FileUtil.java#L494
| } | ||
| } | ||
|
|
||
| private File downloadImageFromUrl(String urlString) { |
There was a problem hiding this comment.
If your code may return nulls, please use Java Optional
| private File downloadImageFromUrl(String urlString) { | ||
| try { | ||
| URL url = new URL(urlString); | ||
| String fileName = url.getPath().substring(url.getPath().lastIndexOf('/') + 1); |
There was a problem hiding this comment.
There should be a more proper way to do this.
Some method like getParent().
Manually processing paths by characters is not the best approach, as for example on Linux path separator is /, on Windows \
| try { | ||
| URL url = new URL(urlString); | ||
| String fileName = url.getPath().substring(url.getPath().lastIndexOf('/') + 1); | ||
| Path imageFolder = databaseContext.getFirstExistingFileDir(preferences.getFilePreferences()).orElse(Path.of("images")); |
There was a problem hiding this comment.
Introduce a constant for "images" literal
|
|
||
| private void insertMarkdownImage(File imageFile) { | ||
| Path relativePath = databaseContext.getDatabasePath().map(dbPath -> dbPath.getParent().relativize(imageFile.toPath())).orElse(imageFile.toPath()); | ||
| String markdownImage = String.format("\n\n", relativePath.toString().replace("\\", "/")); |
There was a problem hiding this comment.
It would be also good if that image template would also be a constant
|
@eslamnumber sorry for the many "JabRef Machine" comments, but one comment is missing - and I am trying to fix https://github.com/koppor/ghprcomment/ using this PR as example. Also refs jbangdev/jbang#1972 somehow. |
|
@trag-bot didn't find any issues in the code! ✅✨ |
|
JUnit tests are failing. You can see which checks are failing by locating the box "Some checks were not successful" on the pull request page. To see the test output, locate "Tests / Unit tests (pull_request)" and click on it. You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide. |
|
You have removed the "Mandatory Checks" section from your pull request description. Please adhere to our pull request template. |
|
Note that your PR will not be reviewed/accepted until you have gone through the mandatory checks in the description and marked each of them them exactly in the format of |
Pull Request Title:
Fix #10559: Improve Image Handling in Comment Fields
Closes #10559
Description of Changes:
This pull request fixes the issue where images dragged into the comment field are not properly linked or saved. It ensures that images are correctly referenced and persist even after reopening the application.
Changes Implemented:
Steps to Reproduce (Before Fix):
Expected Behavior:
dev-ai-wide-10.png) should appear in the editor.dev-ai-wide-10.pngshould be stored in the same directory as the.bibfile.Actual Behavior (Before Fix):
TODOs:
Mandatory Checks:
CHANGELOG.mddescribed in a way that is understandable for the average user (if change is visible to the user).Additional Information:
Deep link to the problematic image:
Reference Feature in VS Code:
Next Steps:
Closes #10559