Skip to content

Comments

Add reset buttons to sliders in new Instrument View#40830

Open
jclarkeSTFC wants to merge 8 commits intomantidproject:mainfrom
jclarkeSTFC:40776_reset_buttons_iv_sliders
Open

Add reset buttons to sliders in new Instrument View#40830
jclarkeSTFC wants to merge 8 commits intomantidproject:mainfrom
jclarkeSTFC:40776_reset_buttons_iv_sliders

Conversation

@jclarkeSTFC
Copy link
Contributor

@jclarkeSTFC jclarkeSTFC commented Feb 5, 2026

Instead of dragging the contour and integration limit sliders in the new Instrument View, one can now click a reset button to return to the maximum range.

Closes #40776.

  • Contour limits are recalculated when the integration limits are changed (since changing the integration limits will update the min and max counts).
  • The line plot x-axis will now update when the integration limits are changed.

To test:

Play around with both sliders and min/max edit boxes, check that the button works and the image updates correctly


Reviewer

Your comments will be used as part of the gatekeeper process. Comment clearly on what you have checked and tested during your review. Provide an audit trail for any changes requested.

As per the review guidelines:

  • Is the code of an acceptable quality? (Code standards/GUI standards)
  • Has a thorough functional test been performed? Do the changes handle unexpected input/situations?
  • Are appropriately scoped unit and/or system tests provided?
  • Do the release notes conform to the guidelines and describe the changes appropriately?
  • Has the relevant (user and developer) documentation been added/updated?
  • If the PR author isn’t in the mantid-developers or mantid-contributors teams, add a review comment rerun ci to authorize/rerun the CI

Gatekeeper

As per the gatekeeping guidelines:

  • Has a thorough first line review been conducted, including functional testing?
  • At a high-level, is the code quality sufficient?
  • Are the base, milestone and labels correct?

@jclarkeSTFC jclarkeSTFC added this to the Release 6.16 milestone Feb 5, 2026
@jclarkeSTFC jclarkeSTFC added the Epic Used for issues and PRs that are managed under the ISIS Epic Workstream label Feb 5, 2026
@jclarkeSTFC
Copy link
Contributor Author

@coderabbitai full review

@coderabbitai
Copy link

coderabbitai bot commented Feb 5, 2026

✅ Actions performed

Full review triggered.

@coderabbitai
Copy link

coderabbitai bot commented Feb 5, 2026

📝 Walkthrough

Walkthrough

This pull request implements reset button functionality for the Instrument View sliders. The release notes documentation was updated to reflect the new feature. The view model now stores full limit values for integration and counts ranges. The presenter introduces two new methods to handle integration and contour range reset actions. The window class extends the min/max group box UI component to include reset buttons, adds helper methods to update edit boxes with new limit values, and establishes signal connections between the reset buttons and presenter methods.

Suggested reviewers

  • RichardWaiteSTFC
  • cailafinn
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title clearly and concisely describes the main change: adding reset buttons to sliders in the new Instrument View, which is directly supported by the code changes.
Linked Issues check ✅ Passed The code changes implement the requirement from issue #40776 to add reset buttons for the contour and integration range sliders in the new Instrument View.
Out of Scope Changes check ✅ Passed All code changes are directly related to implementing reset button functionality for the sliders as specified in issue #40776; no out-of-scope changes are present.
Description check ✅ Passed The pull request description clearly describes adding reset buttons to the contour and integration limit sliders in the new Instrument View, aligning with the changeset modifications across multiple files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@qt/python/instrumentview/instrumentview/FullInstrumentViewPresenter.py`:
- Around line 144-148: The reset handler on_integration_limits_reset_clicked
calls update_integration_range(...) which recalculates counts but does not
assign the model's integration_limits, leaving _integration_limits stale; after
calling update_integration_range(self._model.full_integration_limits,
entire_range=True) explicitly set the model's integration_limits to the
full_integration_limits (e.g. self._model.integration_limits =
self._model.full_integration_limits or via the model's setter if one exists)
before updating the view with set_integration_range_limits,
set_integration_min_max_boxes and set_view_integration_limits so downstream
save/export reads the updated limits.

@coderabbitai
Copy link

coderabbitai bot commented Feb 5, 2026

Note

Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it.


Generating unit tests... This may take up to 20 minutes.

@coderabbitai

This comment was marked as resolved.

@jclarkeSTFC jclarkeSTFC marked this pull request as ready for review February 5, 2026 13:00
@andy-bridger andy-bridger self-assigned this Feb 11, 2026
Copy link
Collaborator

@andy-bridger andy-bridger left a comment

Choose a reason for hiding this comment

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

I have played around with it and am happy that it functions as described, I have a few questions as to things that may well be intentional choices and/or targets of future work, so just say the word and I am happy to approve this for what it is.

My main question is for the contour range limits. By having the fixed max and min limits based on the integration of the entire spectrum, if I have cropped to a small subset of my data with the integration limit slider, I can't obtain meaningful contours as all the integrated counts in this new integration range are below the global lower limit - could the contour range limits be dynamic and depend on the integration limits?

Follow up is about the integration limits, and this one is I'm guessing something that has already been discussed/ decided upon. I would have found it useful for testing this if the integration limits chosen were reflected in the plotted spectra as well as in the instrument render, is this going to be done in future?

When changing the integration limits, the count range needs to be
updated, otherwise the contour will be showing a range that is not
useful.
When setting the x limits on the line plot, we need to handle any
unit conversion from the workspace unit to the currentl selected
unit for the line plot.
@jclarkeSTFC
Copy link
Contributor Author

I have played around with it and am happy that it functions as described, I have a few questions as to things that may well be intentional choices and/or targets of future work, so just say the word and I am happy to approve this for what it is.

My main question is for the contour range limits. By having the fixed max and min limits based on the integration of the entire spectrum, if I have cropped to a small subset of my data with the integration limit slider, I can't obtain meaningful contours as all the integrated counts in this new integration range are below the global lower limit - could the contour range limits be dynamic and depend on the integration limits?

Follow up is about the integration limits, and this one is I'm guessing something that has already been discussed/ decided upon. I would have found it useful for testing this if the integration limits chosen were reflected in the plotted spectra as well as in the instrument render, is this going to be done in future?

Thanks, I think I've implemented both of the things you mentioned, should be more usable now.

@andy-bridger
Copy link
Collaborator

Thanks, I think those changes make it much easier to use!

From being easier to use, I've used it a bit more, I think there is probably something which needs to be done for the integration limits when units are in d-spacing. Seemingly from quite a few instruments I've tested you can have a banks of detectors with vastly different d-space ranges, which make the sliders basically pointless (see below for enginx, with a transmission bank that makes the slider pretty unuseable for the diffraction banks)

image

and to a lesser extent osiris:
image

I had two ideas for solving the issue (I suppose three if you count "do nothing, address in another PR"):

  • Have some to buttons "set new integration limits", "reset slider to integration limit", "reset integration limits"
  • Have the integration limits and contour respect masking so at least you can mask out banks that are annoying

I think the former gives more user control but will probably be difficult to make aesthetically pleasing, the latter perhaps should be included either way?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Epic Used for issues and PRs that are managed under the ISIS Epic Workstream

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reset buttons for Instrument View sliders

2 participants