Don't delete ETMViewStateSaver manually
AbandonedPublic

Authored by dkurz on Aug 27 2016, 10:11 AM.

Details

Reviewers
None
Group Reviewers
KDE PIM
Summary

The API docs of KViewStateSerializer states explicitly that
savers destroy themselves, and that you shouldn't keep a
pointer to them after creating them on the heap.

Test Plan

Compile, run

Diff Detail

Repository
R92 PIM: Common Mail Support
Lint
Lint Skipped
Unit
Unit Tests Skipped
dkurz updated this revision to Diff 6315.Aug 27 2016, 10:11 AM
dkurz retitled this revision from to Don't delete ETMViewStateSaver manually.
dkurz updated this object.
dkurz edited the test plan for this revision. (Show Details)
dkurz added a reviewer: KDE PIM.
dkurz set the repository for this revision to R92 PIM: Common Mail Support.
Restricted Application added a project: KDE PIM. · View Herald TranscriptAug 27 2016, 10:11 AM
Restricted Application added a subscriber: kde-pim. · View Herald Transcript

It doesn't work when I implemented it.
So I prefere to keep code as it.
Regards

dkurz added a comment.Aug 28 2016, 6:27 PM

It doesn't work when I implemented it.

Sorry, I don't understand what you mean by that. I tested my implementation: In KMail's main widget, I tried various non-empty quick search texts and cleared the line edit again, and the selection state was correctly restored every time. It is not restored to the pre-quicksearch state when I click on one of the folders in the narrowed-down folder list, but I think that's intended behaviour. Please feel free to test it yourself, it works like a charm.

Maybe you hit some upstream bug when you tried to fix it. As I wrote, the api docs state explicitly that we should not keep a persistent pointer to savers on the heap. Also, a grep on the PIM code base reveals that restoring is always done as I suggest in this patch, with the single exception of todoview.cpp in the eventviews module:

$ grep -Rn "new \(Akonadi::\)\?ETMViewStateSaver\>" .
./eventviews/src/todo/todoview.cpp:1224:    mTreeStateRestorer = new Akonadi::ETMViewStateSaver();
./messagelib/messagelist/src/pane.cpp:1121:                    ETMViewStateSaver *saver = new ETMViewStateSaver;
./messagelib/messagelist/src/pane.cpp:1125:                        ETMViewStateSaver *saver = new ETMViewStateSaver;
./mailcommon/src/folder/foldertreewidget.cpp:168:        auto saver = new Akonadi::ETMViewStateSaver;
./kdepim/kaddressbook/src/mainwidget.cpp:424:        Akonadi::ETMViewStateSaver *saver = new Akonadi::ETMViewStateSaver;
./kdepim/kaddressbook/src/mainwidget.cpp:433:        Akonadi::ETMViewStateSaver *saver = new Akonadi::ETMViewStateSaver;
./kdepim/kaddressbook/src/mainwidget.cpp:442:        Akonadi::ETMViewStateSaver *saver = new Akonadi::ETMViewStateSaver;
./kdepim/kmail/src/kmmainwidget.cpp:354:    ETMViewStateSaver *saver = new ETMViewStateSaver;
./kdepim/korganizer/src/akonadicollectionview.cpp:225:        treeStateRestorer = new Akonadi::ETMViewStateSaver(); // not a leak
./kdepim-addons/examples/apps/etm_usage/unreadmailsincollectionswidget.cpp:161:    ETMViewStateSaver *restorer = new ETMViewStateSaver;

But maybe I just misunderstood your meaning.

knauss added a subscriber: knauss.Nov 29 2016, 1:14 PM
knauss added inline comments.
src/folder/foldertreewidget.cpp
168

just for curiosity, who is responsible for deleting this pointer? So far I would suggest just to create a QScopedPointer<Akonadi::ETMViewStateSaver> saver =[...]

to make sure, that we don't have a dangling pointer.

dkurz abandoned this revision.Aug 3 2017, 5:37 PM