Reviewing merge request #57: QTemporaryFile: use QCoreApplication::applicationName() as base filename
Ending up with thousands of qt_temp.* files in /tmp doesn't really help finding which application is not deleting its temporary files.
So this merge request changes QTemporaryFile in order to use the qcoreapp appname (if available) as the default base filename.
With this change, KDE's KTemporaryFile can be dropped in favour of QTemporaryFile (losing a bit of convenience API while doing that, such as setPrefix/setSuffix, but losing no features).
- Requested by:
David Faure - Status: Merged
- Source repository: dfaures-qtbase:temporaryfiles
- Target repository: qtbase:master
- Created at: September 18th, 2011 20:24
Commits that would be merged:
Comments
Pushed new version 1
+ if (!QCoreApplication::applicationName().isEmpty())
you have an indentation problem here …
Oswald Buddenhagen |
2011-09-19 08:56:08 UTC
on src/corelib/io/qtemporaryfile.cpp
+ determined from QCoreApplication::applicationName (otherwise qt_temp) and will
please put parentheses after applicationName.
Oswald Buddenhagen |
2011-09-19 08:57:18 UTC
on src/corelib/io/qtemporaryfile.cpp
+ determined from QCoreApplication::applicationName (otherwise qt_temp) and will
you removed the \c from qt_temp …
Oswald Buddenhagen |
2011-09-19 08:57:30 UTC
on src/corelib/io/qtemporaryfile.cpp
+ templateName = QDir::tempPath() + QLatin1Char('/');
With QStringBuilder enabled, it’s actually better to have one long line of +, as that pre-calculates the size.
thiago-personal |
2011-09-19 11:48:10 UTC
on src/corelib/io/qtemporaryfile.cpp
+ "qcoreappname.XXXXXX". The file is stored in the system's temporary directory.
You didn’t change the template in setDefaultTemplateName(), so there’s no need to change the comment.
thiago-personal |
2011-09-19 11:49:46 UTC
on src/corelib/io/qtemporaryfile.cpp
+ void QTemporaryFilePrivate::setDefaultTemplateName()
I think it’s strange to have a setter without a parameter. Maybe a getter would be more clear. Something like:
inline QString QTemporaryFilePrivate::defaultTemplateName() const
{
if (QCoreApplication::applicationName().isEmpty())
return QDir::tempPath() + QLatin1String("/qt_temp.XXXXXX");
return QDir::tempPath() + QLatin1Char('/') +
QCoreApplication::applicationName() + QLatin1String(".XXXXXX");
}
Adriano Tinoco d'Oliveira Rezende |
2011-09-19 16:08:45 UTC
on src/corelib/io/qtemporaryfile.cpp
Thanks for the comments, all fixed, except for the last comment by thiago: I -did- change the default template, since it now uses the qcoreapplication name (if set). So I think the documentation change (qcoreappname.XXXXXX) makes sense.
Pushed new version 2
Ah, I get it. “qcoreappname” wasn’t meant to be understood literally. That’s how I had read it though.
Good enough as it is, but I recommend a simple fix to the doc text to clarify.
Pushed new version 3
Merged as c7a0fd0950179ed9ae0aa1e5b02df9b6a383c709 and e316deffef68287b03f6eed6e98b5bfcd984280d
Add a new comment:
Login or create an account to post a comment