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: Additional where conditions in DICOM table view; Safer SQL query #955

Conversation

cpinter
Copy link
Member

@cpinter cpinter commented Mar 15, 2021

In addition to filtering for field values, now it is possible to define where conditions that use the operators 'greater', 'less', 'greater or equal', and 'less or equal'.
For example addSqlGreaterEqualWhereCondition("Series.SeriesDate", "2020-01-01").

The SQL query is now assembled using a QSqlQuery and its value binding feature instead of setting the query directly from string, thus making it safer.

For consistency, alias functions have been added for the old where condition functions while keeping those for backwards compatibility (i.e. addSqlWhereCondition -> addSqlEqualWhereCondition, removeSqlWhereCondition -> removeSqlEqualWhereCondition)

Functions have been added to remove all types of where conditions individually, as well as one clearSqlWhereConditions function to clear all where conditions.

@cpinter
Copy link
Member Author

cpinter commented Mar 15, 2021

For the record the first approach for this change was in this PR: #954

Copy link
Member

@jcfr jcfr left a comment

Choose a reason for hiding this comment

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

Beside of few nitpicks and stylistic comments, looks good.

I am also wondering if one of the existing tests could be updated to test this.

Libs/DICOM/Widgets/ctkDICOMTableView.cpp Outdated Show resolved Hide resolved
Libs/DICOM/Widgets/ctkDICOMTableView.cpp Outdated Show resolved Hide resolved
Libs/DICOM/Widgets/ctkDICOMTableView.cpp Outdated Show resolved Hide resolved
Libs/DICOM/Widgets/ctkDICOMTableView.cpp Outdated Show resolved Hide resolved
@@ -615,6 +675,92 @@ void ctkDICOMTableView::addSqlWhereCondition(const QString column, const QString
d->sqlWhereConditions.insert(column, values);
}

//------------------------------------------------------------------------------
void ctkDICOMTableView::removeSqlWhereCondition(const QString column)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void ctkDICOMTableView::removeSqlWhereCondition(const QString column)
void ctkDICOMTableView::removeSqlWhereCondition(const QString& column)

Copy link
Member Author

Choose a reason for hiding this comment

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

Just out of curiosity: why is it important to pass a column name (which is usually not longer than a dozen characters) by pointer not value? I'll change these anyway just wondering...

Copy link
Member

Choose a reason for hiding this comment

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

Matter of consistency

Libs/DICOM/Widgets/ctkDICOMTableView.h Outdated Show resolved Hide resolved
Libs/DICOM/Widgets/ctkDICOMTableView.h Outdated Show resolved Hide resolved
Libs/DICOM/Widgets/ctkDICOMTableView.h Outdated Show resolved Hide resolved
Libs/DICOM/Widgets/ctkDICOMTableView.h Outdated Show resolved Hide resolved
Libs/DICOM/Widgets/ctkDICOMTableView.cpp Outdated Show resolved Hide resolved
In addition to filtering for field values, now it is possible to define where conditions that use the operators 'greater', 'less', 'greater or equal', and 'less or equal'.
For example addSqlGreaterEqualWhereCondition("Series.SeriesDate", "2020-01-01").

The SQL query is now assembled using a QSqlQuery and its value binding feature instead of setting the query directly from string, thus making it safer.

For consistency, alias functions have been added for the old where condition functions while keeping those for backwards compatibility (i.e. addSqlWhereCondition -> addSqlEqualWhereCondition, removeSqlWhereCondition -> removeSqlEqualWhereCondition)

Functions have been added to remove all types of where conditions individually, as well as one clearSqlWhereConditions function to clear all where conditions.
@cpinter cpinter force-pushed the dicom-table-additional-where-conditions branch from 29cab27 to afa1ea9 Compare March 16, 2021 08:49
@cpinter
Copy link
Member Author

cpinter commented Mar 16, 2021

I fixed all the requested things. Can it be merged? @jcfr @lassoan
Thanks

Copy link
Member

@lassoan lassoan left a comment

Choose a reason for hiding this comment

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

Looks great, thank you.

@lassoan lassoan merged commit edc3f53 into commontk:master Mar 16, 2021
@jcfr
Copy link
Member

jcfr commented Mar 16, 2021

Thanks @cpinter and @lassoan

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

Successfully merging this pull request may close these issues.

3 participants