Commit a594a209 authored by Ahmad Samir's avatar Ahmad Samir Committed by Kurt Hindenburg
Browse files

Fix issues with the container used to hold the Profiles in the ProfileManager

I'd used a std::set with a compare function, so that the profiles are always
sorted by name ...etc, but that is wrong, as the Key type in the set is a
QExplicitlySharedDataPointer, if the QESDP is copied, its own address is
going to change, but the address of of the object it's managing (Profile)
is the same, that's why QESDP internally overloads operator== to compare
the address of its d pointer (the one returned by data()).

That meant that multiple QESDP could exist in the set, not good... (a QSet
worked because it uses a hash which is based on the address of the object
returned by data()).

So, use a vector, that is sorted by profile name (we only need to sort it
when allProfiles() is called, and when a profile name is changed).

This fixes an assert in ProfileManager::setDefaultProfile(); to test open
the profile manager and select the built-in profile and click "set default".

Add a unit test.
parent df16adb7
......@@ -14,12 +14,20 @@
// Konsole
#include "../profile/Profile.h"
#include "../profile/ProfileGroup.h"
#include "../profile/ProfileManager.cpp"
#include "../profile/ProfileWriter.h"
#include <QStandardPaths>
#include <array>
using namespace Konsole;
void ProfileTest::initTestCase()
{
QStandardPaths::setTestModeEnabled(true);
}
void ProfileTest::testProfile()
{
// create a new profile
......@@ -215,6 +223,40 @@ void ProfileTest::testProfileFileNames()
delete writer;
}
void ProfileTest::testProfileNameSorting()
{
auto *manager = ProfileManager::instance();
const int origCount = manager->allProfiles().size();
Profile::Ptr profile1 = Profile::Ptr(new Profile);
profile1->setProperty(Profile::UntranslatedName, QStringLiteral("Indiana"));
manager->addProfile(profile1);
auto list = manager->allProfiles();
int counter = 1;
QCOMPARE(list.size(), origCount + counter++);
// Built-in profile always at the top
QCOMPARE(list.at(0)->name(), QStringView(u"Default"));
QVERIFY(std::is_sorted(list.cbegin(), list.cend(), profileNameLessThan));
Profile::Ptr profile2 = Profile::Ptr(new Profile);
profile2->setProperty(Profile::UntranslatedName, QStringLiteral("Old Paris"));
manager->addProfile(profile2);
list = manager->allProfiles();
QCOMPARE(list.size(), origCount + counter++);
QVERIFY(std::is_sorted(list.cbegin(), list.cend(), profileNameLessThan));
Profile::Ptr profile3 = Profile::Ptr(new Profile);
profile3->setProperty(Profile::UntranslatedName, QStringLiteral("New Zealand"));
manager->addProfile(profile3);
list = manager->allProfiles();
QCOMPARE(list.size(), origCount + counter++);
QVERIFY(std::is_sorted(list.cbegin(), list.cend(), profileNameLessThan));
QCOMPARE(list.at(0)->name(), QLatin1String("Default"));
}
void ProfileTest::testFallbackProfile()
{
// create a new profile
......
......@@ -16,10 +16,12 @@ class ProfileTest : public QObject
Q_OBJECT
private Q_SLOTS:
void initTestCase();
void testProfile();
void testClone();
void testProfileGroup();
void testProfileFileNames();
void testProfileNameSorting();
void testFallbackProfile();
};
......
......@@ -37,6 +37,18 @@ static bool stringLessThan(const QString &p1, const QString &p2)
return QString::localeAwareCompare(p1, p2) < 0;
}
static bool profileNameLessThan(const Profile::Ptr &p1, const Profile::Ptr &p2)
{
// Always put the Default/fallback profile at the top
if (p1->isFallback()) {
return true;
} else if (p2->isFallback()) {
return false;
}
return stringLessThan(p1->name(), p2->name());
}
ProfileManager::ProfileManager()
: _defaultProfile(nullptr)
, _fallbackProfile(nullptr)
......@@ -91,6 +103,11 @@ ProfileManager *ProfileManager::instance()
return theProfileManager;
}
ProfileManager::Iterator ProfileManager::findProfile(const Profile::Ptr &profile) const
{
return std::find(_profiles.cbegin(), _profiles.cend(), profile);
}
void ProfileManager::initFallbackProfile()
{
_fallbackProfile = Profile::Ptr(new Profile());
......@@ -230,10 +247,15 @@ void ProfileManager::saveSettings()
appConfig->sync();
}
void ProfileManager::sortProfiles()
{
std::sort(_profiles.begin(), _profiles.end(), profileNameLessThan);
}
QList<Profile::Ptr> ProfileManager::allProfiles()
{
loadAllProfiles();
sortProfiles();
return loadedProfiles();
}
......@@ -289,17 +311,20 @@ void ProfileManager::changeProfile(Profile::Ptr profile, QHash<Profile::Property
persistent = persistent && !profile->name().isEmpty();
bool messageShown = false;
bool isNameChanged = false;
// Insert the changes into the existing Profile instance
for (auto it = propertyMap.cbegin(); it != propertyMap.cend(); ++it) {
const auto property = it.key();
auto value = it.value();
isNameChanged = property == Profile::Name || property == Profile::UntranslatedName;
// "Default" is reserved for the fallback profile, override it;
// The message is only shown if the user manually typed "Default"
// in the name box in the edit profile dialog; i.e. saving the
// fallback profile where the user didn't change the name at all,
// the uniqueProfileName is used silently a couple of lines above.
if ((property == Profile::Name || property == Profile::UntranslatedName) && value == QLatin1String("Default")) {
if (isNameChanged && value == QLatin1String("Default")) {
value = uniqueProfileName;
if (!messageShown) {
KMessageBox::sorry(nullptr,
......@@ -357,6 +382,10 @@ void ProfileManager::changeProfile(Profile::Ptr profile, QHash<Profile::Property
}
}
if (isNameChanged) {
sortProfiles();
}
// Notify the world about the change
Q_EMIT profileChanged(profile);
}
......@@ -367,9 +396,10 @@ void ProfileManager::addProfile(const Profile::Ptr &profile)
_defaultProfile = profile;
}
_profiles.insert(profile);
Q_EMIT profileAdded(profile);
if (findProfile(profile) == _profiles.cend()) {
_profiles.push_back(profile);
Q_EMIT profileAdded(profile);
}
}
bool ProfileManager::deleteProfile(Profile::Ptr profile)
......@@ -387,7 +417,9 @@ bool ProfileManager::deleteProfile(Profile::Ptr profile)
}
setShortcut(profile, QKeySequence());
_profiles.erase(profile);
if (auto it = findProfile(profile); it != _profiles.end()) {
_profiles.erase(it);
}
// mark the profile as hidden so that it does not show up in the
// Manage Profiles dialog and is not saved to disk
......@@ -408,7 +440,7 @@ bool ProfileManager::deleteProfile(Profile::Ptr profile)
void ProfileManager::setDefaultProfile(const Profile::Ptr &profile)
{
Q_ASSERT(_profiles.find(profile) != _profiles.cend());
Q_ASSERT(findProfile(profile) != _profiles.cend());
const auto oldDefault = _defaultProfile;
_defaultProfile = profile;
......
......@@ -18,7 +18,7 @@
#include <QStringList>
#include <QVariant>
#include <set>
#include <vector>
// Konsole
#include "Profile.h"
......@@ -34,6 +34,8 @@ class KONSOLEPROFILE_EXPORT ProfileManager : public QObject
Q_OBJECT
public:
using Iterator = std::vector<Profile::Ptr>::const_iterator;
/**
* Constructs a new profile manager and loads information about the available
* profiles.
......@@ -213,20 +215,14 @@ private:
// otherwise
QString saveProfile(const Profile::Ptr &profile);
static bool profileNamesCompare(const Profile::Ptr &p1, const Profile::Ptr &p2)
{
// Always put the Default/fallback profile at the top
if (p1->isFallback()) {
return true;
} else if (p2->isFallback()) {
return false;
}
// Sorts _profiles by profile name
void sortProfiles();
return QString::localeAwareCompare(p1->name(), p2->name()) < 0;
}
// Uses std::find to find "profile" _profiles
Iterator findProfile(const Profile::Ptr &profile) const;
// A list of all loaded profiles, sorted by profile name
std::set<Profile::Ptr, decltype(profileNamesCompare) *> _profiles{profileNamesCompare};
std::vector<Profile::Ptr> _profiles;
Profile::Ptr _defaultProfile;
Profile::Ptr _fallbackProfile;
......
......@@ -196,4 +196,6 @@ void ProfileModel::update(QExplicitlySharedDataPointer<Profile> profile)
{
int row = m_profiles.indexOf(profile);
dataChanged(index(row, 0), index(row, COLUMNS - 1));
// Resort as the profile name could have changed
populate();
}
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment