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

Implemented about dialog in Java FX #968

Merged
merged 3 commits into from
Apr 26, 2016
Merged

Conversation

boceckts
Copy link
Contributor

This is the first dialog in JabRef being implemented in JavaFX. The Dialog is highly flexible in positioning and styling its components through a fxml and corresponding css stylesheet. If JavaFX should be a part of JabRef it is probabbly usefull to define a global stylesheet as well to become a consistent look and feel.
To include JavaFX in JabRef I had to adjust the starting and closing behaviour of Jabref.

In order to run these commits, Java 8_40 or newer is required!

  • Change in CHANGELOG.md described?
  • Changes in pull request outlined? (What, why, ...)
  • Tests created for changes?
  • Tests green?

@simonharrer
Copy link
Contributor

Travis CI uses Jdk 1.8.0_31, and because of this cannot compile this.

@boceckts
Copy link
Contributor Author

As it says in the error message the class javafx.scene.control.Alert can not be found since this has been introduced in Java 8_40 along with javafx.scene.control.Dialog. However this is a essential part in creating a javafx gui since it is basically the equivalent class to JOptionPane / JDialog classes in swing.
Is it possible to update the jdk version of the Travis CI?

@simonharrer
Copy link
Contributor

Please do investigate on this.

@Siedlerchr
Copy link
Member

@boceckts This is currently a problem with Travis,but it can be updated:
travis-ci/travis-ci#3259

This one looks the most promising:
travis-ci/travis-ci#3259 (comment)


closeButton.setText(Localization.lang("Close"));

copyButton.setText(Localization.lang("Copy_version_to_clipboard"));
Copy link
Member

Choose a reason for hiding this comment

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

Just use normal spaces instead of underscore. Then the circleci test should pass.

@koppor
Copy link
Member

koppor commented Mar 15, 2016

@boceckts As JabRef uses both CircleCI and Travis and tests for both OpenJDK and JRE, there won't be a simple solution.

@Siedlerchr
Copy link
Member

For cicle CI
Higher jdk versions than Build 25 (this was from one blog post) seem to be not available, a workaound seems to be this hack:
https://discuss.circleci.com/t/java-version/2435

Edit// This one here for installing new version of qt5 seems to be related/similar:
https://discuss.circleci.com/t/using-qt-5-with-circleci/88

@boceckts
Copy link
Contributor Author

Thank you for all your feedback, I'm goinig to further investigate on this.

try (InputStream imageStream = JabRef.class.getResourceAsStream("/images/external/JabRef-icon-48.png");
InputStream fxmlStream = JabRef.class.getResourceAsStream("/gui/help/AboutDialogLayout.fxml");) {
DialogPane dialogContentPane = (DialogPane) loader.load(fxmlStream);
javafx.scene.image.Image img = new javafx.scene.image.Image(imageStream);
Copy link
Contributor

Choose a reason for hiding this comment

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

don't use abbreviation here. img-> image

@boceckts boceckts force-pushed the AboutDialogFX branch 2 times, most recently from 7c34de8 to ae7fbe7 Compare March 16, 2016 15:02
@boceckts
Copy link
Contributor Author

I created a new AboutDialog class which handles its initialization itself as previously. This way there are less changes and imports in the JabrefFrame class.
The improvements mentioned in the previous comments are also included.

@tschechlovdev tschechlovdev added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed stupro-ready-for-internal-review labels Mar 16, 2016
@tschechlovdev tschechlovdev changed the title [WIP] Implemented about dialog in Java FX Implemented about dialog in Java FX Mar 16, 2016
@Siedlerchr
Copy link
Member

Interestingly I found out that circle-ci is already using java8 u40:

Using Java 1.8.0_40 from /usr/lib/jvm/jdk1.8.0/jre
From the log of:
./gradlew -Pdev=true -Pinstall4jDir="install4j6" clean release --stacktrace || exit 0

@boceckts
Copy link
Contributor Author

Thanks for pointing this out, I already wondered since I didn't touch the circle.yml yet the tests are passing on CircleCI.

setDialogStyle(JabRefMain.class.getResource("/gui/help/AboutDialog.css").toExternalForm());
} catch (IOException e) {
logger.debug("AboutDialog could not be completely loaded!", e);
} catch (NullPointerException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not catch NPEs.

@koppor
Copy link
Member

koppor commented Mar 20, 2016

Please ensure that this version of JabRef also runs under Linux with OpenJDK.

@boceckts
Copy link
Contributor Author

As this states https://docs.travis-ci.com/user/trusty-ci-environment/ it should be possible to switch to the trusty beta build environment where we should be able to use the open jdk 1.8.0_60. I'm going to try this first before manually testing it.

@koppor
Copy link
Member

koppor commented Apr 25, 2016

We should sit together and think about a good design. I would like to have tabs instead of these strange pull down menus. Reason: responsiveness of the UI. If I click something, I want to read the things instantly. Proposal: Fix the minor issues here, then merge and then think about a good design of the dialog.

I liked the JabRef 3.2 About dialog more - maybe we can put each heading into a tab and render the first two paragraphs above that.

grabbed_20160425-090444
grabbed_20160425-090519


websiteGithub.set("https://github.com/JabRef/jabref");

copyButtonText.set(Localization.lang("Copy_Version"));
Copy link
Member

@tobiasdiez tobiasdiez Apr 25, 2016

Choose a reason for hiding this comment

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

Please change it back to %Copy Version in the fxml file. We will later provide a proper fix for this issue.

@koppor
Copy link
Member

koppor commented Apr 25, 2016

For reference: The current state of the dialog:

grabbed_20160425-090724

@codecov-io
Copy link

codecov-io commented Apr 26, 2016

Current coverage is 27.32%

Merging #968 into javafx will decrease coverage by -0.00%

@@           javafx    #968   diff @@
=====================================
  Files         696     699     +3   
  Lines       46446   46557   +111   
  Methods         0       0          
  Branches     7703    7705     +2   
=====================================
+ Hits        12719   12721     +2   
- Misses      32628   32738   +110   
+ Partials     1099    1098     -1   
  1. 2 files (not in diff) in .../net/sf/jabref/logic were modified. more
    • Misses -1
    • Hits +1
  2. 2 files (not in diff) in ...n/java/net/sf/jabref were modified. more
    • Partials -1
    • Hits +1

Sunburst

Powered by Codecov. Last updated by eeb7cf3

The old dialog and html layout file have been removed.
The layout of the dialog panel is defined in the AboutDialog.fxml file
located in the resources with its stylesheet. The AboutDialogViewModel
class takes care of the text initialization of any components in the
layout. It also implements any methodes called from any component of the
dialog panel. The dialog is build and shown in the AboutDialogView
class.
@boceckts
Copy link
Contributor Author

I changed the code accordingly to your comments.
Note that at the current stage the dialog can not be shown as it throws an exception with the message Resource "Copy Version" not found.

@tobiasdiez
Copy link
Member

I will now merge this PR in (in the JavaFX branch) so that you can work on further dialogs.

You are right, the keyboard shortcuts dialog looks relatively good. The "grab" button is probably superfluous and "Default" should specify if it resets just one shortcut or all. Moreover, I like the design in the Vivaldi browser:
pic
See also the description https://help.vivaldi.com/article/keyboard-shortcuts/.
I especially like that the shortcuts are grouped.

@tobiasdiez tobiasdiez merged commit f954e0b into JabRef:javafx Apr 26, 2016
@boceckts boceckts deleted the AboutDialogFX branch May 6, 2016 10:25
@koppor
Copy link
Member

koppor commented Sep 7, 2016

Current state is still not that nice.

grabbed_20160907-185247

I know that removing code is hard, but could you please make the dialog appearing more than the old one and showing all information nice in one place? I think, we can show it on one page (maybe not the developers). We do not need collapsing for one liners.

@boceckts
Copy link
Contributor Author

boceckts commented Sep 7, 2016

I will simplyfy it and remove the tab view.

@boceckts
Copy link
Contributor Author

boceckts commented Sep 8, 2016

@JabRef/developers opinions to the following design?

aboutdialogfx

@Siedlerchr
Copy link
Member

Looks good! Just one minor thing, the URL for contributing seems to be not aligned at the same height as the label

@tobiasdiez
Copy link
Member

Looks way better!
Only criticism:

  • The background color of the header is...well...ugly 😄
  • Display the URLs is unnecessary, better link text
  • Remove border around the authors field (if easily possible)
  • The license is wrong ( ? @koppor ? )
  • The sub-header "Developer" and "Authors" don't stand out.

I would slightly reorganize everything, but your version is also fine
image
(without the underlined text, that's a relict from the markup software)

@Siedlerchr
Copy link
Member

@tobiasdiez and no Comic Sans MS style (as from the mockup) http://www.comicsanscriminal.com/ ;)

@boceckts
Copy link
Contributor Author

boceckts commented Sep 9, 2016

So there are two new versions similar to what @tobiasdiez suggested.
(For some reason the authors contain weird characters for me)

screen shot 2016-09-09 at 12 40 38
screen shot 2016-09-09 at 12 45 01

It really looks a bit weird to me with so much space in the top but the information to display is rather small, maybe resize the dialog a bit smaller?

@boceckts
Copy link
Contributor Author

boceckts commented Sep 9, 2016

I tried a smaller width for the dialog and personally this is my favorite, all the headings and information are left aligned and the icon has a small reflection which does not interfere with any text. Opinions are welcomed.

screen shot 2016-09-09 at 15 54 17

@Siedlerchr
Copy link
Member

The small dialog looks good. ❤️

@tobiasdiez
Copy link
Member

tobiasdiez commented Sep 9, 2016

Yes I really love the smaller dialog. Good work! Where is the PR?? I want to merge it right-away 😄
(small suggestion: maybe move the "Developers" headline a bit down to increase the separation to the previous text)

@Siedlerchr
Copy link
Member

@boceckts Could you please test with the gradle change I introduced in #1945 and see if authors now are displayed correctly`?. And please check the encoding of the Authors file (should be UTF-8 without BOM)

@boceckts boceckts mentioned this pull request Sep 10, 2016
5 tasks
@boceckts
Copy link
Contributor Author

@Siedlerchr the encoding is set to UTF-8 without BOM but unfortunately your changes have no effect for me.
@tobiasdiez There it is 😄 and I moved down the sub heading a little bit.

@Siedlerchr
Copy link
Member

Regarding the encoding problems, I just fixed it with #1954. Please try again with the changes I made (gradle changes + version info)
And check: src\main\resources\build.properties if it is correctly encoded as utf8 without bom.

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.