pyimagej icon indicating copy to clipboard operation
pyimagej copied to clipboard

Add an ij.py.show_ui function

Open ctrueden opened this issue 1 year ago • 11 comments

It takes care to show the UI on the AWT event dispatch thread.

If the UI is already visible, it raises the application frame instead of calling ij.ui().showUI() again, since SciJava Common currently has a bug when doing that where it hoses the UI (see scijava/scijava-common#460).

@elevans @hinerm @gselzer What do y'all think? Good? Or do you see any possible problems with this? If it were part of the PyImageJ library, I think it would reduce the code footprint of napari-imagej a little bit.

ctrueden avatar May 05 '23 02:05 ctrueden

One small question is: instead of using only setVisible(true), should we also be invoking requestFocus() on the already-shown application frame?

ctrueden avatar May 05 '23 02:05 ctrueden

I really like the option to easily call the ImageJ2 UI instead of legacy. This also seems to alleviate the https://github.com/scijava/scijava-common/issues/460 issue which is good. I think the only potential downside here is the name could be confusing to users on which call they should use. Generally most people can use either one so I think this can be easily addressed in the docs. :+1:

elevans avatar May 05 '23 14:05 elevans

One small question is: instead of using only setVisible(true), should we also be invoking requestFocus() on the already-shown application frame?

Oh, and as far as this goes, I think sure?

gselzer avatar May 05 '23 15:05 gselzer

I think the only potential downside here is the name could be confusing to users on which call they should use.

I agree, and now I'm leaning more toward fixing this behavior on the Java side to more closely reflect the above logic... at which point we would not need to add this code to PyImageJ anymore.

ctrueden avatar May 05 '23 20:05 ctrueden

Codecov Report

Patch coverage: 12.50% and project coverage change: -0.28 :warning:

Comparison is base (8baaac0) 76.93% compared to head (e9b6e1e) 76.66%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #260      +/-   ##
==========================================
- Coverage   76.93%   76.66%   -0.28%     
==========================================
  Files          16       16              
  Lines        1869     1877       +8     
==========================================
+ Hits         1438     1439       +1     
- Misses        431      438       +7     
Impacted Files Coverage Δ
src/imagej/__init__.py 61.52% <12.50%> (-0.71%) :arrow_down:

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov-commenter avatar May 05 '23 20:05 codecov-commenter

@ctrueden Since https://github.com/scijava/scijava-common/issues/460 has been resolved and merged, is this still an issue? Do we need this PR?

elevans avatar Jan 29 '24 21:01 elevans

@elevans I don't know; testing needed. There is an edit to the scijava/scijava-common#465 PR saying it does not actually fix scijava/scijava-common#460, which I guess would mean more work could be needed on the Java side and/or here. Feel free to dig.

ctrueden avatar Jan 30 '24 14:01 ctrueden

Thanks for the reply! Sounds good. I'll do some digging :pick:.

elevans avatar Jan 30 '24 15:01 elevans

Looks like this is working around a bug in scijava common and if fixed there we can close this PR

hinerm avatar Jul 16 '24 15:07 hinerm

See also scijava/scijava-common@3074675da4f3cf27544c8241fe674ae87b73468c

ctrueden avatar Jul 16 '24 16:07 ctrueden

I'll test out that change and report back to make sure it works as intended. If it does then I think we can close out this PR and rely on this fix.

elevans avatar Jul 19 '24 15:07 elevans