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

FIX: set message crash with long error message #622

Merged
merged 5 commits into from
Oct 11, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions docs/source/upcoming_release_notes/622-fix_long_pos_msg_crash.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
622 fix_long_pos_msg_crash
##########################

API Breaks
----------
- N/A

Features
--------
- N/A

Bugfixes
--------
- Fix an issue where long status messages from positioner widget moves
would fail to display in certain circumstances.

Maintenance
-----------
- N/A

Contributors
------------
- zllentz
2 changes: 1 addition & 1 deletion typhos/positioner.py
Original file line number Diff line number Diff line change
Expand Up @@ -801,7 +801,7 @@ def _set_status_text(
tooltip = f"{text}: {tooltip}"
else:
tooltip = text
text = message.text[:max_length] + '...'
text = text[:max_length] + '...'
self.ui.status_label.setText(text)
if tooltip and "\n" not in tooltip:
# Force rich text, qt auto line wraps if it detects rich text
Expand Down
29 changes: 28 additions & 1 deletion typhos/tests/test_positioner.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

from typhos.alarm import KindLevel
from typhos.positioner import TyphosPositionerWidget
from typhos.status import TyphosStatusResult
from typhos.status import TyphosStatusMessage, TyphosStatusResult
from typhos.utils import SignalRO

from .conftest import RichSignal, show_widget
Expand Down Expand Up @@ -274,3 +274,30 @@ def has_limit_error():
assert 'LimitError' in widget.ui.status_label.text()

qtbot.waitUntil(has_limit_error, timeout=1000)


@pytest.mark.no_gc
def test_positioner_widget_long_status_text(motor_widget, qtbot):
Copy link
Contributor

Choose a reason for hiding this comment

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

I had to remind myselfl aoub this no_gc call (from an earlier CI overhaul), but its usage here jives with my recollection of when and where we were applying this

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add qtbot as a fixture argument to this test? We give qtbot the motor widget in fixture construction, and it's not used to waitUntil or similar in the test body

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you're right, this is probably a copy/paste test case no thinking situation

motor, widget = motor_widget

assert widget.ui.status_label.text() == ''

widget._set_status_text("Oh no! " * 1000, max_length=50)

text = widget.ui.status_label.text()
assert text.endswith("...")
assert len(text) < 60

widget._set_status_text("")
assert widget.ui.status_label.text() == ''

widget._set_status_text(
TyphosStatusMessage(
"Error! " * 1000,
tooltip="Test suite giant error string!",
),
max_length=50,
)
text = widget.ui.status_label.text()
assert text.endswith("...")
assert len(text) < 60