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.

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

Implement drag and drop reordering in General files list #3194

Merged
merged 8 commits into from
Sep 5, 2017

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Sep 3, 2017

Add drag and drop related methods to ViewModelFactory
Make LInkedFile serializeable
Fixes #3165 (comment)

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
  • If you changed the localization: Did you run gradle localizationUpdate?

Add drag and drop related methods to ViewModelFactory
Make LInkedFile serializeable
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 3, 2017
@koppor koppor changed the title Implemente drag and drop reordering in General files list Implement drag and drop reordering in General files list Sep 4, 2017
Copy link
Member

@lenhard lenhard left a comment

Choose a reason for hiding this comment

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

The code looks fine and the reordering works.

However, JabRef does not remember that a file has been reordered. So when I start JabRef anew the files are back to their original order. Is that intented?

@Siedlerchr
Copy link
Member Author

@lenhard Hm, that's odd. I will try to look at it. But I think it could be possible that there is no database changed event fired

* upstream/master:
  Update wiremock from 2.7.1 -> 2.8.0
  Update log4j from 2.8.2 -> 2.9.0
  Added the name of some new authors
  Fix typo
  Improvement in Japanese translation (#3193)
* upstream/master:
  Remove untranslated Strings from the greek menu translation
  Remove untranslated Strings from the greek translation
  Update wiremock from 2.7.1 -> 2.8.0
  Update log4j from 2.8.2 -> 2.9.0
  Added the name of some new authors
  Fix typo
  local db and shared db sync fix #2284 with changelog
  local db and shared db sync fix #2284
  Improvement in Japanese translation (#3193)
@Siedlerchr
Copy link
Member Author

Siedlerchr commented Sep 5, 2017

@lenhard The new file ordering is now also changed in the bibtex data

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Some minor code comments.

@@ -14,7 +15,8 @@
/**
* Represents the link to an external file (e.g. associated PDF file).
*/
public class LinkedFile {
//Serialiable is required for drag and drop
Copy link
Member

Choose a reason for hiding this comment

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

Why not a real class comment? I would always use the class comments.

@@ -113,6 +144,21 @@ protected void updateItem(T item, boolean empty) {
if (toContextMenu != null) {
setContextMenu(toContextMenu.call(viewModel));
}
if (toOnDragDetected != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Oh, wow, this code looks odd. Is JavaFX really like that?

@koppor
Copy link
Member

koppor commented Sep 5, 2017

I have following file entry in my bib entry:

file      = {:Cla2008 -The Birth of Model Checking.pdf:PDF},

It is rendered like this:

grafik

Does this come from this PR?

@lenhard
Copy link
Member

lenhard commented Sep 5, 2017

I've tested it briefly with an entry with two file links and there reordering was possible and also stored between sessions.

@@ -76,6 +82,31 @@
return this;
}

public ViewModelListCellFactory<T> setOnDragDetected(BiConsumer<T, ? super MouseEvent> toOnDragDetected) {
this.toOnDragDetected = toOnDragDetected;
Copy link
Member Author

Choose a reason for hiding this comment

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

@koppor The null check refers to this factory function where it gets assigned

@Siedlerchr
Copy link
Member Author

@koppor Your display thing is related to the autolinked file. One of your files got autolinked.

@@ -49,12 +56,67 @@ public LinkedFilesEditor(String fieldName, DialogService dialogService, BibDatab
.withTooltip(LinkedFileViewModel::getDescription)
.withGraphic(LinkedFilesEditor::createFileDisplay)
.withContextMenu(this::createContextMenuForFile)
.withOnMouseClickedEvent(this::handleItemMouseClick);
.withOnMouseClickedEvent(this::handleItemMouseClick)
Copy link
Member Author

Choose a reason for hiding this comment

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

@koppor The BiConsumer stuff allows to do these fancy things

… dragdropfiles

* 'dragdropfiles' of https://github.com/JabRef/jabref:
  remove empty line
  Make content binding bidirectional so changes are also recognized in bibdb
  Remove untranslated Strings from the greek menu translation
  Remove untranslated Strings from the greek translation
  local db and shared db sync fix #2284 with changelog
  local db and shared db sync fix #2284
* upstream/master:
  Update xmlunit from 2.4.0 -> 2.5.0
  Fix tooltip for hotkey
@koppor
Copy link
Member

koppor commented Sep 5, 2017 via email

@Siedlerchr Siedlerchr merged commit 647b382 into master Sep 5, 2017
@Siedlerchr Siedlerchr deleted the dragdropfiles branch September 5, 2017 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants