Differential D10964

Use QPointer member variables in Gwenview StartHelper
ClosedPublic
Actions

Authored by muhlenpfordt on Mar 2 2018, 10:15 AM.

Details

Reviewers
rkflx
Group Reviewers
Gwenview
Commits
R260:a08587f77fd0: Use QPointer member variables in Gwenview StartHelper
Summary

cppcheck complains about the never deleted local pointer variable
that creates the Gwenview::MainWindow.
This patch uses QPointer and QScopedPointer for MainWindow
and the optional QTemporaryDir variables to clean this.

Test Plan

Check Gwenview's normal behaviour starting with different arguments
(none, single/multiple image/s, single/multiple directory/ies).

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
muhlenpfordt requested review of this revision.Mar 2 2018, 10:15 AM
muhlenpfordt created this revision.
muhlenpfordt added a reviewer: Gwenview.Mar 2 2018, 10:17 AM
muhlenpfordt added inline comments.
app/main.cpp
117–118

Same functionality here, just the Qt way.

118

A QScopedPointer for MainWindow does not work since it's already deleted on quit. We most likely don't need the delete mMainWindow but to be safe and clean it's added.

rkflx added a subscriber: rkflx.Mar 2 2018, 1:24 PM

Lint OK

💯

BTW, related: https://www.mail-archive.com/kde-devel@kde.org/msg10072.html

Check session (re-)store with e.g. xsm.

Nice, did not know that trick yet. Works a bit differently to Plasma's session restore, though, so I'm not sure it's the best test case.

app/main.cpp
118

Add qDebug() << mMainWindow; after new and in the destructor, then compare the output both for Quit and Plasma session saving (~/.local/share/sddm/xorg-session.log).

Yesterday I also thought it's better to have, but now I suspect Qt is already deleting it for us, being a QObject.

It's only with xsm the output sometimes is not null, but it's not crashing in that case either.

Thoughts?

muhlenpfordt added inline comments.Mar 2 2018, 1:59 PM
app/main.cpp
118

The MainWindow is created with the flag Qt::WA_DeleteOnClose: KXmlGuiWindow. I think we can remove ~StartHelper().
Good hint for the session restore testing. :)

rkflx accepted this revision.Mar 2 2018, 2:09 PM

Thanks again ;)

(And sorry your other patches will take me more time to get to… Maybe chime in on some of the other tasks meanwhile?)

app/main.cpp
118

Okay, do that.

This revision is now accepted and ready to land.Mar 2 2018, 2:09 PM
muhlenpfordt updated this revision to Diff 28407.Mar 2 2018, 2:12 PM
muhlenpfordt edited the test plan for this revision. (Show Details)
muhlenpfordt removed a reviewer: rkflx.

Removed StartHelper destructor

This revision now requires review to proceed.Mar 2 2018, 2:12 PM
muhlenpfordt added a reviewer: rkflx.Mar 2 2018, 2:14 PM
In D10964#216893, @muhlenpfordt wrote:

removed a reviewer: rkflx.

Huh what's this? :(

rkflx accepted this revision.Mar 2 2018, 2:19 PM

I guess you used arc diff --edit --verbatim without using arc amend first. On Phab I was set as reviewer, but you forced your old version where I was not (in the sense of the reviewer state, not in the sense of the updated test plan).

This revision is now accepted and ready to land.Mar 2 2018, 2:19 PM
muhlenpfordt added a comment.Mar 2 2018, 3:58 PM
In D10964#216902, @rkflx wrote:

I guess you used arc diff --edit --verbatim without using arc amend first.

Ok, that was the missing part. I wonder why this did not happen ever before. ;)

In D10964#216884, @rkflx wrote:

(And sorry your other patches will take me more time to get to…

All good. One step at a time... :)

Closed by commit R260:a08587f77fd0: Use QPointer member variables in Gwenview StartHelper (authored by muhlenpfordt). · Explain WhyMar 2 2018, 4:00 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mapp/main.cpp (21 lines)

Diff 28416

View Options

app/main.cpp

Loading...