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

Remove customjfx #3779

Merged
merged 5 commits into from
Mar 11, 2018
Merged

Remove customjfx #3779

merged 5 commits into from
Mar 11, 2018

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Feb 26, 2018 •
edited
Loading

This removes the custom JfXPanel hack we introduced for #3028 @koppor
As now the main frame is javafx and no longer swing this hack is no longer needed and can be removed.

I did not completely remove the class, because some dialogs still require a jfxpanel with a scene

  • 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?)

@Siedlerchr
Get rid of customjfx hack
cec8d2c 
Big step towards jdk9 compatibility
No longer needed
CodeArea still does not work
@Siedlerchr Siedlerchr changed the base branch from master to maintable-beta February 26, 2018 14:03
tobiasdiez
tobiasdiez reviewed Feb 26, 2018
View reviewed changes
Copy link
Member

@tobiasdiez tobiasdiez left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The dialogs which still embed JavaFX in Swing, they don't contain any input possibilities? If they do, then users may run into the same problems with accented characters, right?

@Siedlerchr
Copy link
Member Author

Hm, good question. I will take a look at that.

@Siedlerchr
Copy link
Member Author

As far as I see the panel is only used to add the PreviewPanel which does not provide a way to input content. So we can safely remove that.

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 26, 2018
@Siedlerchr Siedlerchr mentioned this pull request Feb 28, 2018
Merged
6 tasks
@tobiasdiez
Copy link
Member

Ok, but we may still need it in the future for example when the preferences are migrated pane by pane. If the library makes problems with Java 9, then I'm OK with merge. Otherwise I would keep it for the moment until the majority of the dialogs is also converted.

@tobiasdiez
Copy link
Member

So, is our custom fx hack a problem for java 9 or not? In the latter case I would vote for closing this PR; otherwise just merge it.

@Siedlerchr
Copy link
Member Author

I want to get rid of the class completely, it can be safely removed as JavaFX is our first class citizen.
For swing content we need to embed we can now use Swing Node

I will update the PR to convert the remaining usage to fx

@Siedlerchr
Merge remote-tracking branch 'upstream/maintable-beta' into removeCus…
4bbd3b6 
…tomjfx

* upstream/maintable-beta: (28 commits)
  Fixes #3796: Pretend that we have every translation for every key (#3820)
  adjust line wrapping on column
  replace open office install selection dialog with choice dialog Wrap error messages in fx executor (because abstract worker is swing thread)
  Convert swing error dialog in save db action to custom fx dialog
  Replace swing export dialog in ExportToClipboardAction with fx choices dlg
  run find fulltext dialog in fx thread
  fix cut, copy & paste action from toolbar
  Set resizable for custom dialog with dialog pane convert merge entries dlg to javafx and make action working again
  checkstyle argh!
  Update test order -> Execute checkstyle first (#3807)
  embed cleanup dialog in javafx
  Fix some non FX Thread issues Main drawback: Cleanup panel is in backgroung due to swing thread)
  Allow search field to be smaller
  Fix Codacy Unused Params, Fields (#3753)
  Dialogstojavafx (#3801)
  Remove unused import
  Add group coloring in maintable as replacement for marked entries (#3769)
  Enable travis build for maintable-beta (#3804)
  Remove setting of tooltip manually
  Change open last edited dialgo to javafx (#3800)
  ...
@Siedlerchr
Remove TextArea and rework Edit Preamble Dialog to javafx to be on th…
4ee916a 
…e safe side with customjfxpanel
@Siedlerchr
Remove basePanel from Preamble Change
9bca863
@Siedlerchr
Copy link
Member Author

I now converted the Preamble Editor to javafx. This was the only critical thing which remained and had a TextArea as input field.
All other cases where Customjfxpanel is used is uncrictical and only the previewpanel in the External Changes Dialog.

tobiasdiez
tobiasdiez approved these changes Mar 11, 2018
View reviewed changes
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

I still have a bit of a stomachache because we might need to incorporate JavaFX again in Swing (I'm looking at you my dear preference dialog). But let's hope we don't and move forward with this PR.

@Siedlerchr
Copy link
Member Author

@tobiasdiez Well, we can simply do it the other way round then. We wrap it in javafx by using a SwingNode.
I did this with the cleanup operations already

@Siedlerchr Siedlerchr merged commit d31f2d8 into maintable-beta Mar 11, 2018
@Siedlerchr Siedlerchr deleted the removeCustomjfx branch March 11, 2018 17:36
Siedlerchr added a commit that referenced this pull request Mar 14, 2018
@Siedlerchr
Merge remote-tracking branch 'upstream/maintable-beta' into fixdragan…
e12335d 
…drop

* upstream/maintable-beta: (97 commits)
  Update Eclipse style to intellij style (#3827)
  fix some not on fx thread dialogs in preferences
  fix inital save error Convert last dialog to javafx refactor exception
  Merge changes for renamed actions
  New Crowdin translations (#3835)
  Partial revert of #3715 since double click on group should expand/collapse it (#3834)
  Add Spanish translations (#3833)
  update applicationsinsights
  update javafxsvg to 1.3.0
  Disable FTP Download on CI
  Apply copy linked files dialog fix from master
  Show dialog when copy files did not found file (#3826)
  Remove customjfx (#3779)
  Disable FTP Download on CI
  Remove color customization for maintable from preferences (#3808)
  Remove erroneous escape character (#3831)
  New translations JabRef_en.properties (French)
  New translations JabRef_en.properties (Tagalog)
  New translations JabRef_en.properties (Italian)
  New translations JabRef_en.properties (Indonesian)
  ...

# Conflicts:
#	src/main/java/org/jabref/gui/maintable/MainTable.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobiasdiez tobiasdiez tobiasdiez approved these changes

Assignees
No one assigned
Labels
os: linux status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Siedlerchr @tobiasdiez