Avoid FlatLaf crashing the GUI #135

ctrueden · 2022-11-17T16:14:59Z

Always queue ui.show/showUI calls on the EDT
Fix usage of private and now-removed PyImageJ API
Use existing imagej.gateway if already running
Add a hack to avoid FlatLaf in problem situations
Include imagej-legacy by default

This avoids threading issues which cause deadlocks on macOS.

In a future PyImageJ release, when running in GUI mode (and *only* when running in GUI mode), the created ImageJ2 gateway will be cached in the global variable imagej.gateway, so that it is accessible from other threads, since the imagej.init function never returns in that case. We can utilize this in napari-imagej to hook into that ImageJ2 gateway, rather than creating a new one. By doing this, napari-imagej becomes usable interactively while PyImageJ is in GUI mode, which means it can now work with a GUI on macOS! But you need to start napari + PyImageJ in a special way: import imagej, napari napari.Viewer() imagej.init(mode='gui') As an aside: this construction does *not* work on non-macOS platforms, because imagej.init(mode='gui') manually blocks the thread with a polling sleep loop, which means that napari.run() does not get invoked to start up napari's event loop, which results in napari hanging. However, on Linux (and likely Windows) you can instead do: import imagej, napari imagej.gateway = imagej.init(mode='interactive') imagej.gateway.ui().showUI() napari.Viewer() napari.run() To utilize the pathway enabled by this commit.

Specifically, if the UI is swing rather than legacy, then FlatLaf doesn't work with napari-imagej for some reason. We need to figure out why, but for the moment, this HACK avoids the issue by switching to the system default Look+Feel proactively before the Java UI can crash. Unfortunately, it has the side effect of changing the user's preferred Look+Feel preference globally for all their ImageJ2 instances, not just within napari-imagej. So we'll eventually need something better. And in any case, we should make FlatLaf work, since FlatLaf is great.

This avoids the issue with FlatLaf not working with the Swing UI. But we'll need to figure out what to do about imagej/imagej-legacy#280.

gselzer

Thanks so much @ctrueden!

I had a couple ideas, please let me know what you think.

Also, looks like there are a couple of places where I'm still using old PyImageJ API, causing the tests to fail 😨 We should probably:

Update those to make the tests pass
Probably bump up the minimum pyimagej version, because those functions are new, right?

gselzer · 2022-11-17T16:19:31Z

src/napari_imagej/config_default.yaml

 # This can be used to include original ImageJ functionality.
 # Iff true, original ImageJ functionality (ij.* packages) will be available.
 # Defaults to false.
-include_imagej_legacy: false
+include_imagej_legacy: true


What is the reasoning for this change?

Assuming we do this, we also need to change the documentation.

I wrote the reasoning into the commit message.

"This avoids the issue with FlatLaf not working with the Swing UI."

Because whenever the FlatLaf avoidance hack is triggered, it changes the user's preferred Look+Feel globally to the system one, meaning the next time they run Fiji (outside napari-imagej), it won't be FlatLaf for them anymore. 😢

Wasn't the point of the hack napari_imagej/java to get around that issue? Or am I missing something?

We made add_legacy=False the default before we added the GUI buttons, right? Now that we have the GUI buttons, bringing up the unfinished Swing UI by default strikes me as rather rude; people are very used to the legacy UI, and it is much better tested, and the FlatLaf bug doesn't happen with that UI.

I agree that there are challenges to solve if we change this, particularly imagej/imagej-legacy#280.

I agree that there are challenges to solve if we change this, particularly imagej/imagej-legacy#280.

I thought this was the main reason that we chose to use pure IJ2. See #58

The other reason that we chose to use pure IJ2 is that closing the legacy UI disables napari-imagej, as described in #93.

Tbh, I don't really like either option here.

Also, if we do decide to make legacy the default, we should rewrite the conda job testing legacy to test pure IJ2.

Actually, if you close the IJ2 UI on this branch, you terminate napari. So that stinks, and we have to get to the bottom of that...

Actually, if you close the IJ2 UI on this branch, you terminate napari. So that stinks, and we have to get to the bottom of that...

Okay, this seems to be fixed with 008daec.

src/napari_imagej/java.py

src/napari_imagej/widgets/menu.py

ctrueden · 2022-11-17T16:50:23Z

@gselzer Do you have time to fix up this PR along the lines of your suggestions? I have a dozen other things I'm supposed to get done today.

gselzer · 2022-11-17T16:51:32Z

@gselzer Do you have time to fix up this PR along the lines of your suggestions? I have a dozen other things I'm supposed to get done today.

Yeah, I think I can fix it up.

We have to bump the minimum pyimagej version to use it

codecov-commenter · 2022-11-17T19:15:14Z

Codecov Report

Base: 91.34% // Head: 89.89% // Decreases project coverage by -1.44% ⚠️

Coverage data is based on head (251977f) compared to base (9ce9650).
Patch coverage: 27.77% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #135      +/-   ##
==========================================
- Coverage   91.34%   89.89%   -1.45%     
==========================================
  Files          41       41              
  Lines        3917     3949      +32     
==========================================
- Hits         3578     3550      -28     
- Misses        339      399      +60

Impacted Files	Coverage Δ
src/napari_imagej/java.py	`81.88% <8.10%> (-5.92%)`	⬇️
src/napari_imagej/widgets/menu.py	`74.04% <54.54%> (-11.68%)`	⬇️
src/napari_imagej/types/converters/images.py	`100.00% <100.00%> (ø)`
src/napari_imagej/types/converters/labels.py	`100.00% <100.00%> (ø)`
src/napari_imagej/utilities/_module_utils.py	`87.50% <100.00%> (+0.04%)`	⬆️
tests/widgets/test_menu.py	`72.16% <0.00%> (-3.31%)`	⬇️
src/napari_imagej/widgets/result_tree.py	`89.62% <0.00%> (-1.89%)`	⬇️
tests/widgets/test_result_runner.py	`83.67% <0.00%> (+8.16%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Otherwise the post-showing setup may not work as intended. For example, closing the IJ2 GUI will terminate napari, if we don't do this.

gselzer · 2022-11-17T21:09:13Z

Looking at the CodeCov results:

We're losing a significant amount of coverage in napari_imagej/widgets/menu.py, but I'm alright with that since the code that lost coverage is now being run on a different thread thanks to ij().thread().queue().
We're also losing coverage in napari_imagej/java.py, since we are no longer running IJ2 by default. Thus the new HACK fixing L+F isn't getting tested. I'm okay with skipping the coverage here as long as we rewrite the legacy CI test to be a IJ2 UI test.
We're skipping a test in tests/widgets/test_menu.py, because it gets skipped on CI. The reason for skipping, NPE from Napari/ImageJ2 Data Transfer on CI with ImageJ Legacy #124, feels relevant to this PR @ctrueden

Steps to merge:

Resolve this discussion
- Rewrite the legacy CI testing job if needed

ctrueden added 5 commits November 16, 2022 14:56

Always queue ui.show/showUI calls on the EDT

f8e6cd2

This avoids threading issues which cause deadlocks on macOS.

Fix usage of private and now-removed PyImageJ API

907d916

Include imagej-legacy by default

8baab60

This avoids the issue with FlatLaf not working with the Swing UI. But we'll need to figure out what to do about imagej/imagej-legacy#280.

ctrueden added this to the 0.1.0 milestone Nov 17, 2022

ctrueden added the bug Something isn't working label Nov 17, 2022

ctrueden requested a review from gselzer November 17, 2022 16:15

gselzer reviewed Nov 17, 2022

View reviewed changes

Update imagej API

4b74cc0

We have to bump the minimum pyimagej version to use it

gselzer added 5 commits November 17, 2022 13:43

Run post-showing setup on Java GUI thread, too

008daec

Otherwise the post-showing setup may not work as intended. For example, closing the IJ2 GUI will terminate napari, if we don't do this.

Format code

2f348af

Partition ImageJ config and init into functions

71c849a

Add TODOs for scyjava EventQueue wrappers

0c813de

Better comments for EDT queue calls

251977f

gselzer force-pushed the fix-gui branch from 6a0648f to 251977f Compare November 17, 2022 21:01

Configure legacy UI as the default

9c0d65d

gselzer merged commit 5498c0d into main Nov 18, 2022

gselzer deleted the fix-gui branch November 18, 2022 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid FlatLaf crashing the GUI #135

Avoid FlatLaf crashing the GUI #135

ctrueden commented Nov 17, 2022

gselzer left a comment

gselzer Nov 17, 2022

ctrueden Nov 17, 2022

ctrueden Nov 17, 2022

ctrueden Nov 17, 2022

gselzer Nov 17, 2022

ctrueden Nov 17, 2022 •

edited

Loading

gselzer Nov 17, 2022 •

edited

Loading

gselzer Nov 17, 2022

gselzer Nov 17, 2022

gselzer Nov 17, 2022 •

edited

Loading

ctrueden commented Nov 17, 2022

gselzer commented Nov 17, 2022

codecov-commenter commented Nov 17, 2022 •

edited

Loading

gselzer commented Nov 17, 2022

Avoid FlatLaf crashing the GUI #135

Avoid FlatLaf crashing the GUI #135

Conversation

ctrueden commented Nov 17, 2022

gselzer left a comment

Choose a reason for hiding this comment

gselzer Nov 17, 2022

Choose a reason for hiding this comment

ctrueden Nov 17, 2022

Choose a reason for hiding this comment

ctrueden Nov 17, 2022

Choose a reason for hiding this comment

ctrueden Nov 17, 2022

Choose a reason for hiding this comment

gselzer Nov 17, 2022

Choose a reason for hiding this comment

ctrueden Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

gselzer Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

gselzer Nov 17, 2022

Choose a reason for hiding this comment

gselzer Nov 17, 2022

Choose a reason for hiding this comment

gselzer Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

ctrueden commented Nov 17, 2022

gselzer commented Nov 17, 2022

codecov-commenter commented Nov 17, 2022 • edited Loading

Codecov Report

gselzer commented Nov 17, 2022

ctrueden Nov 17, 2022 •

edited

Loading

gselzer Nov 17, 2022 •

edited

Loading

gselzer Nov 17, 2022 •

edited

Loading

codecov-commenter commented Nov 17, 2022 •

edited

Loading