D13596: Fix there being more security updates than total updates in notifier
Zren created this revision. Restricted Application added a project: Plasma. Restricted Application added a subscriber: plasma-devel. Zren requested review of this revision. REVISION 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? REPOSITORY R134 Discover Software Store REVISION DETAIL https://phabricator.kde.org/D13596 AFFECTED FILES notifier/DiscoverNotifier.cpp To: Zren Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D13596: Fix there being more security updates than total updates in notifier
ngraham added subscribers: apol, ngraham. ngraham added reviewers: Discover Software Store, apol. ngraham added a comment. 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. REPOSITORY R134 Discover Software Store REVISION DETAIL https://phabricator.kde.org/D13596 To: Zren, #discover_software_store, apol Cc: ngraham, apol, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart
D13596: Fix there being more security updates than total updates in notifier
davidedmundson added a comment. > 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. REPOSITORY R134 Discover Software Store REVISION DETAIL https://phabricator.kde.org/D13596 To: Zren, #discover_software_store, apol Cc: davidedmundson, ngraham, apol, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart
D13596: Fix there being more security updates than total updates in notifier
apol accepted this revision. apol added a comment. This revision is now accepted and ready to land. Thanks! Please land in 5.12. REPOSITORY R134 Discover Software Store REVISION DETAIL https://phabricator.kde.org/D13596 To: Zren, #discover_software_store, apol Cc: davidedmundson, ngraham, apol, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart
D13596: Fix there being more security updates than total updates in notifier
apol added a comment. You just commit it to Plasma/5.12 branch and it will get propagated when I merge 5.12 to 5.13 and master REPOSITORY R134 Discover Software Store REVISION DETAIL https://phabricator.kde.org/D13596 To: Zren, #discover_software_store, apol Cc: davidedmundson, ngraham, apol, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart
D13596: Fix there being more security updates than total updates in notifier
This revision was automatically updated to reflect the committed changes. Closed by commit R134:9577e7d1d333: Fix there being more security updates than total updates in notifier (authored by Zren). REPOSITORY R134 Discover Software Store CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13596?vs=36297&id=36355 REVISION DETAIL https://phabricator.kde.org/D13596 AFFECTED FILES notifier/DiscoverNotifier.cpp To: Zren, #discover_software_store, apol Cc: davidedmundson, ngraham, apol, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart
D13596: Fix there being more security updates than total updates in notifier
Zren added a comment. Thanks @apol for merging it. Thanks @ngraham for testing it and the link and @davidedmundson for explaining how bugfixes are merged. REPOSITORY R134 Discover Software Store REVISION DETAIL https://phabricator.kde.org/D13596 To: Zren, #discover_software_store, apol Cc: davidedmundson, ngraham, apol, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart
