Skip to content

Fix #10559: Improve Image Handling and Align with Expected Behavior #12819

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public static FieldEditorFX getForField(final Field field,
} else if (field == InternalField.KEY_FIELD) {
return new CitationKeyEditor(field, suggestionProvider, fieldCheckers, databaseContext, undoAction, redoAction);
} else if (fieldProperties.contains(FieldProperty.MARKDOWN)) {
return new MarkdownEditor(field, suggestionProvider, fieldCheckers, preferences, undoManager, undoAction, redoAction);
return new MarkdownEditor(field, suggestionProvider, fieldCheckers, preferences,databaseContext, undoManager,undoAction, redoAction);
} else {
// There was no specific editor found

Expand Down
102 changes: 101 additions & 1 deletion src/main/java/org/jabref/gui/fieldeditors/MarkdownEditor.java
Original file line number Diff line number Diff line change
@@ -1,24 +1,39 @@
package org.jabref.gui.fieldeditors;

import javax.swing.undo.UndoManager;
import java.io.File;
import java.io.IOException;
import java.net.URL;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardCopyOption;

import javafx.scene.control.TextInputControl;
import javafx.scene.input.DragEvent;
import javafx.scene.input.Dragboard;
import javafx.scene.input.TransferMode;

import org.jabref.gui.ClipBoardManager;
import org.jabref.gui.autocompleter.SuggestionProvider;
import org.jabref.gui.preferences.GuiPreferences;
import org.jabref.gui.undo.RedoAction;
import org.jabref.gui.undo.UndoAction;
import org.jabref.logic.integrity.FieldCheckers;
import org.jabref.model.database.BibDatabaseContext;
import org.jabref.model.entry.field.Field;

import com.vladsch.flexmark.html2md.converter.FlexmarkHtmlConverter;

public class MarkdownEditor extends SimpleEditor {
private final FlexmarkHtmlConverter flexmarkHtmlConverter = FlexmarkHtmlConverter.builder().build();
private final BibDatabaseContext databaseContext;
private final GuiPreferences preferences;

public MarkdownEditor(Field field, SuggestionProvider<?> suggestionProvider, FieldCheckers fieldCheckers, GuiPreferences preferences, UndoManager undoManager, UndoAction undoAction, RedoAction redoAction) {
public MarkdownEditor(Field field, SuggestionProvider<?> suggestionProvider, FieldCheckers fieldCheckers, GuiPreferences preferences, BibDatabaseContext databaseContext, UndoManager undoManager, UndoAction undoAction, RedoAction redoAction) {
super(field, suggestionProvider, fieldCheckers, preferences, true, undoManager, undoAction, redoAction);
this.databaseContext = databaseContext;
this.preferences = preferences;
setupDragAndDrop();
}

@Override
Expand All @@ -37,6 +52,91 @@ public void paste() {
};
}

private void setupDragAndDrop() {
getTextInput().setOnDragOver(event -> {
if (event.getDragboard().hasFiles() || event.getDragboard().hasUrl()) {
event.acceptTransferModes(TransferMode.COPY);
}
event.consume();
});

getTextInput().setOnDragDropped(this::handleImageDrop);
}

private void handleImageDrop(DragEvent event) {
Dragboard dragboard = event.getDragboard();
if (dragboard.hasFiles()) {
for (File file : dragboard.getFiles()) {
if (isImageFile(file)) {
File copiedFile = copyFileToImagesFolder(file);
if (copiedFile != null) {
insertMarkdownImage(copiedFile);
}
}
}
event.setDropCompleted(true);
} else if (dragboard.hasUrl()) {
String url = dragboard.getUrl();
File downloadedFile = downloadImageFromUrl(url);
if (downloadedFile != null) {
insertMarkdownImage(downloadedFile);
}
event.setDropCompleted(true);
} else {
event.setDropCompleted(false);
}
event.consume();
}

private boolean isImageFile(File file) {
Copy link
Member

Choose a reason for hiding this comment

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

String fileName = file.getName().toLowerCase();
return fileName.endsWith(".png") || fileName.endsWith(".jpg") || fileName.endsWith(".jpeg") || fileName.endsWith(".gif");
}

private File copyFileToImagesFolder(File file) {
Path imageFolder = databaseContext.getFirstExistingFileDir(preferences.getFilePreferences()).orElse(Path.of("images"));
if (!Files.exists(imageFolder)) {
try {
Files.createDirectories(imageFolder);
} catch (IOException e) {
e.printStackTrace();
Copy link

Choose a reason for hiding this comment

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

Using printStackTrace() for error handling is not appropriate for production code. Should use proper logging mechanism to handle exceptions and provide meaningful error messages.

return null;
Copy link

Choose a reason for hiding this comment

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

Methods returning null violate modern Java practices. Should use Optional to handle absence of value instead of returning null.

}
}

Path destinationFile = imageFolder.resolve(file.getName());
try {
Files.copy(file.toPath(), destinationFile, StandardCopyOption.REPLACE_EXISTING);
return destinationFile.toFile();
} catch (IOException e) {
e.printStackTrace();
return null;
}
}

private File downloadImageFromUrl(String urlString) {
Copy link
Member

Choose a reason for hiding this comment

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

If your code may return nulls, please use Java Optional

try {
URL url = new URL(urlString);
String fileName = url.getPath().substring(url.getPath().lastIndexOf('/') + 1);
Copy link
Member

Choose a reason for hiding this comment

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

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 \

Path imageFolder = databaseContext.getFirstExistingFileDir(preferences.getFilePreferences()).orElse(Path.of("images"));
Copy link
Member

Choose a reason for hiding this comment

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

Introduce a constant for "images" literal

if (!Files.exists(imageFolder)) {
Files.createDirectories(imageFolder);
}
Path destinationFile = imageFolder.resolve(fileName);
Files.copy(url.openStream(), destinationFile, StandardCopyOption.REPLACE_EXISTING);
return destinationFile.toFile();
} catch (IOException e) {
e.printStackTrace();
return null;
}
}

private void insertMarkdownImage(File imageFile) {
Path relativePath = databaseContext.getDatabasePath().map(dbPath -> dbPath.getParent().relativize(imageFile.toPath())).orElse(imageFile.toPath());
String markdownImage = String.format("\n![Image](%s)\n", relativePath.toString().replace("\\", "/"));
Copy link
Member

Choose a reason for hiding this comment

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

It would be also good if that image template would also be a constant

getTextInput().replaceSelection(markdownImage);
}

public void setEditable(boolean isEditable) {
getTextInput().setEditable(isEditable);
}
Expand Down