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

Add an ij.py.show_ui function #260

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add an ij.py.show_ui function #260

wants to merge 2 commits into from

Conversation

ctrueden
Copy link
Member

@ctrueden ctrueden commented May 5, 2023

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 ctrueden changed the title WIP: Add an ij.py.show_ui function Add an ij.py.show_ui function May 5, 2023
@ctrueden
Copy link
Member Author

ctrueden commented May 5, 2023

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

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).
Copy link
Contributor

@gselzer gselzer left a comment

Choose a reason for hiding this comment

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

Looks great, although I have a suggestion and also a question:

Suggestion: Could we use @JImplementationFor to make this the default behavior for UIService.showUI()? Then we don't even need new API.

Question: Do we also want to migrate the "on-close" behavior of napari-imagej? Because without that, we might want a docstring as to why we're messing with the application frame - i.e. it might be more logical to do nothing otherwise, if ui.isVisible() returns true.

src/imagej/__init__.py Outdated Show resolved Hide resolved
@elevans
Copy link
Member

elevans commented May 5, 2023

I really like the option to easily call the ImageJ2 UI instead of legacy. This also seems to alleviate the scijava/scijava-common#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. 👍

@gselzer
Copy link
Contributor

gselzer commented May 5, 2023

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?

Co-authored-by: Gabriel Selzer <[email protected]>
@ctrueden
Copy link
Member Author

ctrueden commented May 5, 2023

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.

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 12.50% and project coverage change: -0.28 ⚠️

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

📣 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%) ⬇️

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

@elevans
Copy link
Member

elevans commented Jan 29, 2024

@ctrueden Since scijava/scijava-common#460 has been resolved and merged, is this still an issue? Do we need this PR?

@ctrueden
Copy link
Member Author

@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.

@elevans
Copy link
Member

elevans commented Jan 30, 2024

Thanks for the reply! Sounds good. I'll do some digging ⛏️.

@hinerm
Copy link
Member

hinerm commented Jul 16, 2024

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

@ctrueden
Copy link
Member Author

See also scijava/scijava-common@3074675

@elevans
Copy link
Member

elevans commented Jul 19, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants