From ba32f0e42f8681e7fe80c4a56e4f4d13b21789ea Mon Sep 17 00:00:00 2001 From: Johannes Zarl-Zierl Date: Sun, 26 Jul 2020 01:04:58 +0200 Subject: [PATCH] Review TODO/FIXME comments. --- AnnotationDialog/Dialog.cpp | 7 +++++-- DB/MemberMap.cpp | 2 +- DB/NewImageFinder.cpp | 2 +- ImageManager/AsyncLoader.cpp | 7 +++---- Map/GeoCluster.cpp | 1 - ThumbnailView/ThumbnailModel.cpp | 5 +++-- XMLDB/Database.cpp | 2 +- 7 files changed, 14 insertions(+), 12 deletions(-) diff --git a/AnnotationDialog/Dialog.cpp b/AnnotationDialog/Dialog.cpp index 5940de80..eeb2ad83 100644 --- a/AnnotationDialog/Dialog.cpp +++ b/AnnotationDialog/Dialog.cpp @@ -467,7 +467,7 @@ void AnnotationDialog::Dialog::slotCopyPrevious() if (m_current < 1) return; - // FIXME: it would be better to compute the "previous image" in a better way, but let's stick with this for now... + // (jzarl 2020-07-26): defining the "previous image" as the one before this is the behaviour of the least surprise: DB::ImageInfo &old_info = m_editList[m_current - 1]; m_positionableTagCandidates.clear(); @@ -1109,7 +1109,10 @@ void AnnotationDialog::Dialog::slotDeleteImage() if (m_setup == SearchMode) return; - if (m_setup == InputMultiImageConfigMode) //TODO: probably delete here should mean remove from selection + // should delete mean "remove from selection" or "delete"? + // is the user even aware that the dialog is in multi image mode? + // IMO (jzarl) this is too ambiguous to do anything other than bail out: + if (m_setup == InputMultiImageConfigMode) return; DB::ImageInfoPtr info = m_origList[m_current]; diff --git a/DB/MemberMap.cpp b/DB/MemberMap.cpp index 42db0b9e..e87377ca 100644 --- a/DB/MemberMap.cpp +++ b/DB/MemberMap.cpp @@ -142,7 +142,7 @@ QMap MemberMap::groupMap(const QString &category) const */ QStringList MemberMap::calculateClosure(QMap &resultSoFar, const QString &category, const QString &group) const { - resultSoFar[group] = StringSet(); // Prevent against cykles. + resultSoFar[group] = StringSet(); // Prevent against cycles. StringSet members = m_members[category][group]; StringSet result = members; for (StringSet::const_iterator it = members.begin(); it != members.end(); ++it) { diff --git a/DB/NewImageFinder.cpp b/DB/NewImageFinder.cpp index 9b75f780..337038e3 100644 --- a/DB/NewImageFinder.cpp +++ b/DB/NewImageFinder.cpp @@ -393,7 +393,7 @@ bool NewImageFinder::findImages() QElapsedTimer timer; timer.start(); - // TODO: maybe the databas interface should allow to query if it + // TODO: maybe the database interface should allow to query if it // knows about an image ? Here we've to iterate through all of them and it // might be more efficient do do this in the database without fetching the // whole info. diff --git a/ImageManager/AsyncLoader.cpp b/ImageManager/AsyncLoader.cpp index 5b68a1ff..0b3ecf75 100644 --- a/ImageManager/AsyncLoader.cpp +++ b/ImageManager/AsyncLoader.cpp @@ -64,10 +64,9 @@ void ImageManager::AsyncLoader::init() // as that'd mean that a dual-core box would only have one core decoding images, which would be // suboptimal. // In case of only one core in the computer, use one core for thumbnail generation - // TODO(isilmendil): It seems that many people have their images on NFS-mounts. - // Should we somehow detect this and allocate less threads there? - // rlk 20180515: IMO no; if anything, we need more threads to hide - // the latency of NFS. + // jzarl: It seems that many people have their images on NFS-mounts. + // Should we somehow detect this and allocate less threads there? + // rlk 20180515: IMO no; if anything, we need more threads to hide the latency of NFS. int desiredThreads = Settings::SettingsData::instance()->getThumbnailBuilderThreadCount(); if (desiredThreads == 0) { desiredThreads = qMax(1, qMin(16, QThread::idealThreadCount() - 1)); diff --git a/Map/GeoCluster.cpp b/Map/GeoCluster.cpp index 7988a270..e2806540 100644 --- a/Map/GeoCluster.cpp +++ b/Map/GeoCluster.cpp @@ -102,7 +102,6 @@ Marble::GeoDataLatLonAltBox Map::GeoCluster::boundingRegion() const Marble::GeoDataCoordinates Map::GeoCluster::center() const { - // TODO(jzarl): check how this compares to e.g. the center of all coordinates instead: return boundingRegion().center(); } diff --git a/ThumbnailView/ThumbnailModel.cpp b/ThumbnailView/ThumbnailModel.cpp index af2ecf36..6a3cfae9 100644 --- a/ThumbnailView/ThumbnailModel.cpp +++ b/ThumbnailView/ThumbnailModel.cpp @@ -167,7 +167,6 @@ void ThumbnailView::ThumbnailModel::setImageList(const DB::FileNameList &items) preloadThumbnails(); } -// TODO(hzeller) figure out if this should return the m_imageList or m_displayList. DB::FileNameList ThumbnailView::ThumbnailModel::imageList(Order order) const { if (order == SortedOrder && m_sortDirection == NewestFirst) @@ -319,8 +318,10 @@ void ThumbnailView::ThumbnailModel::pixmapLoaded(ImageManager::ImageRequest *req // The delegate now fetches the newly loaded image from the cache. DB::ImageInfoPtr imageInfo = DB::ImageDB::instance()->info(fileName); - // TODO(hzeller): figure out, why the size is set here. We do an implicit + // (hzeller): figure out, why the size is set here. We do an implicit // write here to the database. + // (jzarl 2020-07-25): when loading a fullsize pixmap, we get the size "for free"; + // calculating it separately would cost us more than writing to the database from here. if (fullSize.isValid() && imageInfo) { imageInfo->setSize(fullSize); } diff --git a/XMLDB/Database.cpp b/XMLDB/Database.cpp index 61a09862..3ee3564d 100644 --- a/XMLDB/Database.cpp +++ b/XMLDB/Database.cpp @@ -447,7 +447,7 @@ DB::ImageInfoList XMLDB::Database::takeImagesFromSelection(const DB::FileNameLis if (selection.isEmpty()) return result; - // iterate over all images (expensive!!) TODO: improve? + // iterate over all images (expensive!!) for (DB::ImageInfoListIterator it = m_images.begin(); it != m_images.end(); /**/) { const DB::FileName imagefile = (*it)->fileName(); DB::FileNameList::const_iterator si = selection.begin(); -- GitLab