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

create ExposureWidget as a core widget + properly connect exposure signals #150

Merged
merged 8 commits into from
Apr 28, 2022

Conversation

ianhi
Copy link
Contributor

@ianhi ianhi commented Mar 14, 2022

Adds two new core widgets:

  1. A general exposure widget that takes any camera neame
  2. an exposure widget that follows the current camera device (getCameraDevice) as closely as possible. Though not perfectly, see setCameraDevice does not emit a propertyChanged event micro-manager/mmCoreAndDevices#181

There's also a bugfix in here. Previously if you loaded a config before opening napari then the exposure widget would not fill with the correct value - now it does.

@ianhi ianhi mentioned this pull request Mar 14, 2022
@ianhi ianhi changed the title make an ExposureWidget as a core widget + properly connect exposure signals create ExposureWidget as a core widget + properly connect exposure signals Mar 14, 2022
@ianhi
Copy link
Contributor Author

ianhi commented Mar 14, 2022

Test failure is unrelated: https://github.com/tlambert03/napari-micromanager/issues/153 so this is ready for review

@ianhi ianhi mentioned this pull request Mar 14, 2022
@tlambert03
Copy link
Member

can you rebase on main?

@codecov
Copy link

codecov bot commented Mar 14, 2022

Codecov Report

Merging #150 (4e6ff81) into main (96a1f1f) will increase coverage by 0.43%.
The diff coverage is 92.53%.

@@            Coverage Diff             @@
##             main     #150      +/-   ##
==========================================
+ Coverage   80.23%   80.66%   +0.43%     
==========================================
  Files          23       24       +1     
  Lines        2054     2100      +46     
==========================================
+ Hits         1648     1694      +46     
  Misses        406      406              
Impacted Files Coverage Δ
micromanager_gui/main_window.py 69.36% <40.00%> (-0.05%) ⬇️
micromanager_gui/_core_widgets/_exposure_widget.py 96.55% <96.55%> (ø)
micromanager_gui/_core_widgets/__init__.py 100.00% <100.00%> (ø)
micromanager_gui/_gui_objects/_tab_widget.py 93.58% <100.00%> (-0.46%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96a1f1f...4e6ff81. Read the comment docs.

@ianhi ianhi mentioned this pull request Mar 14, 2022
@ianhi
Copy link
Contributor Author

ianhi commented Apr 27, 2022

@tlambert03 or @fdrgsp if you get a chance to look at this sometime that'd be great - the exposure not prefilling has been driving me nuts

Copy link
Member

@tlambert03 tlambert03 left a comment

Choose a reason for hiding this comment

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

nice! very minor comment/question. otherwise LGTM!

g_Keyword_CoreDevice, g_Keyword_CoreCamera
).connect(self._camera_updated)

def setCamera(self, camera: str = None, force: bool = False):
Copy link
Member

Choose a reason for hiding this comment

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

are you thinking of a use case for this? or just maybe nice to have? maybe "yagni"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the method on ExposureWidget or the version on the DefaultCamera version?

Copy link
Member

Choose a reason for hiding this comment

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

The one with the big warning on default cam... ah, but maybe I see now, it's already there in the superclass and you're just restricting it on the subclass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah exactly.

micromanager_gui/_core_widgets/_exposure_widget.py Outdated Show resolved Hide resolved
@tlambert03 tlambert03 merged commit 318724a into pymmcore-plus:main Apr 28, 2022
@ianhi ianhi deleted the exposure-widget branch April 28, 2022 16:34
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.

None yet

2 participants