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

Smooth shutdown #4109

Merged
merged 7 commits into from
Jun 13, 2018
Merged

Smooth shutdown #4109

merged 7 commits into from
Jun 13, 2018

Conversation

tobiasdiez
Copy link
Member

@tobiasdiez tobiasdiez commented Jun 6, 2018

I tried to remove the System.exit call so that JabRef shutdown smoothly. However, currently an AWT-thread does not want to exit. In order to solve this problem, I removed most calls that would init the AWT toolkit. This includes:

  • Use JavaFX clipboard instead of AWT
  • Remove L&F and font customization

Moreover, the Azure telemetry SDK has a bug microsoft/ApplicationInsights-Java#662 that prevents certain threads from terminating. I added the workaround mentioned in this issue.


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

Copy link
Member

@stefan-kolb stefan-kolb left a comment

Choose a reason for hiding this comment

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

Looks good so far. I just added some comments that came to my mind.

}
// On Linux, Java FX fonts look blurry per default. This can be improved by using a non-default rendering
// setting. See https://github.com/woky/javafx-hates-linux
if (Globals.prefs.getBoolean(JabRefPreferences.FX_FONT_RENDERING_TWEAK)) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a Linux switch anywhere?

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 is turned on by default only on Linux: defaults.put(FX_FONT_RENDERING_TWEAK, OS.LINUX);


private static final Logger LOGGER = LoggerFactory.getLogger(ClipBoardManager.class);
public static final DataFormat XML = new DataFormat("application/xml");
Copy link
Member

Choose a reason for hiding this comment

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

Is this used anywhere?

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, when we put xml in the clipboard (which may happen for copied citation styles)

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, when we put xml in the clipboard (which may happen for copied citation styles)

Copy link
Member

Choose a reason for hiding this comment

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

We have already a class DragAndDropDataFormats, maybe you could rename that and add it to the collection?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want to interfere with your PR (especially, since I think it might completely remove the DragAndDropFormats class or make it internal to the DragAndDropManager).

import org.jabref.logic.l10n.Localization;
import org.jabref.logic.util.BuildInfo;

public class CopyVersionToClipboardAction extends AbstractAction {
Copy link
Member

Choose a reason for hiding this comment

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

Functionality totally removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is still a "copy version" button in the about dialog but no separate menu entry. (I didn't change anything as part of this PR; this class was completely unused).


public class ClipBoardManagerTest {

private ClipBoardManager clipBoardManager;
Copy link
Member

Choose a reason for hiding this comment

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

What about the tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

The clipboard manager needs to run on the JavaFX thread to work, which is not easily achieved in a test. Thus, deleting these tests was easier than trying to rewrite them ;-)

@tobiasdiez tobiasdiez changed the title [WIP] Smooth shutdown Smooth shutdown Jun 6, 2018
@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jun 6, 2018
@Siedlerchr
Copy link
Member

Siedlerchr commented Jun 6, 2018

I created a branch from master, cherry picked 39e30cc, and deleted all system.exit() calls.
It works fine for me. For reference, I have created a PR to show the diff: #4111

@tobiasdiez tobiasdiez mentioned this pull request Jun 7, 2018
6 tasks
Copy link
Member

@stefan-kolb stefan-kolb left a comment

Choose a reason for hiding this comment

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

So we use this insetad of the PR of Christoph?!

client.trackSessionState(SessionState.End);
client.flush();

// Workaround for bug https://github.com/Microsoft/ApplicationInsights-Java/issues/662
Copy link
Member

Choose a reason for hiding this comment

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

Add FIXME so we can revert this after they merged the bug fix.

Copy link
Member

Choose a reason for hiding this comment

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

I really would use my PR as a base. As this done does more than the initial necessary solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand the complication (including merge conflicts) but well it doesn't really matter. @Siedlerchr @stefan-kolb can I merge this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Guess so.

@tobiasdiez tobiasdiez merged commit 70e9734 into master Jun 13, 2018
@tobiasdiez tobiasdiez deleted the smoothShutdown branch June 13, 2018 16:04
@Siedlerchr
Copy link
Member

Siedlerchr commented Jun 14, 2018

Your PR caused this error now

 java.lang.ExceptionInInitializerError
        at org.jabref.gui.fieldeditors.LinkedFileViewModelTest.setUp(LinkedFileViewModelTest.java:55)
        Caused by:
        java.lang.IllegalStateException: This operation is permitted on the event thread only; currentThread = Test worker
            at com.sun.glass.ui.Application.checkEventThread(Application.java:443)
            at com.sun.glass.ui.ClipboardAssistance.<init>(ClipboardAssistance.java:40)
            at com.sun.javafx.tk.quantum.QuantumToolkit.getSystemClipboard(QuantumToolkit.java:1200)
            at javafx.scene.input.Clipboard.getSystemClipboardImpl(Clipboard.java:413)
            at javafx.scene.input.Clipboard.getSystemClipboard(Clipboard.java:178)
            at org.jabref.gui.ClipBoardManager.<init>(ClipBoardManager.java:41)
            at org.jabref.Globals.<clinit>(Globals.java:57)
            ... 1 more


https://travis-ci.org/JabRef/jabref/jobs/392180785

@Siedlerchr
Copy link
Member

And removing setting the font size without providing an alternative was not the good idea!

@stefan-kolb
Copy link
Member

stefan-kolb commented Jun 14, 2018 via email

@tobiasdiez
Copy link
Member Author

Sorry, I was so used to failing tests against the maintable-branch that I ignored the red travis icon. Tests are now fixed in master.

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