Commit b8077150 authored by Denis Kurz's avatar Denis Kurz
Browse files

Prevent tag name vs gid confusion

Summary:
The old code relied on the display name of the tags of the item provided
via load(const Akonadi::Item&) being set, but those aren't actually
fetched. This led IncidenceCategories to believe that an item's
categories are the gids of its tags. Saving the item without confirming
the category dialog (e.g. by only modifying the description) led to this
gid being written back to Akonadi. Opening the item again afterwards
then even leads to a corresponding Akonadi tag being created.

We reduce state in IncidenceEditor, stop matching Tags twice a
row (in onTagsFetched and matchExistingCategories), and rely more on
incidence->categories(), where no gid confusion exists.

BUG: 373257
FIXED-IN: 5.6.1

Test Plan:
I editted multiple events in KOrganizer, modifying categories or
description or title of the event, or all of them in one change.
For categories, I tried to set related subsets of my tags (a sub-
or superset of the previously selected tags), or completely unrelated
sets (deselecting all selected, and selecting previously other ones
instead). All changes were saved as expected, and no gid-named
categories were created in the process.

Reviewers: #kde_pim, dvratil

Reviewed By: #kde_pim, dvratil

Subscribers: dvratil

Tags: #kde_pim

Differential Revision: https://phabricator.kde.org/D7548
parent add5866a
......@@ -45,7 +45,7 @@ IncidenceCategories::IncidenceCategories(Ui::EventOrTodoDesktop *ui)
void IncidenceCategories::onSelectionChanged(const Akonadi::Tag::List &list)
{
mSelectedTags = list;
Q_UNUSED(list);
mDirty = true;
checkDirtyStatus();
}
......@@ -55,8 +55,7 @@ void IncidenceCategories::load(const KCalCore::Incidence::Ptr &incidence)
mLoadedIncidence = incidence;
mDirty = false;
mWasDirty = false;
mSelectedTags.clear();
mMissingCategories = incidence->categories();
mMissingCategories.clear();
if (mLoadedIncidence) {
Akonadi::TagFetchJob *fetchJob = new Akonadi::TagFetchJob(this);
......@@ -67,29 +66,31 @@ void IncidenceCategories::load(const KCalCore::Incidence::Ptr &incidence)
void IncidenceCategories::load(const Akonadi::Item &item)
{
mSelectedTags = item.tags();
mUi->mTagWidget->setSelection(item.tags());
Q_UNUSED(item);
}
void IncidenceCategories::save(const KCalCore::Incidence::Ptr &incidence)
{
Q_ASSERT(incidence);
incidence->setCategories(categories());
if (mDirty) {
incidence->setCategories(categories());
}
}
void IncidenceCategories::save(Akonadi::Item &item)
{
if (item.tags() != mSelectedTags) {
item.setTags(mSelectedTags);
const auto &selectedTags = mUi->mTagWidget->selection();
if (mDirty) {
item.setTags(selectedTags);
}
}
QStringList IncidenceCategories::categories() const
{
QStringList list;
list.reserve(mSelectedTags.count() + mMissingCategories.count());
for (const Akonadi::Tag &tag : qAsConst(mSelectedTags)) {
const auto &selectedTags = mUi->mTagWidget->selection();
list.reserve(selectedTags.count() + mMissingCategories.count());
for (const Akonadi::Tag &tag : selectedTags) {
list << tag.name();
}
list << mMissingCategories;
......@@ -113,26 +114,12 @@ bool IncidenceCategories::isDirty() const
void IncidenceCategories::printDebugInfo() const
{
qCDebug(INCIDENCEEDITOR_LOG) << "mSelectedCategories = " << categories();
qCDebug(INCIDENCEEDITOR_LOG) << "selected categories = " << categories();
qCDebug(INCIDENCEEDITOR_LOG) << "mMissingCategories = " << mMissingCategories;
qCDebug(INCIDENCEEDITOR_LOG) << "mLoadedIncidence->categories() = "
<< mLoadedIncidence->categories();
}
void IncidenceCategories::matchExistingCategories(const QStringList &categories,
const Akonadi::Tag::List &existingTags)
{
for (const QString &category : categories) {
auto matchedTagIter = std::find_if(existingTags.cbegin(), existingTags.cend(),
[&category](const Akonadi::Tag &tag) {
return tag.name() == category;
});
Q_ASSERT(matchedTagIter != existingTags.cend());
if (!mSelectedTags.contains(*matchedTagIter)) {
mSelectedTags << *matchedTagIter;
}
}
}
void IncidenceCategories::onTagsFetched(KJob *job)
{
if (job->error()) {
......@@ -141,15 +128,20 @@ void IncidenceCategories::onTagsFetched(KJob *job)
}
Akonadi::TagFetchJob *fetchJob = static_cast<Akonadi::TagFetchJob *>(job);
const Akonadi::Tag::List jobTags = fetchJob->tags();
QStringList matchedCategories;
for (const Akonadi::Tag &tag : jobTags) {
Q_ASSERT(mLoadedIncidence);
mMissingCategories = mLoadedIncidence->categories();
Akonadi::Tag::List selectedTags;
selectedTags.reserve(mMissingCategories.count());
for (const auto &tag : jobTags) {
if (mMissingCategories.removeAll(tag.name()) > 0) {
matchedCategories << tag.name();
selectedTags << tag;
}
}
matchExistingCategories(matchedCategories, jobTags);
createMissingCategories();
mUi->mTagWidget->setSelection(mSelectedTags);
mUi->mTagWidget->setSelection(selectedTags);
}
void IncidenceCategories::onMissingTagCreated(KJob *job)
......@@ -159,7 +151,11 @@ void IncidenceCategories::onMissingTagCreated(KJob *job)
return;
}
Akonadi::TagCreateJob *createJob = static_cast<Akonadi::TagCreateJob *>(job);
mMissingCategories.removeAll(createJob->tag().name());
mSelectedTags << createJob->tag();
mUi->mTagWidget->setSelection(mSelectedTags);
int count = mMissingCategories.removeAll(createJob->tag().name());
Q_ASSERT(count > 0);
QVector<Akonadi::Tag> selectedTags;
selectedTags.reserve(mUi->mTagWidget->selection().count() + 1);
selectedTags << mUi->mTagWidget->selection() << createJob->tag();
mUi->mTagWidget->setSelection(selectedTags);
}
......@@ -48,8 +48,6 @@ public:
void printDebugInfo() const override;
private:
void matchExistingCategories(const QStringList &categories,
const Akonadi::Tag::List &existingTags);
void createMissingCategories();
private Q_SLOTS:
......@@ -59,7 +57,6 @@ private Q_SLOTS:
private:
Ui::EventOrTodoDesktop *mUi;
Akonadi::Tag::List mSelectedTags;
/**
* List of categories for which no tag might exist.
......
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