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

ENH: add ScientificDoubleSpinBox and use it in MultiModeValueEdit #208

Merged
merged 6 commits into from
Sep 20, 2023

Conversation

tangkong
Copy link
Contributor

@tangkong tangkong commented Sep 19, 2023

Description

Creates a ScientificDoubleSpinBox for use in MultiModeValueEdit
closes #207

Motivation and Context

Scientific Notation DoubleSpinBox

The standard DoubleSpinBox has limited significant digits, and will coerce values with more sig figs. With the standard QSpinBox we arbitrarily chose a number of decimals (4), which prevented more precise values in comparisons.

I can't really take credit for this implementation, but I did make it qt5 compatible (why does validate return a tuple[state, str, int]? Why can't it just take the validation status? 🤔 This apparently changed a lot over qt versions)

Reported by Rizheng.

Enum ComboBox failing to load on init (#207)

EnumStrings weren't initializing properly in some cases (initializing to an empty list), causing the widget to be effectively non-operational. Added a wait_for_connection call, which is added to a BusyCursorThread.

Test removal

test_edit_run_toggle has caused problems in the past, but with the wait_for_connection call added to fix #207 this started to fail more consistently. Since some (many) of the test configs have PVs that will require the full timeout and eventually fail, I'm hypothesizing these threads/timeouts are holding onto widgets. I've removed the test for now, but do want to re-add a test to make sure edit-to-run conversions do well. Maybe later though

How Has This Been Tested?

Interactively

Where Has This Been Documented?

This PR
image

Pre-merge checklist

  • Code works interactively
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Code has been checked for threading issues (no blocking tasks in GUI thread)
  • Test suite passes locally
  • Test suite passes on GitHub Actions
  • Ran docs/pre-release-notes.sh and created a pre-release documentation page
  • Pre-release docs include context, functional descriptions, and contributors as appropriate

@tangkong tangkong requested review from ZLLentz and klauer September 19, 2023 22:12
Copy link
Contributor

@klauer klauer left a comment

Choose a reason for hiding this comment

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

I'm honestly OK with this as is, despite my comments.

If we could do away with the regex I'd clearly be happier though

def validate(self, string: str, position: int) -> Tuple[QValidator.State, str, int]:
if valid_float_string(string):
return (QValidator.Acceptable, string, position)
if string == "" or string[position-1] in 'e.-+':
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing array[x-1] with no bounds check concerns me a bit...
Have an explanation as to how validate() gets called with string and position to help me understand why such a check isn't necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Validate is a method of entry widgets, string is the string in the widget, and position is the cursor position in that string. This clause is what lets you type out "2e" or "2." and not have the validator block your inputs. position will be in range(len(string)).

A closer look at this reveals weird ways to break this logic. I'll try to touch it up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This logic branch isn't necessarily perfect, but in cases where you type an invalid float (say: "2.3ee", where the validator still reports "intermediate"), the spinbox will revert the change if you hit enter/shift focus. I actually think this is ok as is, since the fixup method will repair broken entries.

atef/widgets/config/utils.py Outdated Show resolved Hide resolved
atef/widgets/config/utils.py Outdated Show resolved Hide resolved
atef/widgets/config/utils.py Show resolved Hide resolved
@tangkong
Copy link
Contributor Author

Thanks for keeping me honest. I've nuked the regex and condensed the logic into ScientificDoubleSpinBox.

Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

This looks good to me

@tangkong tangkong merged commit 8212d7f into pcdshub:master Sep 20, 2023
8 of 9 checks passed
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.

BUG: enum values in SetValueStep sometimes fail to load
3 participants