Skip to content

Fix Inelastic Symmetrise Preview crash #38051

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Sep 27, 2024
Merged

Fix Inelastic Symmetrise Preview crash #38051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
84cf1a2
Move ADS instantiation to annonymous namespace
robertapplin Sep 20, 2024
4599728
Set file problem to empty string if loading successful
robertapplin Sep 20, 2024
271c2a4
Remove m_autoLoad as member variable from DataSelector
robertapplin Sep 20, 2024
71301ea
Avoid auto loading when validating on Symmetrise tab
robertapplin Sep 20, 2024
74a8a67
Update Spectroscopy test suites
robertapplin Sep 20, 2024
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
9 changes: 5 additions & 4 deletions qt/scientific_interfaces/Inelastic/Processor/SymmetrisePresenter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ using namespace MantidQt::CustomInterfaces::InterfaceUtils;

namespace {
Mantid::Kernel::Logger g_log("SymmetrisePresenter");

auto &ads = Mantid::API::AnalysisDataService::Instance();
} // namespace

namespace MantidQt {
Expand All @@ -29,8 +31,7 @@ namespace CustomInterfaces {
SymmetrisePresenter::SymmetrisePresenter(QWidget *parent,
std::unique_ptr<MantidQt::API::IAlgorithmRunner> algorithmRunner,
ISymmetriseView *view, std::unique_ptr<ISymmetriseModel> model)
: DataProcessor(parent, std::move(algorithmRunner)), m_adsInstance(Mantid::API::AnalysisDataService::Instance()),
m_view(view), m_model(std::move(model)), m_isPreview(false) {
: DataProcessor(parent, std::move(algorithmRunner)), m_view(view), m_model(std::move(model)), m_isPreview(false) {
m_view->subscribePresenter(this);
setRunWidgetPresenter(std::make_unique<RunPresenter>(this, m_view->getRunView()));
setOutputPlotOptionsPresenter(
Expand All @@ -43,7 +44,7 @@ SymmetrisePresenter::SymmetrisePresenter(QWidget *parent,
SymmetrisePresenter::~SymmetrisePresenter() { m_propTrees["SymmPropTree"]->unsetFactoryForManager(m_dblManager); }

void SymmetrisePresenter::handleValidation(IUserInputValidator *validator) const {
validateDataIsOfType(validator, m_view->getDataSelector(), "Sample", DataType::Red);
validateDataIsOfType(validator, m_view->getDataSelector(), "Sample", DataType::Red, false, false);
}

void SymmetrisePresenter::handleRun() {
Expand All @@ -57,7 +58,7 @@ void SymmetrisePresenter::handleRun() {

// Return if no data has been loaded
auto const dataWorkspaceName = m_view->getDataName();
if (dataWorkspaceName.empty())
if (!ads.doesExist(dataWorkspaceName))
return;
// Return if E range is incorrect
if (!m_view->verifyERange(dataWorkspaceName))
Expand Down
1 change: 0 additions & 1 deletion qt/scientific_interfaces/Inelastic/Processor/SymmetrisePresenter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ class MANTIDQT_INELASTIC_DLL SymmetrisePresenter : public DataProcessor,
void setFileExtensionsByName(bool filter) override;
void setLoadHistory(bool doLoadHistory) override;

Mantid::API::AnalysisDataServiceImpl &m_adsInstance;
ISymmetriseView *m_view;
std::unique_ptr<ISymmetriseModel> m_model;
// wether batch algorunner is running preview or run buttons
Expand Down
11 changes: 2 additions & 9 deletions qt/widgets/common/inc/MantidQtWidgets/Common/DataSelector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ class EXPORT_OPT_MANTIDQT_COMMON DataSelector : public API::MantidWidget {

// These are global properties of data selector
Q_PROPERTY(bool optional READ isOptional WRITE isOptional)
Q_PROPERTY(bool autoLoad READ willAutoLoad WRITE setAutoLoad)
Q_PROPERTY(QString loadLabelText READ getLoadBtnText WRITE setLoadBtnText)

public:
Expand All @@ -73,7 +72,7 @@ class EXPORT_OPT_MANTIDQT_COMMON DataSelector : public API::MantidWidget {
/// Get the workspace name from the list of files
QString getWsNameFromFiles() const;
/// Get the currently available file or workspace name
virtual QString getCurrentDataName() const;
virtual QString getCurrentDataName(bool const autoLoad = true) const;
/// Sets which selector (file or workspace) is visible
void setSelectorIndex(int index);
/// Sets if the option to choose selector is visible
Expand All @@ -85,7 +84,7 @@ class EXPORT_OPT_MANTIDQT_COMMON DataSelector : public API::MantidWidget {
/// Get whether the workspace selector is currently being shown
bool isWorkspaceSelectorVisible() const;
/// Checks if widget is in a valid state
virtual bool isValid();
virtual bool isValid(bool const autoLoad = true);
/// Get file problem, empty string means no error.
QString getProblem() const;
/// Read settings from the given group
Expand All @@ -96,10 +95,6 @@ class EXPORT_OPT_MANTIDQT_COMMON DataSelector : public API::MantidWidget {
bool isOptional() const;
/// Sets if optional
void isOptional(bool /*optional*/);
/// Gets will auto load
bool willAutoLoad() const;
/// Sets will auto load
void setAutoLoad(bool /*load*/);
/// Check if the widget will show the load button
bool willShowLoad();
/// Set if the load button should be shown
Expand Down Expand Up @@ -376,8 +371,6 @@ private slots:
Mantid::API::AlgorithmRuntimeProps m_loadProperties;
/// Algorithm Runner used to run the load algorithm
MantidQt::API::QtAlgorithmRunner m_algRunner;
/// Flag to enable auto loading. By default this is set to true.
bool m_autoLoad;
/// Flag to show or hide the load button. By default this is set to true.
bool m_showLoad;
/// Flag if optional
Expand Down
3 changes: 2 additions & 1 deletion qt/widgets/common/inc/MantidQtWidgets/Common/MockUserInputValidator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ class MockUserInputValidator : public MantidQt::CustomInterfaces::IUserInputVali
MOCK_METHOD3(checkFieldIsValid, bool(const QString &errorMessage, QLineEdit *field, QLabel *errorLabel));
MOCK_METHOD2(checkWorkspaceSelectorIsNotEmpty, bool(const QString &name, WorkspaceSelector *workspaceSelector));
MOCK_METHOD2(checkFileFinderWidgetIsValid, bool(const QString &name, const FileFinderWidget *widget));
MOCK_METHOD3(checkDataSelectorIsValid, bool(const QString &name, DataSelector *widget, bool silent));
MOCK_METHOD4(checkDataSelectorIsValid,
bool(const QString &name, DataSelector *widget, bool const silent, bool const autoLoad));
MOCK_METHOD3(checkWorkspaceGroupIsValid, bool(QString const &groupName, QString const &inputType, bool silent));
MOCK_METHOD2(checkWorkspaceExists, bool(QString const &workspaceName, bool silent));
MOCK_METHOD2(checkValidRange, bool(QString const &name, std::pair<double, double> range));
Expand Down
6 changes: 4 additions & 2 deletions qt/widgets/common/inc/MantidQtWidgets/Common/UserInputValidator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ class DLLExport IUserInputValidator {
virtual bool checkFieldIsValid(const QString &errorMessage, QLineEdit *field, QLabel *errorLabel = nullptr) = 0;
virtual bool checkWorkspaceSelectorIsNotEmpty(const QString &name, WorkspaceSelector *workspaceSelector) = 0;
virtual bool checkFileFinderWidgetIsValid(const QString &name, const FileFinderWidget *widget) = 0;
virtual bool checkDataSelectorIsValid(const QString &name, DataSelector *widget, bool silent = false) = 0;
virtual bool checkDataSelectorIsValid(const QString &name, DataSelector *widget, bool const silent = false,
bool const autoLoad = true) = 0;
virtual bool checkWorkspaceGroupIsValid(QString const &groupName, QString const &inputType, bool silent = false) = 0;
virtual bool checkWorkspaceExists(QString const &workspaceName, bool silent = false) = 0;
virtual bool checkValidRange(const QString &name, std::pair<double, double> range) = 0;
Expand Down Expand Up @@ -76,7 +77,8 @@ class DLLExport UserInputValidator final : public IUserInputValidator {
/// Check that the given FileFinderWidget widget has valid files.
bool checkFileFinderWidgetIsValid(const QString &name, const FileFinderWidget *widget) override;
/// Check that the given DataSelector widget has valid input.
bool checkDataSelectorIsValid(const QString &name, DataSelector *widget, bool silent = false) override;
bool checkDataSelectorIsValid(const QString &name, DataSelector *widget, bool const silent = false,
bool const autoLoad = true) override;
/// Check that the given start and end range is valid.
bool checkValidRange(const QString &name, std::pair<double, double> range) override;
/// Check that the given ranges dont overlap.
Expand Down
69 changes: 21 additions & 48 deletions qt/widgets/common/src/DataSelector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,7 @@
using namespace Mantid::API;

namespace {

bool doesExistInADS(std::string const &workspaceName) {
return AnalysisDataService::Instance().doesExist(workspaceName);
}
auto &ads = AnalysisDataService::Instance();

std::string cutLastOf(const std::string &str, const std::string &delimiter) {
const auto cutIndex = str.rfind(delimiter);
Expand All @@ -50,7 +47,6 @@ std::string loadAlgName(const std::string &filePath) {
}

void makeGroup(std::string const &workspaceName) {
auto const &ads = AnalysisDataService::Instance();
if (!ads.retrieveWS<WorkspaceGroup>(workspaceName)) {
const auto groupAlg = AlgorithmManager::Instance().createUnmanaged("GroupWorkspaces");
groupAlg->initialize();
Expand All @@ -65,8 +61,7 @@ void makeGroup(std::string const &workspaceName) {
namespace MantidQt::MantidWidgets {

DataSelector::DataSelector(QWidget *parent)
: API::MantidWidget(parent), m_loadProperties(), m_algRunner(), m_autoLoad(true), m_showLoad(true),
m_alwaysLoadAsGroup(false) {
: API::MantidWidget(parent), m_loadProperties(), m_algRunner(), m_showLoad(true), m_alwaysLoadAsGroup(false) {
m_uiForm.setupUi(this);
connect(m_uiForm.cbInputType, SIGNAL(currentIndexChanged(int)), this, SLOT(handleViewChanged(int)));
connect(m_uiForm.pbLoadFile, SIGNAL(clicked()), this, SIGNAL(loadClicked()));
Expand Down Expand Up @@ -110,14 +105,8 @@ void DataSelector::handleFileInput() {
return;
}

// attempt to load the file
if (m_autoLoad) {
emit filesAutoLoaded();
autoLoadFile(filename);
} else {
// files were found
emit filesFound();
}
emit filesAutoLoaded();
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if I'm not following the code and PR comments very well, but I just wanted to double check that this change is what's intended - it looks like the code here previously used to check whether auto load was enabled/requested, but now it looks like it will always perform the auto-load regardless?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for checking. I found out that the m_autoLoad option is always set to true, and so I decided to do a small bit of simplification for this class by removing the option entirely. The only place where we might not want to do auto-loading is now in the isValid function, and I have provided an option which can be passed to this function directly

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, great, thanks for explaining, that all sounds good then!

autoLoadFile(filename);
}

/**
Expand Down Expand Up @@ -152,37 +141,36 @@ bool DataSelector::isWorkspaceSelectorVisible() const { return !isFileSelectorVi
*
* Checks using the relvant widgets isValid method depending
* on what view is currently being shown
*
* @param autoLoad :: Whether to automatically load the data if it is not found in the ADS.
* @return :: If the data selector is valid
*/
bool DataSelector::isValid() {
bool DataSelector::isValid(bool const autoLoad) {
bool isValid = false;

if (isFileSelectorVisible()) {
isValid = m_uiForm.rfFileInput->isValid();

// check to make sure the user hasn't deleted the auto-loaded file
// since choosing it.
if (isValid && m_autoLoad) {
if (isValid && autoLoad) {
auto const wsName = getCurrentDataName().toStdString();

isValid = !wsName.empty();
if (isValid && !doesExistInADS(wsName)) {
if (isValid && !ads.doesExist(wsName)) {
// attempt to reload if we can
// don't use algorithm runner because we need to know instantly.
auto const filepath = m_uiForm.rfFileInput->getUserInput().toString().toStdString();
if (!filepath.empty())
executeLoadAlgorithm(filepath, wsName);

isValid = doesExistInADS(wsName);
isValid = ads.doesExist(wsName);

if (!isValid) {
m_uiForm.rfFileInput->setFileProblem("The specified workspace is "
"missing from the analysis data "
"service");
}
} else {
auto &ads = AnalysisDataService::Instance();
if (!ads.doesExist(wsName)) {
return isValid;
}
Expand Down Expand Up @@ -268,16 +256,15 @@ void DataSelector::setLoadProperty(std::string const &propertyName, bool const v
* @param error :: Whether loading completed without error
*/
void DataSelector::handleAutoLoadComplete(bool error) {
if (!error) {
if (m_alwaysLoadAsGroup) {
makeGroup(getWsNameFromFiles().toStdString());
}
m_uiForm.rfFileInput->setFileProblem(error ? "Could not load file. See log for details." : "");

// emit that we got a valid workspace/file to work with
emit dataReady(getWsNameFromFiles());
} else {
m_uiForm.rfFileInput->setFileProblem("Could not load file. See log for details.");
if (error) {
return;
}
if (m_alwaysLoadAsGroup) {
makeGroup(getWsNameFromFiles().toStdString());
}
emit dataReady(getWsNameFromFiles());
}

/**
Expand Down Expand Up @@ -350,10 +337,10 @@ QString DataSelector::getWsNameFromFiles() const {
* If multiple files are allowed, and auto-loading is off, it will return the
* full user input.
* If there is no valid input the method returns an empty string.
*
* @param autoLoad :: Whether or not autoload is turned on.
* @return The name of the current data item
*/
QString DataSelector::getCurrentDataName() const {
QString DataSelector::getCurrentDataName(bool const autoLoad) const {
QString filename("");

int index = m_uiForm.stackedDataSelect->currentIndex();
Expand All @@ -362,7 +349,7 @@ QString DataSelector::getCurrentDataName() const {
case 0:
// the file selector is visible
if (m_uiForm.rfFileInput->isValid()) {
if (m_uiForm.rfFileInput->allowMultipleFiles() && !m_autoLoad) {
if (m_uiForm.rfFileInput->allowMultipleFiles() && !autoLoad) {
// if multiple files are allowed, auto-loading is not on, return the
// full user input
filename = getFullFilePath();
Expand All @@ -380,20 +367,6 @@ QString DataSelector::getCurrentDataName() const {
return filename;
}

/**
* Gets whether the widget will attempt to auto load files
*
* @return Whether the widget will auto load
*/
bool DataSelector::willAutoLoad() const { return m_autoLoad; }

/**
* Sets whether the widget will attempt to auto load files.
*
* @param load :: Whether the widget will auto load
*/
void DataSelector::setAutoLoad(bool load) { m_autoLoad = load; }

/**
* Gets the text displayed on the load button
*
Expand Down Expand Up @@ -457,7 +430,7 @@ void DataSelector::dropEvent(QDropEvent *de) {

auto const dragData = mimeData->text().toStdString();

if (de->mimeData() && doesExistInADS(dragData)) {
if (de->mimeData() && ads.doesExist(dragData)) {
m_uiForm.wsWorkspaceInput->dropEvent(de);
if (de->dropAction() == before_action) {
setWorkspaceSelectorIndex(mimeData->text());
Expand All @@ -473,7 +446,7 @@ void DataSelector::dropEvent(QDropEvent *de) {
}

auto const filepath = m_uiForm.rfFileInput->getText().toStdString();
if (de->mimeData() && !doesExistInADS(dragData) && !filepath.empty()) {
if (de->mimeData() && !ads.doesExist(dragData) && !filepath.empty()) {
auto const file = extractLastOf(filepath, "/");
if (fileFound(file)) {
auto const workspaceName = cutLastOf(file, ".");
Expand Down
6 changes: 4 additions & 2 deletions qt/widgets/common/src/UserInputValidator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,10 +139,12 @@ bool UserInputValidator::checkFileFinderWidgetIsValid(const QString &name, const
* @param name :: the "name" of the widget so as to be recognised by the user.
* @param widget :: the widget to check
* @param silent True if an error should not be added to the validator.
* @param autoLoad True if the data should be reloaded if it is not in the ADS.
* @returns True if the input was valid
*/
bool UserInputValidator::checkDataSelectorIsValid(const QString &name, DataSelector *widget, bool silent) {
if (!widget->isValid()) {
bool UserInputValidator::checkDataSelectorIsValid(const QString &name, DataSelector *widget, bool const silent,
bool const autoLoad) {
if (!widget->isValid(autoLoad)) {
addErrorMessage(name.toStdString() + " error: " + widget->getProblem().toStdString(), silent);
return false;
}
Expand Down
17 changes: 11 additions & 6 deletions qt/widgets/spectroscopy/inc/MantidQtWidgets/Spectroscopy/DataValidationHelper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,27 +16,32 @@ enum DataType { Red, Sqw, Calib, Corrections };
MANTID_SPECTROSCOPY_DLL bool validateDataIsOneOf(MantidQt::CustomInterfaces::IUserInputValidator *uiv,
MantidQt::MantidWidgets::DataSelector *dataSelector,
std::string const &inputType, DataType const &primaryType,
std::vector<DataType> const &otherTypes, bool silent = false);
std::vector<DataType> const &otherTypes, bool const silent = false,
bool const autoLoad = true);

MANTID_SPECTROSCOPY_DLL bool validateDataIsOfType(MantidQt::CustomInterfaces::IUserInputValidator *uiv,
MantidQt::MantidWidgets::DataSelector *dataSelector,
std::string const &inputType, DataType const &type,
bool silent = false);
bool const silent = false, bool const autoLoad = true);

MANTID_SPECTROSCOPY_DLL bool validateDataIsAReducedFile(MantidQt::CustomInterfaces::IUserInputValidator *uiv,
MantidQt::MantidWidgets::DataSelector *dataSelector,
std::string const &inputType, bool silent = false);
std::string const &inputType, bool const silent = false,
bool const autoLoad = true);

MANTID_SPECTROSCOPY_DLL bool validateDataIsASqwFile(MantidQt::CustomInterfaces::IUserInputValidator *uiv,
MantidQt::MantidWidgets::DataSelector *dataSelector,
std::string const &inputType, bool silent = false);
std::string const &inputType, bool const silent = false,
bool const autoLoad = true);

MANTID_SPECTROSCOPY_DLL bool validateDataIsACalibrationFile(MantidQt::CustomInterfaces::IUserInputValidator *uiv,
MantidQt::MantidWidgets::DataSelector *dataSelector,
std::string const &inputType, bool silent = false);
std::string const &inputType, bool const silent = false,
bool const autoLoad = true);

MANTID_SPECTROSCOPY_DLL bool validateDataIsACorrectionsFile(MantidQt::CustomInterfaces::IUserInputValidator *uiv,
MantidQt::MantidWidgets::DataSelector *dataSelector,
std::string const &inputType, bool silent = false);
std::string const &inputType, bool const silent = false,
bool const autoLoad = true);

} // namespace DataValidationHelper
Loading
Loading