Differential D10466

Prevent Akregator from overwriting feeds.opml with an emtpy feed list
ClosedPublic
Actions

Authored by mgannon on Feb 12 2018, 5:09 PM.

Details

Reviewers
mlaurent
Commits
R201:6678efb711e2: Apply patch from mgannon
Summary

BUG : 381929

Quitting akregator causes ~/.local/share/akregator/data/feeds.opml to be overwritten using an empty feed list with the result that upon relaunching an error message appears and only the standard built in feeds are available.

In troubleshooting the issue, I noticed that slotSaveFeedList() is called five times between quitting the program and the program is finally shutdown. It is only the last time that the function is called with an empty list. This implies that there might be an error in the slot logic, since the function should only need to be called once. The attached patch seemed the easiest way to deal with the issue.

emptylist.patch478 BDownload

Test Plan

i. Start a clean installation of akregator.
ii. Import a list of feeds from an opml file.
iii. Logout from the kde desktop
iv. Login into the kde desktop.

Without the patch, an error message appears saying it is unable to open the file.

Diff Detail

Repository
R201 Akregator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mgannon requested review of this revision.Feb 12 2018, 5:09 PM
mgannon created this revision.
mlaurent added a comment.Feb 13 2018, 5:50 AM

it seems that when "void MainWidget::slotOnShutdown()" is calling it calls:
"setFeedList(QSharedPointer<FeedList>());"

> indeed the list will be empty.

I will look at why it's calling 5 times first

mlaurent added a comment.Feb 13 2018, 6:30 AM

ok I removed a call to slotSaveFeedList(). I commited in 17.12.3

mlaurent added a comment.Feb 13 2018, 6:31 AM

indeed
"if (xml.isEmpty()) {

return;

}"
is better that your code.

Could you create a new patch and commit ?
Or do you want that I commit it ?

mlaurent requested changes to this revision.Feb 14 2018, 5:31 AM
This revision now requires changes to proceed.Feb 14 2018, 5:31 AM
anthonyfieroni added a comment.Feb 25 2018, 4:53 PM

You can commit it on its behalf :)

mgannon requested review of this revision.Feb 25 2018, 6:36 PM

emptylist.patch478 BDownload

Sorry for the delay. I didn't get any emails from phabricator (but bugs.kde.org does work). Attached is a patch with the requested change using xml.isEmpty() to test for an empty list and returning without saving if the list is empty. I've verified that on my system (Gentoo Akregator 17.12.2-r2) the files are not deleted.

mlaurent added a comment.Feb 25 2018, 7:07 PM

@anthonyfieroni no patch was not corrected ...

mlaurent requested changes to this revision.Feb 25 2018, 7:08 PM

@mgannon sorry your patch is still not correct.
and please update review no attach a patch it's better :)
Thanks

This revision now requires changes to proceed.Feb 25 2018, 7:08 PM
mgannon updated this revision to Diff 28059.Feb 25 2018, 7:14 PM

Sorry for all the difficulties. This is the xml.IsEmpty() patch.

anthonyfieroni added inline comments.Feb 25 2018, 7:51 PM
src/akregator_part.cpp
531

At KDE it's used 4 spaces = 1 tab, so just correct indentation.

mgannon updated this revision to Diff 28065.Feb 25 2018, 8:23 PM
mgannon marked an inline comment as done.
mgannon added inline comments.
src/akregator_part.cpp
531

Hopefully it is correct now. Thanks for your patience!

mgannon marked an inline comment as done.Feb 25 2018, 8:31 PM
mlaurent accepted this revision.Feb 26 2018, 5:45 AM

I think that you don't have commit access.
I will commit for you.
thanks
regards

This revision is now accepted and ready to land.Feb 26 2018, 5:45 AM
Closed by commit R201:6678efb711e2: Apply patch from mgannon (authored by mlaurent). · Explain WhyFeb 26 2018, 5:49 AM
This revision was automatically updated to reflect the committed changes.
Restricted Application added a project: KDE PIM. · View Herald TranscriptFeb 26 2018, 5:49 AM
Restricted Application added a subscriber: KDE PIM. · View Herald Transcript

Revision Contents
Changeset List

PathPackages
Msrc/akregator_part.cpp (4 lines)

Diff 28084

View Options

src/akregator_part.cpp

Loading...