QObject based RunEngine#9
Conversation
|
@teddyrendahl How does this merge look? |
| @@ -0,0 +1,305 @@ | |||
| import logging | |||
| from bluesky import RunEngine | |||
| from bluesky.utils import RunEngineInterrupted, install_qt_kicker | |||
There was a problem hiding this comment.
install_qt_kicker deprecated: https://github.com/bluesky/bluesky/blob/f18f759e58bfa2453c66c524d0479d409f64304c/bluesky/utils.py#L785
|
|
||
| class QRunEngine(QObject): | ||
| sigDocumentYield = Signal(str, dict) | ||
| sigAbort = Signal() # TODO: wireup me |
| # Instantiate widget information and layout | ||
| super().__init__(parent=parent) | ||
|
|
||
| self.setStyleSheet('QLabel {qproperty-alignment: AlignCenter}') |
There was a problem hiding this comment.
I wonder if mily should make a consistent set of rules as to how QWidget subclasses handle style sheets.
There was a problem hiding this comment.
Not necessarily disagreeing but I think this is pretty harmless here? I believe this is to avoid running a for loop over all the labels to and using QLabel.setAlignment.
There was a problem hiding this comment.
Harmless, sure - but not so easily extensible.
I doubt it's a controversial opinion, but I think everything in mily should be designed with extensibility in mind, where possible.
teddyrendahl
left a comment
teddyrendahl
left a comment
There was a problem hiding this comment.
Thanks for working on this @ronpandolfi!
| # Instantiate widget information and layout | ||
| super().__init__(parent=parent) | ||
|
|
||
| self.setStyleSheet('QLabel {qproperty-alignment: AlignCenter}') |
There was a problem hiding this comment.
Not necessarily disagreeing but I think this is pretty harmless here? I believe this is to avoid running a for loop over all the labels to and using QLabel.setAlignment.
| self.engine = engine or QRunEngine() | ||
|
|
||
|
|
||
| @property |
There was a problem hiding this comment.
On the first iteration @tacaswell suggested that we instead have a start QSlot that accepts a plan. That way you can easily pass in plans w/o dealing with attributes.
There was a problem hiding this comment.
Still stand by that suggestion, the RE widget should not hold any plans as state, but should be passed plans to be run by some other part of the GUI.
Not sure if passing (plan_fun, args, kwargs) and expecting this widget to call the plan or passing in an iterable makes more sense. In the case of passing in the callable + args it gives the widget a chance to do some nice introspcetion (like displaying the name of the plan it is running). On the other hand, the iterable scheme matches what the RE does more directly.
| self.setLayout(lay) | ||
| self._plan = None | ||
|
|
||
| # Accept either RunEngine or QRunengine for engine |
There was a problem hiding this comment.
I think this logic needs to go in the engine.setter if we truly want to accept both? For example:
# Will be QRunEngine
RE = RunEngine()
ew = EngineWidget(engine=RE)
# Won't be QRunEngine
RE = RunEngine()
ew = EngineWidget()
ew.engine = RE| return self._engine | ||
|
|
||
| @engine.setter | ||
| def engine(self, engine): |
There was a problem hiding this comment.
If we have an existing _engine we need to disconnect the prior signals from their slots. Otherwise we risk having our widget being updated by multiple engines
There was a problem hiding this comment.
It is not clear to me that allowing changing the runengine behind the widget is something we want to allow. If we really need to throw away a RunEngine instance might as well make a new Qt widget for it as well?
| try: | ||
| func(self.engine) | ||
| # Pausing raises an exception | ||
| except RunEngineInterrupted as exc: |
There was a problem hiding this comment.
Might be worth catching generic Exception objects here. Nobody likes a GUI crash and this is where any bubbles from the execution of the plan will bubble up. There might also need to be some logic to make sure it is ready to take another plan afterwards (teardown and re-up of a failed RunEngine execution)? 🤔
|
|
||
| from mily.runengine import EngineWidget | ||
|
|
||
| # Fixtures borrowed from pytest-qt |
There was a problem hiding this comment.
Just add pytest-qt as you discussed offline. Commenting here as a reminder.
|
Can someone remind me what the plan for this and https://github.com/bluesky/bluesky-ui/tree/master/bluesky_ui is? |
| def __init__(self, runengine=None, **kwargs): | ||
| super(QRunEngine, self).__init__() | ||
|
|
||
| self.RE = runengine or RunEngine(context_managers=[], **kwargs) |
There was a problem hiding this comment.
Given that we need to create the RE with just the right settings (no context managers, probably a different during_task, and maybe using qthreads instead of python threads to push the asyncio event loop to the background, maybe with qumash to embed the asyncio event loop in the qt event loop (!!)) I don't think we should allow the user to pass in an existing RunEngine
4 participants
No description provided.