diff --git a/autotests/kurlcomboboxtest.h b/autotests/kurlcomboboxtest.h --- a/autotests/kurlcomboboxtest.h +++ b/autotests/kurlcomboboxtest.h @@ -29,6 +29,8 @@ private Q_SLOTS: void testTextForItem(); void testTextForItem_data(); + void testSetUrlMultipleTimes(); + void testRemoveUrl(); }; #endif //KURLCOMBOBOXTEST_H diff --git a/autotests/kurlcomboboxtest.cpp b/autotests/kurlcomboboxtest.cpp --- a/autotests/kurlcomboboxtest.cpp +++ b/autotests/kurlcomboboxtest.cpp @@ -45,3 +45,68 @@ QCOMPARE(combo.itemText(0), expectedText); } + +void KUrlComboBoxTest::testSetUrlMultipleTimes() +{ + KUrlComboBox combo(KUrlComboBox::Directories); + combo.setUrl(QUrl("http://kde.org")); + combo.setUrl(QUrl("http://www.kde.org")); +} + +void KUrlComboBoxTest::testRemoveUrl() +{ + KUrlComboBox combo(KUrlComboBox::Both); + combo.addDefaultUrl(QUrl("http://kde.org")); + combo.addDefaultUrl(QUrl("http://www.kde.org")); + + QStringList urls{"http://foo.org", "http://bar.org"}; + combo.setUrls(urls); + + QCOMPARE(combo.urls(), urls); + QCOMPARE(combo.count(), 4); + QCOMPARE(combo.itemText(0), "http://kde.org"); + QCOMPARE(combo.itemText(1), "http://www.kde.org"); + QCOMPARE(combo.itemText(2), "http://foo.org"); + QCOMPARE(combo.itemText(3), "http://bar.org"); + + // Remove a url + combo.removeUrl(QUrl("http://foo.org")); + QCOMPARE(combo.count(), 3); + QCOMPARE(combo.urls(), QStringList{"http://bar.org"}); + QCOMPARE(combo.itemText(0), "http://kde.org"); + QCOMPARE(combo.itemText(1), "http://www.kde.org"); + QCOMPARE(combo.itemText(2), "http://bar.org"); + + // Removing a default url with checkDefaultURLs=true removes the url + combo.removeUrl(QUrl("http://kde.org")); + QCOMPARE(combo.count(), 2); + QCOMPARE(combo.urls(), QStringList{"http://bar.org"}); + QCOMPARE(combo.itemText(0), "http://www.kde.org"); + QCOMPARE(combo.itemText(1), "http://bar.org"); + + // Removing a default url with checkDefaultURLs=false does not remove the url + combo.removeUrl(QUrl("http://www.kde.org"), false); + QCOMPARE(combo.count(), 2); + QCOMPARE(combo.urls(), QStringList{"http://bar.org"}); + QCOMPARE(combo.itemText(0), "http://www.kde.org"); + QCOMPARE(combo.itemText(1), "http://bar.org"); + + // Removing a non-existing url is a no-op + combo.removeUrl(QUrl("http://www.foo.org")); + QCOMPARE(combo.count(), 2); + QCOMPARE(combo.urls(), QStringList{"http://bar.org"}); + QCOMPARE(combo.itemText(0), "http://www.kde.org"); + QCOMPARE(combo.itemText(1), "http://bar.org"); + + // Remove the last user provided url + combo.removeUrl(QUrl("http://bar.org")); + QCOMPARE(combo.count(), 1); + QCOMPARE(combo.urls(), QStringList{}); + QCOMPARE(combo.itemText(0), "http://www.kde.org"); + + // Remove the last url + combo.removeUrl(QUrl("http://www.kde.org")); + QCOMPARE(combo.count(), 0); + QCOMPARE(combo.urls(), QStringList{}); + QCOMPARE(combo.itemText(0), ""); +} diff --git a/src/widgets/kurlcombobox.cpp b/src/widgets/kurlcombobox.cpp --- a/src/widgets/kurlcombobox.cpp +++ b/src/widgets/kurlcombobox.cpp @@ -29,20 +29,18 @@ #include #include +#include +#include +#include + class KUrlComboBoxPrivate { public: KUrlComboBoxPrivate(KUrlComboBox *parent) : m_parent(parent), dirIcon(QStringLiteral("folder")) {} - ~KUrlComboBoxPrivate() - { - qDeleteAll(itemList); - qDeleteAll(defaultList); - } - struct KUrlComboItem { KUrlComboItem(const QUrl &url, const QIcon &icon, const QString &text = QString()) : url(url), icon(icon), text(text) {} @@ -66,13 +64,15 @@ KUrlComboBox::Mode myMode; QPoint m_dragPoint; - QList itemList; - QList defaultList; + using KUrlComboItemList = std::vector>; + KUrlComboItemList itemList; + KUrlComboItemList defaultList; QMap itemMapper; QIcon opendirIcon; }; + QString KUrlComboBoxPrivate::textForItem(const KUrlComboItem *item) const { if (!item->text.isEmpty()) { @@ -135,7 +135,7 @@ // qDebug() << "::urls()"; QStringList list; QString url; - for (int i = d->defaultList.count(); i < count(); i++) { + for (int i = static_cast(d->defaultList.size()); i < count(); i++) { url = itemText(i); if (!url.isEmpty()) { if (QDir::isAbsolutePath(url)) @@ -156,18 +156,16 @@ void KUrlComboBox::addDefaultUrl(const QUrl &url, const QIcon &icon, const QString &text) { - d->defaultList.append(new KUrlComboBoxPrivate::KUrlComboItem(url, icon, text)); + d->defaultList.push_back(std::unique_ptr(new KUrlComboBoxPrivate::KUrlComboItem(url, icon, text))); } void KUrlComboBox::setDefaults() { clear(); d->itemMapper.clear(); - const KUrlComboBoxPrivate::KUrlComboItem *item; - for (int id = 0; id < d->defaultList.count(); id++) { - item = d->defaultList.at(id); - d->insertUrlItem(item); + for (const auto& item : d->defaultList) { + d->insertUrlItem(item.get()); } } @@ -179,7 +177,6 @@ void KUrlComboBox::setUrls(const QStringList &_urls, OverLoadResolving remove) { setDefaults(); - qDeleteAll(d->itemList); d->itemList.clear(); d->urlAdded = false; @@ -201,7 +198,7 @@ // limit to myMaximum items /* Note: overload is an (old) C++ keyword, some compilers (KCC) choke on that, so call it Overload (capital 'O'). (matz) */ - int Overload = urls.count() - d->myMaximum + d->defaultList.count(); + int Overload = urls.count() - d->myMaximum + static_cast(d->defaultList.size()); while (Overload > 0) { if (remove == RemoveBottom) { if (!urls.isEmpty()) { @@ -217,8 +214,6 @@ it = urls.constBegin(); - KUrlComboBoxPrivate::KUrlComboItem *item = nullptr; - while (it != urls.constEnd()) { if ((*it).isEmpty()) { ++it; @@ -237,9 +232,9 @@ continue; } - item = new KUrlComboBoxPrivate::KUrlComboItem(u, d->getIcon(u)); - d->insertUrlItem(item); - d->itemList.append(item); + std::unique_ptr item(new KUrlComboBoxPrivate::KUrlComboItem(u, d->getIcon(u))); + d->insertUrlItem(item.get()); + d->itemList.push_back(std::move(item)); ++it; } } @@ -253,7 +248,7 @@ bool blocked = blockSignals(true); // check for duplicates - QMap::ConstIterator mit = d->itemMapper.constBegin(); + auto mit = d->itemMapper.constBegin(); QString urlToInsert = url.toString(QUrl::StripTrailingSlash); while (mit != d->itemMapper.constEnd()) { Q_ASSERT(mit.value()); @@ -275,48 +270,48 @@ // first remove the old item if (d->urlAdded) { - Q_ASSERT(!d->itemList.isEmpty()); - d->itemList.removeLast(); + Q_ASSERT(!d->itemList.empty()); + d->itemList.pop_back(); d->urlAdded = false; } setDefaults(); - int offset = qMax(0, d->itemList.count() - d->myMaximum + d->defaultList.count()); - for (int i = offset; i < d->itemList.count(); i++) { - d->insertUrlItem(d->itemList[i]); + KUrlComboBoxPrivate::KUrlComboItemList::size_type offset = qMax(KUrlComboBoxPrivate::KUrlComboItemList::size_type(0), d->itemList.size() - d->myMaximum + d->defaultList.size()); + for (auto i = offset; i < d->itemList.size(); i++) { + d->insertUrlItem(d->itemList.at(i).get()); } - KUrlComboBoxPrivate::KUrlComboItem *item = new KUrlComboBoxPrivate::KUrlComboItem(url, d->getIcon(url)); + std::unique_ptr item(new KUrlComboBoxPrivate::KUrlComboItem(url, d->getIcon(url))); const int id = count(); - const QString text = d->textForItem(item); + const QString text = d->textForItem(item.get()); if (d->myMode == Directories) { KComboBox::insertItem(id, d->opendirIcon, text); } else { KComboBox::insertItem(id, item->icon, text); } - d->itemMapper.insert(id, item); - d->itemList.append(item); + d->itemMapper.insert(id, item.get()); + d->itemList.push_back(std::move(item)); setCurrentIndex(id); - Q_ASSERT(!d->itemList.isEmpty()); + Q_ASSERT(!d->itemList.empty()); d->urlAdded = true; blockSignals(blocked); } void KUrlComboBoxPrivate::_k_slotActivated(int index) { - const KUrlComboItem *item = itemMapper.value(index); + auto item = itemMapper.value(index); if (item) { m_parent->setUrl(item->url); emit m_parent->urlActivated(item->url); } } -void KUrlComboBoxPrivate::insertUrlItem(const KUrlComboBoxPrivate::KUrlComboItem *item) +void KUrlComboBoxPrivate::insertUrlItem(const KUrlComboItem *item) { Q_ASSERT(item); @@ -335,9 +330,9 @@ setDefaults(); - int offset = qMax(0, d->itemList.count() - d->myMaximum + d->defaultList.count()); - for (int i = offset; i < d->itemList.count(); i++) { - d->insertUrlItem(d->itemList[i]); + KUrlComboBoxPrivate::KUrlComboItemList::size_type offset = qMax(KUrlComboBoxPrivate::KUrlComboItemList::size_type(0), d->itemList.size() - d->myMaximum + d->defaultList.size()); + for (auto i = offset; i < d->itemList.size(); i++) { + d->insertUrlItem(d->itemList.at(i).get()); } if (count() > 0) { // restore the previous currentItem @@ -356,21 +351,24 @@ void KUrlComboBox::removeUrl(const QUrl &url, bool checkDefaultURLs) { - QMap::ConstIterator mit = d->itemMapper.constBegin(); + auto mit = d->itemMapper.constBegin(); while (mit != d->itemMapper.constEnd()) { if (url.toString(QUrl::StripTrailingSlash) == mit.value()->url.toString(QUrl::StripTrailingSlash)) { - if (!d->itemList.removeAll(mit.value()) && checkDefaultURLs) { - d->defaultList.removeAll(mit.value()); + auto removePredicate = [&mit](const std::unique_ptr& item) { + return item.get() == mit.value(); + }; + d->itemList.erase(std::remove_if(d->itemList.begin(), d->itemList.end(), removePredicate), d->itemList.end()); + if (checkDefaultURLs) { + d->defaultList.erase(std::remove_if(d->defaultList.begin(), d->defaultList.end(), removePredicate), d->defaultList.end()); } } ++mit; } bool blocked = blockSignals(true); setDefaults(); - QListIterator it(d->itemList); - while (it.hasNext()) { - d->insertUrlItem(it.next()); + for (const auto& item : d->itemList) { + d->insertUrlItem(item.get()); } blockSignals(blocked); } @@ -406,7 +404,7 @@ void KUrlComboBox::mouseMoveEvent(QMouseEvent *event) { const int index = currentIndex(); - const KUrlComboBoxPrivate::KUrlComboItem *item = d->itemMapper.value(index); + auto item = d->itemMapper.value(index); if (item && !d->m_dragPoint.isNull() && event->buttons() & Qt::LeftButton && (event->pos() - d->m_dragPoint).manhattanLength() > QApplication::startDragDistance()) { @@ -436,7 +434,7 @@ // updates "item" with icon "icon" // kdelibs4 used to also say "and sets the URL instead of text", but this breaks const-ness, // now that it would require clearing the text, and I don't see the point since the URL was already in the text. -void KUrlComboBoxPrivate::updateItem(const KUrlComboBoxPrivate::KUrlComboItem *item, +void KUrlComboBoxPrivate::updateItem(const KUrlComboItem *item, int index, const QIcon &icon) { m_parent->setItemIcon(index, icon);