Add identifier for output workspaces in Elwin Interface #37762
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adriazalvarez
force-pushed
the
label_output_workspaces
branch
from
8c23f3a to
351792a
Compare
github-actions
bot
added
the
Has Conflicts
|
👋 Hi, @adriazalvarez, Conflicts have been detected against the base branch. Please rebase your branch against the base branch. |
adriazalvarez
added
the
Awaiting User Response
adriazalvarez
force-pushed
the
label_output_workspaces
branch
from
c88164b to
a601281
Compare
github-actions
bot
removed
the
Has Conflicts
|
👋 Hi, @adriazalvarez, Conflicts have been detected against the base branch. Please rebase your branch against the base branch. |
github-actions
bot
added
the
Has Conflicts
adriazalvarez
force-pushed
the
label_output_workspaces
branch
from
16da6d1 to
b012691
Compare
github-actions
bot
added
Has Conflicts
|
👋 Hi, @adriazalvarez, Conflicts have been detected against the base branch. Please rebase your branch against the base branch. |
…to workspace output names in inelastic interfaces
…ent function call and not on the final call to setup elwin algo
…is held by ElwinView widget, and update tab specific suffixes on the output name widget
…ing output name widget, also create mock class for output name widget
…nged from indirect settings
…suffices restrictions are changed from settings
…ter object and call it accordingly
adriazalvarez
force-pushed
the
label_output_workspaces
branch
from
198be53 to
418eaa4
Compare
github-actions
bot
removed
the
Has Conflicts
adriazalvarez
added
Indirect/Inelastic
robertapplin
suggested changes
Oct 30, 2024
There was a problem hiding this comment.
I've tested this and its working really well - it is clear and easy to use, and produced the expected output workspace. Thanks for your hard work with this project!
I have a couple of non-code suggestions:
- I think we should add a tooltip to the "Label" line edit to explain how the provided text will be used
- The Current Output Name is sometimes cut off in the vertical direction:
qt/widgets/spectroscopy/inc/MantidQtWidgets/Spectroscopy/OutputWidget/OutputNamePresenter.h
Outdated
Show resolved
Hide resolved
qt/widgets/spectroscopy/src/OutputWidget/OutputNamePresenter.cpp
Outdated
Show resolved
Hide resolved
… for the label TextEdit to indicate how the widget operates
|
Thanks for the review Rob. |
robertapplin
suggested changes
Nov 1, 2024
qt/widgets/spectroscopy/inc/MantidQtWidgets/Spectroscopy/OutputWidget/OutputName.ui
Outdated
Show resolved
Hide resolved
robertapplin
approved these changes
Nov 5, 2024
There was a problem hiding this comment.
Thanks for the changes, this is a useful new feature which should address the need to produce output workspaces from the Elwin interface with different names so that previously processed workspaces are not overridden
Approving because the test failure is unrelated
mpatrou
pushed a commit
that referenced
this pull request
Nov 21, 2024
This is a squashed version of #37762 Add new output name widget class and gui to add custom output labels to workspace output names in inelastic interfaces Refactor elwin model to update the output workspace names in a different function call and not on the final call to setup elwin algo Use label widget to manage output name workspaces from presenter class Create function to retrieve raw pointer to output name widget, which is held by ElwinView widget, and update tab specific suffixes on the output name widget Add custom output name widget to GUI Refactor elwin model and presenter test to work with changes after using output name widget, also create mock class for output name widget Change layout of the output widget Update allowed suffices on output widget when suffix settings are changed from indirect settings Fix clang format Fix problems in tests after rebase with main Remove reference to ADS in warning label Replace function to update label name after last sprint elwin class modifications Change label abbreviation and use new signal/slot syntax Rename function for adding underscore to hopefully clearer name Reimplemenet overriden suffix function to update the label if output suffices restrictions are changed from settings Split outputname widget class into view and presenter Modify Data processor class and elwin to construct a outputname Add tests for OutputNamePresenter remove empty anonymous namespace Simplify function to find index where to insert the label explicit cast size_t to int to avoid test warning Reformat files after rebase Add model for the output name widget Initialize outputname widget with model and move it to the parent inelastic tab class Add tests for OutputNameModel refactor presenter tests and add mocks for the output name model Improve layout visual aspects and consistency accross OS. Add tooltip for the label TextEdit to indicate how the widget operates fix typo in ui
mpatrou
pushed a commit
that referenced
this pull request
Nov 22, 2024
This is a squashed version of #37762 Add new output name widget class and gui to add custom output labels to workspace output names in inelastic interfaces Refactor elwin model to update the output workspace names in a different function call and not on the final call to setup elwin algo Use label widget to manage output name workspaces from presenter class Create function to retrieve raw pointer to output name widget, which is held by ElwinView widget, and update tab specific suffixes on the output name widget Add custom output name widget to GUI Refactor elwin model and presenter test to work with changes after using output name widget, also create mock class for output name widget Change layout of the output widget Update allowed suffices on output widget when suffix settings are changed from indirect settings Fix clang format Fix problems in tests after rebase with main Remove reference to ADS in warning label Replace function to update label name after last sprint elwin class modifications Change label abbreviation and use new signal/slot syntax Rename function for adding underscore to hopefully clearer name Reimplemenet overriden suffix function to update the label if output suffices restrictions are changed from settings Split outputname widget class into view and presenter Modify Data processor class and elwin to construct a outputname Add tests for OutputNamePresenter remove empty anonymous namespace Simplify function to find index where to insert the label, rename explicit cast size_t to int to avoid test warning Reformat files after rebase Add model for the output name widget Initialize outputname widget with model and move it to the parent inelastic tab class Add tests for OutputNameModel refactor presenter tests and add mocks for the output name model Improve layout visual aspects and consistency accross OS. Add tooltip for the label TextEdit to indicate how the widget operates fix typo in ui
mpatrou
added a commit
that referenced
this pull request
Nov 25, 2024
Add tests for IqtPresenter tab #37901 Elwin allow processing ILL data #38033 Update Qt connection syntax in Inelastic Interfaces #38119 Update Moments tab instructions and add more tests #38274 Construct the model of OutputPlotOptions widget outside the Presenter class. #38289 Add identifier for output workspaces in Elwin Interface #37762 MVP for the ResNorm tab of the Inelastic Bayes Fitting interface #38357 Remove deprecated Calculate Paalman Pings tab from Inelastic Corrections #38391 Use std::pair instead of QPair in utility files #38396 Elwin Elastic line integration issue #38392 --------- Co-authored-by: adriazalvarez <adrian.diaz-alvarez@stfc.ac.uk> Co-authored-by: Applin <robert.applin@stfc.ac.uk> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of work
This PR creates a new widget that can be embedded in inelastic interfaces to add a label to the name of the output workspace. The label is typically added before the default workspace prefix( for example, _red or _sqw), unless there is not prefix on the interface, in which case is added at the end of the default output name (although the algorithm could add another prefix on the final workspace generation). When the label is changed, the widget will generate what would be the final workspace name after running the interface algorithm and check if this name is already in use in the ADS. Then updating a colored label with an appropriate warning message so that the user can know whether data will be overridden, but it does not prevent the data to be overridden if the user still runs the interface algorithm.
Fixes #37201
Further detail of work
Initially, I added the widget as a single class containing all the functionality. On a second revision, I have changed it to contain a presenter and a view to make it more organized and also to add tests.
To test:
irs26176_graphite002_redRunwidget. You will see that next to theCurrent Output Nametext, a label will update with the full expected output name for the current data file. The warning label just before this label is turned on green color with a text indicating that with the current output name, there won't be any overriding workspaces on the ads.NoLabel. Click on the edit and writelabelirs26176_graphite002_label_red_elwin_eq, still the warning label on green as this workspace name is not used on the ADS.label2, the warning label will be green indicating the output name is not overridden.irs26176_graphite002_red_label...Reviewer
Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.
Code Review
Functional Tests
Does everything look good? Mark the review as Approve. A member of
@mantidproject/gatekeeperswill take care of it.Gatekeeper
If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.