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

Make it easier to create FXML dialogs #3880

Merged
merged 6 commits into from
Mar 23, 2018
Merged

Conversation

tobiasdiez
Copy link
Member

@tobiasdiez tobiasdiez commented Mar 22, 2018

Right now it is pretty difficult to create dialogs in JavaFX using FXML. For once, you have to create a bunch of files (fxml, view, controller, view model) and there are huge problems with passing data between the caller and the controller (e.g., no complex objects, cumbersome injection setup,...).

So I took the opportunity to rewrite parts of the afterburner framework to make the workflow more fluid. With these changes, the view class is merged with the controller and it is very easy to pass data to and from the controller. In particular, the code in #3005 should become much simpler.

Moreover, I started to embed the content of a few Swing dialogs (cleanup and preferences) in a JavaFX window. This is kind of proof of concept, works pretty nicely in my opinion and is probably the only way to fix the z-hierarchy issues with the old swing dialogs.

Git is not able to properly sort-out some of the changes (the merge of the view and controller classes) and shows a pretty huge diff. If git gurus like @koppor know a way to fix this, feel free to hijack this PR.


  • Change in CHANGELOG.md described
  • Tests created for changes
  • Manually tested changed features in running JabRef
  • Screenshots added in PR description (for bigger UI changes)
  • Ensured that the git commit message is a good one
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 22, 2018
@@ -132,6 +132,7 @@ dependencies {
compile 'org.fxmisc.flowless:flowless:0.6'
compile 'org.fxmisc.richtext:richtextfx:0.8.2'
compile 'com.sibvisions.external.jvxfx:dndtabpane:0.1'
compile 'javax.inject:javax.inject:1'
Copy link
Member

Choose a reason for hiding this comment

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

Is that an official java dep?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's a popular JavaEE dependency, see https://mvnrepository.com/artifact/javax.inject/javax.inject.

@tobiasdiez
Copy link
Member Author

By the way, here is a very simple example of a dialog that accepts input and provides a result: https://github.com/JabRef/afterburner.fx/blob/master/demo-app/src/main/java/com/airhacks/afterburner/demo/dialog/SimpleDialog.java

@Siedlerchr
Copy link
Member

Cool! Overall looks good. I would suggest we release our patched afterburner also on gradle/maven.
Could you outline the steps to create /convert the old dialogs to new ones? I think updating the wiki pages would be appropriate

@stefan-kolb
Copy link
Member

stefan-kolb commented Mar 22, 2018

I'm not sure what's behind this but a general remark.
I'm not so happy with changes that introduce additional abstraction layers.

  1. Additional knowledge is needed to use them
  2. If the logic behind the library is not 100% tested we get hard to trace errors again
    Probably makes you the only one that can ever change the code 😄
    But you are the JavaFX expert so it's up to you to lead the way!

@koppor
Copy link
Member

koppor commented Mar 23, 2018

@Inject is the key here? Then, we should definitively take it. 👍 Angular also uses that and we should train people in that.

I cannot do anything regarding the huge diffs. Git does it via heuristics. More reading:h ttps://git.wiki.kernel.org/index.php/GitFaq#Why_does_Git_not_.22track.22_renames.3F

@Siedlerchr
Copy link
Member

Injection has already been used for the whole time to inject deps

@tobiasdiez
Copy link
Member Author

tobiasdiez commented Mar 23, 2018

@stefan-kolb You raise good points. In fact, one of the problems we have right now is that the afterburner.fx library has bugs that affect us but is no longer maintained. On the other hand, loading FXML files by hand using built-in classes is tedious and leads to repetitive code that would be needed for every dialog. In the end, the extracted library is very small and essentially only one class ViewLoader. Against my natural tendency, I tried to write as much documentation as possible so that it should be easy to understand and maintain it.

Since the PR only got positive feedback, I'll merge it now. The documentation in the wiki is adapted. The AboutDialog is easy enough to see which steps are required to setup a dialog (and have a look at the cleanup dialog or wiki page for dialog with return values).

@tobiasdiez tobiasdiez merged commit 3c4a3ef into maintable-beta Mar 23, 2018
@tobiasdiez tobiasdiez deleted the swingDialogs branch March 23, 2018 18:38
Siedlerchr added a commit that referenced this pull request Mar 24, 2018
…esDlg

* upstream/maintable-beta: (35 commits)
  Load all field editors using ViewLoader
  Use JabRef icons in FXML
  Move icon stuff to new package gui.icon
  Load EntryEditor using new ViewLoader
  Improve tooltip tests
  Fix import thread problem
  Don't use null as parameter in DialogService
  Make it easier to create FXML dialogs (#3880)
  update slf4j from 1.8.0-beta1 -> 1.8.0-beta2
  New translations JabRef_en.properties (Tagalog)
  New translations JabRef_en.properties (Italian)
  New translations Menu_en.properties (Italian)
  New translations JabRef_en.properties (Indonesian)
  New translations JabRef_en.properties (Greek)
  New translations Menu_en.properties (Greek)
  New translations JabRef_en.properties (Japanese)
  New translations JabRef_en.properties (German)
  New translations JabRef_en.properties (Dutch)
  New translations JabRef_en.properties (Danish)
  New translations JabRef_en.properties (Chinese Simplified)
  ...

# Conflicts:
#	CHANGELOG.md
#	src/main/java/org/jabref/gui/AbstractView.java
#	src/main/java/org/jabref/gui/desktop/JabRefDesktop.java
#	src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java
#	src/main/java/org/jabref/gui/openoffice/StyleSelectDialog.java
#	src/main/java/org/jabref/gui/protectedterms/ProtectedTermsDialog.java
Siedlerchr added a commit that referenced this pull request Mar 30, 2018
…drop

* upstream/maintable-beta: (48 commits)
  Clean unused imports
  Fix missing icons and wrong package for custom icons
  Consistent FX color scheme for JabRef (#3839)
  javafx replacement for file dialog (#3005)
  Reenable closing of entry preview by pressing Esc (#3883)
  Load all field editors using ViewLoader
  Use JabRef icons in FXML
  Move icon stuff to new package gui.icon
  Load EntryEditor using new ViewLoader
  Improve tooltip tests
  Fix import thread problem
  Don't use null as parameter in DialogService
  Make it easier to create FXML dialogs (#3880)
  update slf4j from 1.8.0-beta1 -> 1.8.0-beta2
  New translations JabRef_en.properties (Tagalog)
  New translations JabRef_en.properties (Italian)
  New translations Menu_en.properties (Italian)
  New translations JabRef_en.properties (Indonesian)
  New translations JabRef_en.properties (Greek)
  New translations Menu_en.properties (Greek)
  ...

# Conflicts:
#	src/main/java/org/jabref/gui/actions/CleanupAction.java
#	src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java
#	src/main/java/org/jabref/gui/groups/GroupTreeController.java
#	src/main/java/org/jabref/gui/maintable/MainTable.java
#	src/main/java/org/jabref/logic/cleanup/MoveFilesCleanup.java
#	src/main/java/org/jabref/logic/cleanup/RenamePdfCleanup.java
#	src/test/java/org/jabref/logic/cleanup/MoveFilesCleanupTest.java
#	src/test/java/org/jabref/logic/cleanup/RenamePdfCleanupTest.java
Siedlerchr added a commit that referenced this pull request Apr 1, 2018
…gsjavafx

* upstream/maintable-beta: (188 commits)
  Fix checkstyle
  Exchange Ignore by Disabled (#3912)
  Remove all @author comments and empty method/class comments
  Clean unused imports
  Fix missing icons and wrong package for custom icons
  Consistent FX color scheme for JabRef (#3839)
  javafx replacement for file dialog (#3005)
  Reenable closing of entry preview by pressing Esc (#3883)
  Load all field editors using ViewLoader
  Use JabRef icons in FXML
  Move icon stuff to new package gui.icon
  Load EntryEditor using new ViewLoader
  Improve tooltip tests
  Fix import thread problem
  Don't use null as parameter in DialogService
  Make it easier to create FXML dialogs (#3880)
  update slf4j from 1.8.0-beta1 -> 1.8.0-beta2
  New translations JabRef_en.properties (Tagalog)
  New translations JabRef_en.properties (Italian)
  New translations Menu_en.properties (Italian)
  ...

# Conflicts:
#	src/main/java/org/jabref/gui/JabRefFrame.java
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.

4 participants