Add drag and drop in EntryEditor #9965

wy8881 · 2023-05-31T11:01:29Z

Mandatory checks

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

wy8881 · 2023-05-31T11:04:44Z

Hi,
this is for fixes JabRef#569
I add a function so that a paper can be added into a group like:
1.

2.

3. Can add this paper into a group when it already in another group

However, this drag or drop will happen when dragging the group to the EntryEditor panel instead of dragging it to the exact group field inside General Tab. I try to add setOnDragDropped() function to a tab, but it seems I can't do it. It'll be helpful if someone can give me some advice.

k3KAW8Pnf7mkmdSMPHz27 · 2023-05-31T22:37:20Z

I try to add setOnDragDropped() function to a tab, but it seems I can't do it.

First of all, I don't know if there is a correct way to do this X)
I would start looking at what happens if you use the components of a tab, e.g., tab.getContent() will give you a node.

Why a tab and not a field? If you only set it on the tab, I would be surprised if it works if you drag it to a field.

In order to deal with drag and drop to a field, I'd start looking at

jabref/src/main/java/org/jabref/gui/entryeditor/EntryEditor.java

Lines 262 to 265 in 424ff1c

    
           // General fields from preferences 
        
           for (Map.Entry<String, Set<Field>> tab : entryEditorPreferences.getEntryEditorTabList().entrySet()) { 
        
               entryEditorTabs.add(new UserDefinedFieldsTab(tab.getKey(), tab.getValue(), databaseContext, libraryTab.getSuggestionProviders(), undoManager, dialogService, preferencesService, stateManager, themeManager, libraryTab.getIndexingTaskManager(), taskExecutor, journalAbbreviationRepository)); 
        
           }

And if you follow that trail to its superclass FieldsEditorTab and

jabref/src/main/java/org/jabref/gui/entryeditor/FieldsEditorTab.java

Lines 112 to 113 in 424ff1c

    
           FieldEditorFX fieldEditor = FieldEditors.getForField( 
        
                   field,

inside getForField, you'll find some fields that have special properties.I might be wrong, but I suspect you'll have to create a new field that supports dropped events.

src/main/java/org/jabref/gui/entryeditor/EntryEditor.java

calixtus · 2023-06-01T16:28:43Z

Please move the drag logic to a new GroupEditor implementing SimpleEditor, so we can keep the FieldsEditorTab clean of those special cases. Single responsibilty of the FieldsEditorTab is to draw the FieldEditors.

wy8881 · 2023-06-01T16:59:17Z

Hi, I didn't find this GroupEditor. Do you mean to create a new class implementing SimpleEditor?

calixtus · 2023-06-01T17:05:05Z

- Yes.

wy8881 · 2023-06-01T17:09:07Z

I see. I wonder where I should put this file. It is in gui-entryeditor or gui-fieldeditors?

wy8881 · 2023-06-01T18:17:22Z

Hi,
I have created a GroupEditor in gui-fieldeditors. I set the drag events in bindToEntry() function since I need the entry object. I am not sure if it is appropriate. If anyone has advice please let me know. Thanks

calixtus · 2023-06-01T19:03:33Z

Yes I think that should work in bindToEntry. Although this method is always called if any entry in the main table is selected, there are probably no memory leaks produced, as far as I can see. Looks good so far. Let's see what a second maintainer thinks.
Thanks!

calixtus

Undo unnecessary whitespace changes
Huh, tried to remove directly within github ui, but only commented instead of committet... 🤣

calixtus · 2023-06-01T19:09:43Z

Just fixed some unnecessary whitespace changes. Something was not right in my ui.

Siedlerchr · 2023-06-01T19:23:57Z

I fear the listeners are registered everytime. I would try to register the listeners in the constructor and add a field for the entry
e.g. and in bindToEntry entry you assign this then:

this.entry = entry;

then you have a chance to access the entry in the listeners

k3KAW8Pnf7mkmdSMPHz27 · 2023-06-01T19:39:58Z

then you have a chance to access the entry in the listeners

LinkedFilesEditor.java follows this approach, except that it doesn't use the SimpleEditor. It should be possible to adapt the approach to use SimpleEditor as base.

Siedlerchr · 2023-06-01T19:49:45Z

then you have a chance to access the entry in the listeners

LinkedFilesEditor.java follows this approach, except that it doesn't use the SimpleEditor. It should be possible to adapt the approach to use SimpleEditor as base.

That is even more elegant! Yeah that should be working fine.

Change the type of bibEntry from ObservableOptionalValue<BibEntry> to Optional<BibEntry>

wy8881 · 2023-06-02T09:34:24Z

Hi, I have removed the listener to the constructor, but I still have few questions:

In this code I use if to decide whether the entry has been bound instead of using .map() because that boolean variable cannot be changed inside the lambda. I'm not sure if using .map() and atomicboolean is a better idea.

jabref/src/main/java/org/jabref/gui/fieldeditors/GroupEditor.java

Lines 38 to 44 in 18b204e

    
           if (bibEntry.isPresent()) { 
        
               String changedGroup = bibEntry.map(entry -> entry.getField(StandardField.GROUPS) 
        
                                                                .map(setGroup -> setGroup + (preferences.getBibEntryPreferences().getKeywordSeparator()) + (group.get(0))) 
        
                                                                .orElse(group.get(0))) 
        
                                             .orElse(null); 
        
               bibEntry.map(entry -> entry.setField(StandardField.GROUPS, changedGroup)); 
        
               success = true;

After checking the LinkedFilesEditor.java, I find out that it uses ObservableOptionalValue bibEntry

jabref/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditor.java

Line 70 in 18b204e

private ObservableOptionalValue<BibEntry> bibEntry = EasyBind.wrapNullable(new SimpleObjectProperty<>());

and bind the entry in

jabref/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditor.java

Line 238 in 18b204e

bibEntry = EasyBind.wrapNullable(new SimpleObjectProperty<>(entry));

I write a similar code in GroupEditor, but it fails noDiscardingAChangeLeadsToNewerBackupBeReported() test. The strange thing is that this can be passed in Intellij. Then I changed the type of bibEntry to Optional and it can pass the test. I don't know what is wrong with the another one.

k3KAW8Pnf7mkmdSMPHz27 · 2023-06-02T13:47:51Z

In this code I use if to decide whether the entry has been bound instead of using .map() because that boolean variable cannot be changed inside the lambda. I'm not sure if using .map() and atomicboolean is a better idea.

~~I am sorry, but I don't understand this explanation. Could you write a short snippet showcasing when it cannot be changed? I don't know what circumstances you cannot either use ! or Predicate.not.~~

... I don't know what is wrong with the another one.

This is really weird, and shouldn't happen. Manually re-running the test in GitHub appears to have "fixed" it.

Looking at the code again, perhaps Optional<BibEntry> is the better option. In this setting, there is no need to use JavaFX bindings at all.

k3KAW8Pnf7mkmdSMPHz27 · 2023-06-02T13:56:06Z

In this code I use if to decide whether the entry has been bound instead of using .map() because that boolean variable cannot be changed inside the lambda. I'm not sure if using .map() and atomicboolean is a better idea.

You'll have difficulties dealing with local variables inside of a lambda. If you are willing to try you can rewrite almost the whole dragDropped function as a chain ending it with .ifPresentOrElse using event.setDropCompleted(true/false).

It would be an interesting exercise, but it might make it unreadable.

The outer if would most likely have to be changed to an if-else

wy8881 · 2023-06-02T15:23:24Z

It would be an interesting exercise, but it might make it unreadable.

I tried to rewrite the setOnDragDropped and it seems ugly, so I think it's better to stick with 'if' version

The outer if would most likely have to be changed to an if-else

I can change that but I don't understand the point. I think if the condition fails and the boolean success will stay false. I'm not sure why we still need the else clause.

k3KAW8Pnf7mkmdSMPHz27 · 2023-06-02T21:54:52Z

I tried to rewrite the setOnDragDropped and it seems ugly, so I think it's better to stick with 'if' version

Agreed, thank you for humoring me, and sorry 😅

I can change that but I don't understand the point. I think if the condition fails and the boolean success will stay false. I'm not sure why we still need the else clause.

Logically nothing should change, true. But from my (opinionated 😁) point of view, keeping each if as short/small as possible often creates less code complexity, even if the "logic condition" complexity is the same in this case.

I just re-arranged your latest code and arrived at this, and just to be clear, I think the first version you made with an if is easier to read, and I'd suggest you revert to that version.

Here I can mentally deal with the if (event.getDragboard().hasContent(DragAndDropDataFormats.GROUP)) before I start checking on what the bibEntry code does. Imo it makes it easier to see if there is a missing event.setDropCompleted(false) (line 43 😉)

          if (event.getDragboard().hasContent(DragAndDropDataFormats.GROUP)) {
                final String group = (List<String>) event.getDragboard().getContent(DragAndDropDataFormats.GROUP).get(0);
                bibEntry.ifPresentOrElse(entry -> {
                    final String changedGroup = entry.getField(StandardField.GROUPS)
                                                     .map(setGroup -> setGroup + (preferences.getBibEntryPreferences().getKeywordSeparator()) + (group)).orElse(group);
                    entry.setField(StandardField.GROUPS, changedGroup);
                    event.setDropCompleted(true);
                }, () -> event.setDropCompleted(false));
            } else {
                event.setDropCompleted(false);
            }
            event.consume();
        });

src/main/java/org/jabref/gui/fieldeditors/GroupEditor.java

rewriteDryRun FAILED

k3KAW8Pnf7mkmdSMPHz27

Codewise, it looks good to me! Two very minor things and it is ready to go,

Add a short note to the CHANGELOG
Address the empty string case for groups (dragging all entries, see note in code)

src/main/java/org/jabref/gui/fieldeditors/GroupEditor.java

k3KAW8Pnf7mkmdSMPHz27

LGTM! Thank you for putting your time and effort into this PR!

wy8881 · 2023-06-05T15:05:20Z

Thank you for the help

Add drag and drop in EntryEditor

1801939

Fix code style

bc4ced1

k3KAW8Pnf7mkmdSMPHz27 self-requested a review May 31, 2023 21:51

k3KAW8Pnf7mkmdSMPHz27 reviewed May 31, 2023

View reviewed changes

src/main/java/org/jabref/gui/entryeditor/EntryEditor.java Outdated Show resolved Hide resolved

wy8881 added 4 commits June 1, 2023 11:46

Add drop and drag to groups field

cbf3a96

fix code style

434271e

code style

9219c6a

still code style

6cca6aa

wy8881 marked this pull request as ready for review June 1, 2023 03:19

wy8881 added 2 commits June 2, 2023 03:15

Create a GroupEditor for drag event

dc2cae6

Fix code style

5afabf2

calixtus reviewed Jun 1, 2023

View reviewed changes

Undo unnecessary whitespace changes

6dbddb8

k3KAW8Pnf7mkmdSMPHz27 added the status: changes required Pull requests that are not yet complete label Jun 1, 2023

wy8881 added 2 commits June 2, 2023 17:28

Remove listener to constructor

4c6ba16

Change the type of bibEntry

18b204e

Change the type of bibEntry from ObservableOptionalValue<BibEntry> to Optional<BibEntry>

wy8881 added 2 commits June 3, 2023 00:25

Rewrite setOnDragDropped

282c3b0

code style

04e2d9e

k3KAW8Pnf7mkmdSMPHz27 reviewed Jun 2, 2023

View reviewed changes

src/main/java/org/jabref/gui/fieldeditors/GroupEditor.java Outdated Show resolved Hide resolved

wy8881 added 3 commits June 3, 2023 15:26

Change to if

04cb217

Code style

5ec4fa8

Code style

72ff5b2

rewriteDryRun FAILED

calixtus previously approved these changes Jun 3, 2023

View reviewed changes

k3KAW8Pnf7mkmdSMPHz27 requested changes Jun 5, 2023

View reviewed changes

src/main/java/org/jabref/gui/fieldeditors/GroupEditor.java Outdated Show resolved Hide resolved

Change CHANGELOG and check null case

fba9038

wy8881 dismissed calixtus’s stale review via fba9038 June 5, 2023 12:23

Merge branch 'main' into fix-for-569

494bc57

calixtus approved these changes Jun 5, 2023

View reviewed changes

k3KAW8Pnf7mkmdSMPHz27 approved these changes Jun 5, 2023

View reviewed changes

k3KAW8Pnf7mkmdSMPHz27 merged commit 277ec2d into JabRef:main Jun 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add drag and drop in EntryEditor #9965

Add drag and drop in EntryEditor #9965

wy8881 commented May 31, 2023 •

edited by k3KAW8Pnf7mkmdSMPHz27

Loading

wy8881 commented May 31, 2023 •

edited

Loading

k3KAW8Pnf7mkmdSMPHz27 commented May 31, 2023

calixtus commented Jun 1, 2023

wy8881 commented Jun 1, 2023

calixtus commented Jun 1, 2023 •

edited

Loading

wy8881 commented Jun 1, 2023

wy8881 commented Jun 1, 2023

calixtus commented Jun 1, 2023

calixtus left a comment •

edited

Loading

calixtus commented Jun 1, 2023

Siedlerchr commented Jun 1, 2023 •

edited

Loading

k3KAW8Pnf7mkmdSMPHz27 commented Jun 1, 2023

Siedlerchr commented Jun 1, 2023

wy8881 commented Jun 2, 2023

k3KAW8Pnf7mkmdSMPHz27 commented Jun 2, 2023 •

edited

Loading

k3KAW8Pnf7mkmdSMPHz27 commented Jun 2, 2023 •

edited

Loading

wy8881 commented Jun 2, 2023 •

edited

Loading

k3KAW8Pnf7mkmdSMPHz27 commented Jun 2, 2023 •

edited

Loading

k3KAW8Pnf7mkmdSMPHz27 left a comment

k3KAW8Pnf7mkmdSMPHz27 left a comment

wy8881 commented Jun 5, 2023

Add drag and drop in EntryEditor #9965

Add drag and drop in EntryEditor #9965

Conversation

wy8881 commented May 31, 2023 • edited by k3KAW8Pnf7mkmdSMPHz27 Loading

Mandatory checks

wy8881 commented May 31, 2023 • edited Loading

k3KAW8Pnf7mkmdSMPHz27 commented May 31, 2023

calixtus commented Jun 1, 2023

wy8881 commented Jun 1, 2023

calixtus commented Jun 1, 2023 • edited Loading

wy8881 commented Jun 1, 2023

wy8881 commented Jun 1, 2023

calixtus commented Jun 1, 2023

calixtus left a comment • edited Loading

Choose a reason for hiding this comment

calixtus commented Jun 1, 2023

Siedlerchr commented Jun 1, 2023 • edited Loading

k3KAW8Pnf7mkmdSMPHz27 commented Jun 1, 2023

Siedlerchr commented Jun 1, 2023

wy8881 commented Jun 2, 2023

k3KAW8Pnf7mkmdSMPHz27 commented Jun 2, 2023 • edited Loading

k3KAW8Pnf7mkmdSMPHz27 commented Jun 2, 2023 • edited Loading

wy8881 commented Jun 2, 2023 • edited Loading

k3KAW8Pnf7mkmdSMPHz27 commented Jun 2, 2023 • edited Loading

k3KAW8Pnf7mkmdSMPHz27 left a comment

Choose a reason for hiding this comment

k3KAW8Pnf7mkmdSMPHz27 left a comment

Choose a reason for hiding this comment

wy8881 commented Jun 5, 2023

wy8881 commented May 31, 2023 •

edited by k3KAW8Pnf7mkmdSMPHz27

Loading

wy8881 commented May 31, 2023 •

edited

Loading

calixtus commented Jun 1, 2023 •

edited

Loading

calixtus left a comment •

edited

Loading

Siedlerchr commented Jun 1, 2023 •

edited

Loading

k3KAW8Pnf7mkmdSMPHz27 commented Jun 2, 2023 •

edited

Loading

k3KAW8Pnf7mkmdSMPHz27 commented Jun 2, 2023 •

edited

Loading

wy8881 commented Jun 2, 2023 •

edited

Loading

k3KAW8Pnf7mkmdSMPHz27 commented Jun 2, 2023 •

edited

Loading