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

COMP: Fix various deprecation warning C4996 #915

Merged
merged 13 commits into from
Jun 22, 2020

Conversation

jamesobutler
Copy link
Contributor

This PR aims to fix various deprecation warnings.

So far I've updated based on a per widget basis, so some of the same type of deprecations I haven't fixed across all widgets yet. I could continue putting all warning fixes into one commit or split based on the type of deprecation being resolved.

@pieper
Copy link
Member

pieper commented May 30, 2020

Thanks for doing this 👍

I like the idea of a commit per deprecation type but not if it's a lot of extra work.

@jamesobutler
Copy link
Contributor Author

Ok probably will do then. Also just noticed that the CI uses the slicer/buildenv-qt5-centos7 image which was recently updated with Qt 5.15 so I can also see the additional warnings in the CI output to be fixed.

warning C4996: 'QString::SkipEmptyParts': was declared deprecated
warning C4996: 'qVariantFromValue': Use QVariant::fromValue() instead.
warning C4996: 'QTextStreamFunctions::endl': Use Qt::endl
warning C4996: 'QButtonGroup::buttonToggled': Use QButtonGroup::idToggled(int, bool) instead
warning C4996: 'QFontMetrics::width': Use QFontMetrics::horizontalAdvance
warning C4996: 'QComboBox::AdjustToMinimumContentsLength': Use AdjustToContents or AdjustToContentsOnFirstShow
warning C4996: 'QList<int>::swap': Use QList<T>::swapItemsAt()
warning C4996: 'QDateTime::QDateTime': Use QDate::startOfDay()
warning C4996: 'QFileDialog::DirectoryOnly': Use setOption(ShowDirsOnly, true) instead
warning C4996: 'qSort': Use std::sort
warning C4996: 'QDirModel::QDirModel': Use QFileSystemModel
@jamesobutler
Copy link
Contributor Author

@pieper I've split things up. I've primarily tried to address the warnings that show as part of the Slicer build. There might still be some deprecations in areas such as the CommandLineModuleExplorer directory, but that also doesn't appear to be maintained much so wasn't going to put additional effort into that right now unless you think it is necessary.

warning C4996: 'QString::null': use QString()
@pieper
Copy link
Member

pieper commented May 31, 2020

Looks great to me. I'd say merge, but let's give @jcfr a chance to have a look too.

warning C4996: 'QString::KeepEmptyParts': was declared deprecated
QTextStream(stderr, QIODevice::WriteOnly)
<< "Line " << line << " - Problem with function " << function << "\n"
<< "\tActual value : '" << actualName << "' = " << actual.join(" ") << " \n"
<< "\tExpected value : '" << expectedName << "' = " << expected.join(" ") << Qt::endl;
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if adding CTK_QT_ENDL or CTK_ENDL to https://github.com/commontk/CTK/blob/master/Libs/Core/ctkCompatibility_p.h would be worth it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure that could be attempted. This to cut down on the number of duplicated new lines in the diff for this commit?

Copy link
Member

Choose a reason for hiding this comment

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

cut down on the number of duplicated new lines in the diff for this commit

The idea was to keep the content of message consistent between all code paths.

That said, it is true the change is localized in this file. This could be revisited if this pattern is common.

@@ -104,16 +109,6 @@ QSize ctkComboBoxPrivate::recomputeSizeHint(QSize &sh) const
}
}
break;
case QComboBox::AdjustToMinimumContentsLength:
Copy link
Member

Choose a reason for hiding this comment

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

Is it intentional to remove this case ?

Copy link
Member

Choose a reason for hiding this comment

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

Would this break something for folks using Qt4 or older version of Qt5 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it was intentional. Pretty much AdjustToMinimumContentsLength is deprecated in Qt 5.15, but before then and all the way back to at least Qt 4.8.7, this SizeAdjustPolicy has said in the notes to just Use AdjustToContents or AdjustToContentsOnFirstShow instead. (See here). Seems like something deprecated for a long time, just not specifically marked as deprecated. The AdjustToContents case a few lines above specifically didn't do this set of code, so I wasn't going to copy it into there.

@jcfr jcfr merged commit 9cff964 into commontk:master Jun 22, 2020
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