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

Jump to bottom

Fix paste of BibTeX data #9985

Merged
merged 3 commits into from
Jun 7, 2023
Merged

Fix paste of BibTeX data #9985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2f50a9c
Improve code quality
koppor Jun 7, 2023
c089e65
Fix handling of paste of text
koppor Jun 7, 2023
29654ec
Fix checkstyle
koppor Jun 7, 2023
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
9 changes: 3 additions & 6 deletions src/main/java/org/jabref/gui/ClipBoardManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import java.awt.datatransfer.UnsupportedFlavorException;
import java.io.IOException;
import java.util.List;
import java.util.Optional;

import javafx.application.Platform;
import javafx.scene.control.TextInputControl;
Expand Down Expand Up @@ -87,10 +86,6 @@ public static String getContents() {
return result;
}

public Optional<String> getBibTeXEntriesFromClipboard() {
return Optional.ofNullable(clipboard.getContent(DragAndDropDataFormats.ENTRIES)).map(String.class::cast);
}

/**
* Get the String residing on the primary clipboard (if it exists).
*
Expand Down Expand Up @@ -150,7 +145,9 @@ public void setContent(List<BibEntry> entries) throws IOException {
final ClipboardContent content = new ClipboardContent();
BibEntryWriter writer = new BibEntryWriter(new FieldWriter(preferencesService.getFieldPreferences()), Globals.entryTypesManager);
String serializedEntries = writer.serializeAll(entries, BibDatabaseMode.BIBTEX);
content.put(DragAndDropDataFormats.ENTRIES, serializedEntries);
Copy link
Member

Choose a reason for hiding this comment

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

Why do you remove setting them for the DragAndDropDataFormat here? I think this was also neceessary for drag and drop with groups

Copy link
Member Author

Choose a reason for hiding this comment

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

Drag drop is handled differently.

I checked all usages of the variable carefully.

Example code:

if (!dragboard.hasContent(DragAndDropDataFormats.GROUP)) {

Copy link
Member

Choose a reason for hiding this comment

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

but that is for drag and drop between groups. Not entries

Copy link
Member Author

Choose a reason for hiding this comment

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

I still assume that dragstrop is handled by dragophandler and not by the copy functionality. Dragging is iMho natively supported by JavaFX and we do not need to emulate it via copy and paste. This is how I understood the existing code. I checked drag and drop, too

// BibEntry is not Java serializable. Thus, we need to do the serialization manually
// At reading of the clipboard in JabRef, we parse the plain string in all cases, so we don't need to flag we put BibEntries here
// Furthermore, storing a string also enables other applications to work with the data
content.putString(serializedEntries);
clipboard.setContent(content);
setPrimaryClipboardContent(content);
Expand Down
8 changes: 4 additions & 4 deletions src/main/java/org/jabref/gui/JabRefFrame.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,9 +253,9 @@ private void initDragAndDrop() {

Dragboard dragboard = tabDragEvent.getDragboard();

if (DragAndDropHelper.hasBibFiles(tabDragEvent.getDragboard())) {
if (DragAndDropHelper.hasBibFiles(dragboard)) {
tabbedPane.getTabs().remove(dndIndicator);
List<Path> bibFiles = DragAndDropHelper.getBibFiles(tabDragEvent.getDragboard());
List<Path> bibFiles = DragAndDropHelper.getBibFiles(dragboard);
OpenDatabaseAction openDatabaseAction = this.getOpenDatabaseAction();
openDatabaseAction.openFiles(bibFiles);
tabDragEvent.setDropCompleted(true);
Expand All @@ -265,8 +265,8 @@ private void initDragAndDrop() {
if (libraryTab.getId().equals(destinationTabNode.getId()) &&
!tabbedPane.getSelectionModel().getSelectedItem().equals(libraryTab)) {
LibraryTab destinationLibraryTab = (LibraryTab) libraryTab;
if (DragAndDropHelper.hasGroups(tabDragEvent.getDragboard())) {
List<String> groupPathToSources = DragAndDropHelper.getGroups(tabDragEvent.getDragboard());
if (DragAndDropHelper.hasGroups(dragboard)) {
List<String> groupPathToSources = DragAndDropHelper.getGroups(dragboard);

copyRootNode(destinationLibraryTab);

Expand Down
30 changes: 16 additions & 14 deletions src/main/java/org/jabref/gui/edit/EditAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,45 +46,47 @@ public String toString() {
public void execute() {
stateManager.getFocusOwner().ifPresent(focusOwner -> {
LOGGER.debug("focusOwner: {}; Action: {}", focusOwner, action.getText());

if (focusOwner instanceof TextInputControl) {
if (focusOwner instanceof TextInputControl textInput) {
// Focus is on text field -> copy/paste/cut selected text
TextInputControl textInput = (TextInputControl) focusOwner;
// DELETE_ENTRY in text field should do forward delete
switch (action) {
case SELECT_ALL -> textInput.selectAll();
case COPY -> textInput.copy();
case UNDO -> textInput.undo();
case REDO -> textInput.redo();
case CUT -> textInput.cut();
case PASTE -> textInput.paste();
case DELETE -> textInput.clear();
case SELECT_ALL -> textInput.selectAll();
case DELETE_ENTRY -> textInput.deleteNextChar();
default -> throw new IllegalStateException("Only cut/copy/paste supported in TextInputControl but got " + action);
case UNDO -> textInput.undo();
case REDO -> textInput.redo();
default -> {
String message = "Only cut/copy/paste supported in TextInputControl but got " + action;
LOGGER.error(message);
throw new IllegalStateException(message);
}
}
} else if ((focusOwner instanceof CodeArea) || (focusOwner instanceof WebView)) {
LOGGER.debug("Ignoring request in CodeArea or WebView");
return;
} else {
LOGGER.debug("Else: {}", focusOwner.getClass().getSimpleName());
// Not sure what is selected -> copy/paste/cut selected entries except for Preview and CodeArea

// ToDo: Should be handled by BibDatabaseContext instead of LibraryTab
switch (action) {
case COPY -> frame.getCurrentLibraryTab().copy();
case CUT -> frame.getCurrentLibraryTab().cut();
case PASTE -> frame.getCurrentLibraryTab().paste();
case DELETE_ENTRY -> frame.getCurrentLibraryTab().delete(false);
case REDO -> {
if (frame.getUndoManager().canRedo()) {
frame.getUndoManager().redo();
}
}
case UNDO -> {
if (frame.getUndoManager().canUndo()) {
frame.getUndoManager().undo();
}
}
default -> LOGGER.debug("Only cut/copy/paste/delete supported but got: {} and focus owner {}", action, focusOwner);
case REDO -> {
if (frame.getUndoManager().canRedo()) {
frame.getUndoManager().redo();
}
}
default -> LOGGER.debug("Only cut/copy/paste/deleteEntry supported but got: {} and focus owner {}", action, focusOwner);
}
}
});
Expand Down
41 changes: 22 additions & 19 deletions src/main/java/org/jabref/gui/maintable/MainTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import java.io.File;
import java.io.IOException;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
Expand Down Expand Up @@ -267,6 +266,11 @@ public void cut() {
}

private void setupKeyBindings(KeyBindingRepository keyBindings) {
EditAction pasteAction = new EditAction(StandardActions.PASTE, libraryTab.frame(), stateManager);
EditAction copyAction = new EditAction(StandardActions.COPY, libraryTab.frame(), stateManager);
EditAction cutAction = new EditAction(StandardActions.CUT, libraryTab.frame(), stateManager);
EditAction deleteAction = new EditAction(StandardActions.DELETE_ENTRY, libraryTab.frame(), stateManager);

this.addEventFilter(KeyEvent.KEY_PRESSED, event -> {
if (event.getCode() == KeyCode.ENTER) {
getSelectedEntries().stream()
Expand All @@ -288,19 +292,19 @@ private void setupKeyBindings(KeyBindingRepository keyBindings) {
event.consume();
break;
case PASTE:
new EditAction(StandardActions.PASTE, libraryTab.frame(), stateManager).execute();
pasteAction.execute();
event.consume();
break;
case COPY:
new EditAction(StandardActions.COPY, libraryTab.frame(), stateManager).execute();
copyAction.execute();
event.consume();
break;
case CUT:
new EditAction(StandardActions.CUT, libraryTab.frame(), stateManager).execute();
cutAction.execute();
event.consume();
break;
case DELETE_ENTRY:
new EditAction(StandardActions.DELETE_ENTRY, libraryTab.frame(), stateManager).execute();
deleteAction.execute();
event.consume();
break;
default:
Expand All @@ -324,23 +328,22 @@ private void clearAndSelectLast() {

public void paste() {
List<BibEntry> entriesToAdd;
entriesToAdd = this.clipBoardManager.getBibTeXEntriesFromClipboard()
.map(importHandler::handleBibTeXData)
.orElseGet(this::handleNonBibTeXStringData);

String content = ClipBoardManager.getContents();
entriesToAdd = importHandler.handleBibTeXData(content);
if (entriesToAdd.isEmpty()) {
entriesToAdd = handleNonBibTeXStringData(content);
}
if (entriesToAdd.isEmpty()) {
return;
}
for (BibEntry entry : entriesToAdd) {
importHandler.importEntryWithDuplicateCheck(database, entry);
}
if (!entriesToAdd.isEmpty()) {
this.requestFocus();
}
}

private List<BibEntry> handleNonBibTeXStringData() {
String data = ClipBoardManager.getContents();
List<BibEntry> entries = new ArrayList<>();
private List<BibEntry> handleNonBibTeXStringData(String data) {
try {
entries = this.importHandler.handleStringData(data);
return this.importHandler.handleStringData(data);
} catch (FetcherException exception) {
if (exception instanceof FetcherClientException) {
dialogService.showInformationDialogAndWait(Localization.lang("Look up identifier"), Localization.lang("No data was found for the identifier"));
Expand All @@ -349,8 +352,8 @@ private List<BibEntry> handleNonBibTeXStringData() {
} else {
dialogService.showErrorDialogAndWait(exception);
}
return List.of();
Copy link
Member

Choose a reason for hiding this comment

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

List.of is unmodiefable. Be careful

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked the calling code and also tried out locally.

In general, the list of entries in the clipboard should not be modified. -- If someone pastes twice and continues working, this should not be modified. I thought, this behavior is also "expected" for immediate code handling pasted data.

Soemtimes, I lean to follow https://github.com/HugoMatilla/Effective-JAVA-Summary#15-minimize-mutability, but I think, its very hard to implement (and causes much more resources).

}
return entries;
}

public void dropEntry(List<BibEntry> entriesToAdd) {
Expand Down Expand Up @@ -393,8 +396,8 @@ private void handleOnDragDetected(TableRow<BibEntryTableViewModel> row, BibEntry

List<BibEntry> entries = getSelectionModel().getSelectedItems().stream().map(BibEntryTableViewModel::getEntry).collect(Collectors.toList());

// The following is necesary to initiate the drag and drop in javafx, although we don't need the contents
// It doesn't work without
// The following is necessary to initiate the drag and drop in JavaFX,
// although we don't need the contents, it does not work without
// Drag'n'drop to other tabs use COPY TransferMode, drop to group sidepane use MOVE
ClipboardContent content = new ClipboardContent();
Dragboard dragboard = startDragAndDrop(TransferMode.COPY_OR_MOVE);
Expand Down