Differential D7548

Prevent tag name vs gid confusion
ClosedPublic
Actions

Authored by dkurz on Aug 25 2017, 9:06 PM.

Details

Reviewers
dvratil
Group Reviewers
KDE PIM
Commits
R78:b807715001e8: 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.

Diff Detail

Repository
R78 PIM: Incidence Editor
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
dkurz created this revision.Aug 25 2017, 9:06 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptAug 25 2017, 9:06 PM
dvratil added a subscriber: dvratil.EditedAug 25 2017, 9:19 PM

I created an event in KOrganizer, gave it a bunch of categories and saved it. Then I edited the event, changed some parts and added some more categories and I did not see any GID-named categories being created. Did I miss some steps?

Btw I've published some docs about Tags: http://cgit.kde.org/akonadi.git/tree/docs/tags.md (it will also be available on api.kde.org once the regeneration happens).

src/incidencecategories.cpp
69 ↗(On Diff #18772)

So who now pre-selects the tags that the Item has?

dkurz marked an inline comment as done.Aug 25 2017, 9:35 PM
In D7548#140147, @dvratil wrote:

I created an event in KOrganizer, gave it a bunch of categories and saved it. Then I edited the event, changed some parts and added some more categories and I did not see any GID-named categories being created. Did I miss some steps?

Are you trying to reproduce the bug in an unpatched version? Some reasons why it might not have triggered for you:

  • you did not close the incidence dialog between the changes
  • you opened the category dialog when making your other changes, too
  • you assigned completely new categories to your tag (I think it should trigger in that case, but I'm not sure)

The bug does not happen as soon as the category dialog is confirmed. In contrast to IncidenceCategories, which gets its Akonadi::Item from the EditorItemManager with incomplete Tags (no attributes fetched), the category dialog has its own set of Tags complete with the TAG attribute.

Try the following:

  1. open an existing event
  2. only change its description
  3. click OK
  4. open the same event again

That should do the trick.

src/incidencecategories.cpp
69 ↗(On Diff #18772)

We already meet incidence (stored in mLoadedIncidence) in the other load method. KCalCore::Incidence does not know about Akonadi::Items, but only has a set of strings for its categories, so there's no risk of confusing the two. The incidence's categories are then used as soon as we have complete information about the tags to match them in onTagsFetched.

dkurz marked an inline comment as done.Aug 25 2017, 9:36 PM

And of course, yay for the Tag docs! That's much more text than I expected

dvratil accepted this revision.Aug 25 2017, 10:20 PM

Still can't reproduce, but your patch makes sense and still works even with your changes, so ship it :-) Thanks for the pitch!

Maybe remove IncidenceCategories::load(Akonadi::Item) since it's useless now.

This revision is now accepted and ready to land.Aug 25 2017, 10:20 PM
Closed by commit R78:b807715001e8: Prevent tag name vs gid confusion (authored by dkurz). · Explain WhyAug 25 2017, 10:48 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Msrc/incidencecategories.h (3 lines)
Msrc/incidencecategories.cpp (66 lines)

Diff 18781

View Options

src/incidencecategories.h

Loading...
View Options

src/incidencecategories.cpp

Loading...