Differential D13596

Fix there being more security updates than total updates in notifier
ClosedPublic
Actions

Authored by Zren on Jun 19 2018, 12:26 AM.

Details

Reviewers
apol
Group Reviewers
Discover Software Store
Commits
R134:9577e7d1d333: Fix there being more security updates than total updates in notifier
Summary

We currently read the previous m_securityCount instead of using the current securityCount.

BUG: 392056
FIXED-IN 5.13.1 (or 5.13.2?)
FIXED-IN 5.12.6 (This should be backported right?) https://github.com/KDE/discover/blame/Plasma/5.12/notifier/DiscoverNotifier.cpp#L82

The bug report is from 5.12.3, and was also reported on reddit today. Both have a screenshot of the bug.
https://bugs.kde.org/show_bug.cgi?id=392056
https://www.reddit.com/r/kde/comments/8rzrr6/so_whats_the_logic_behind_updater/

We could also start the counter at 0 and add securityCount afterward.

-    uint count = securityUpdatesCount();
+    uint count = 0;
     foreach(BackendNotifierModule* module, m_backends)
         count += module->updatesCount();
+    count += securityCount;
Test Plan

I haven't attempted to compile this or test it. I don't have any security updates atm anyways.
I haven't pushed a bugfix to a Plasma/5.__ branch before either. Do I make a separate commit for 5.12, 5.13, and master?

Diff Detail

Repository
R134 Discover Software Store
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Zren created this revision.Jun 19 2018, 12:26 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJun 19 2018, 12:26 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
Zren requested review of this revision.Jun 19 2018, 12:26 AM
ngraham added reviewers: Discover Software Store, apol.Jun 19 2018, 2:28 AM
ngraham added subscribers: apol, ngraham.

For bugfixes like this, the workflow is to commit to the "stable branch" (in this case Plasma/5.12) and then merge to master. In this case you would also merge to Plasma/5.13. the FIXED-IN tag should be 5.12.6.

See also https://community.kde.org/Infrastructure/Phabricator#Landing_on_the_.22Stable_branch.22

As for the patch, I tried it out and it seems to solve the problem! No opinion on whether your current approach or the alternative one you mentioned would be most appropriate. I'll let @apol have the final say.

davidedmundson added a subscriber: davidedmundson.Jun 19 2018, 8:25 AM

Do I make a separate commit for 5.12, 5.13, and master?

If possible, we commit to the oldest relevant branch then merge upwards.

i.e
commit to 5.12
merge 5.12 into 5.13
merge 5.13 into master

Otherwise git will treat them as different commits which makes some admin tasks, like seeing which branches contain a fix, harder.

apol accepted this revision.Jun 19 2018, 2:12 PM

Thanks!
Please land in 5.12.

This revision is now accepted and ready to land.Jun 19 2018, 2:12 PM
apol added a comment.Jun 19 2018, 2:13 PM

You just commit it to Plasma/5.12 branch and it will get propagated when I merge 5.12 to 5.13 and master

Closed by commit R134:9577e7d1d333: Fix there being more security updates than total updates in notifier (authored by Zren). · Explain WhyJun 19 2018, 9:09 PM
This revision was automatically updated to reflect the committed changes.
Zren added a comment.Jun 19 2018, 9:22 PM

Thanks @apol for merging it. Thanks @ngraham for testing it and the link and @davidedmundson for explaining how bugfixes are merged.

Revision Contents
Changeset List

PathPackages
Mnotifier/DiscoverNotifier.cpp (2 lines)

Diff 36355

View Options

notifier/DiscoverNotifier.cpp

Loading...