Refactor ActivityListModel population mechanisms

[claucambra]

Signed-off-by: Claudio Cambra claudio.cambra@gmail.com

As it stands, the ActivityListModel has two issues:

1. It updates coarsely, bulldozing all prior data and replacing with newly-acquired data
2. It has duplicated and messy code relating to inserting and removing rows from the model

This refactoring eliminates these issues, which in turn also prevents the activity list view resetting every time something changes (i.e. receiving new activities, dismissing notifications, etc.) by updating the core data list granularly rather than coarsely. This addresses a point in #4432

NOTE: this PR depends on #4735 as it removes the clearNotifications method which goes unused after the internal refactoring of ActivityListModel and the removal of clearNotifications from the UserModel in #4735

Fixes #4724

[codecov]

Codecov Report

Merging #4736 (20c5f8e) into master (20c5f8e) will not change coverage.
The diff coverage is n/a.

❗ Current head 20c5f8e differs from pull request most recent head 1a5fa50. Consider uploading reports for the commit 1a5fa50 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4736   +/-   ##
=======================================
  Coverage   57.20%   57.20%           
=======================================
  Files         138      138           
  Lines       17138    17138           
=======================================
  Hits         9804     9804           
  Misses       7334     7334           

[claucambra]

/backport to stable-3.5

[mgallien]

/backport to stable-3.5

I would prefer not to backport in case that would induce regressions

[mgallien]

please handle the automated comments from sonarcloud has many of them provide feedback that would need to be addressed

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

91.5% Coverage
0.0% Duplication

[mgallien]

this test and the few next ones would be better written as data driven tests with different data instead of doing an utility method and calling it manually several time
see https://doc.qt.io/qt-5/qttestlib-tutorial2-example.html

[claucambra]

I am not sure, what we want to test here is that the activity is added to the activity list correctly, and that it returns the right data, right? For the latter we have the data testing function, in the test slots that take these methods we are testing if the activity is actually added properly

I am not sure what benefit using the QTest data table would bring, in our case it seems it would add more complexity as we would still need to create the activity objects and keep them around, add the activity data we want to test to the QTest table, and then test the table rows vs. just testing the data we get from the table with the data in the activity we are retaining directly

Am I misunderstanding the purpose of the QTest data table?

[mgallien]

I would say that the data driven tests are the thing you have to use when you want to run teh same code multiple time with different input values
so sounded to me like the solution here

[claucambra]

I don't see a lot of reason for the table as we need to keep the activity objects around and these are never changed. We can just compare these directly, at least as I see it -- we instantiate the activities once in the constructor and that is it, we don't need to re-create new activities in each test

The polymorphic way of testing the methods is just there to remove the need for repeating the same lines of code checking for things over and over (which we would have to do with data driven tests anyway)

[jancborchardt]

It updates coarsely, bulldozing all prior data and replacing with newly-acquired data

Ahh nice, so this also fixes the issue of old Talk notifications which were already seen via other apps still showing up? :)

[claucambra]

It updates coarsely, bulldozing all prior data and replacing with newly-acquired data

Ahh nice, so this also fixes the issue of old Talk notifications which were already seen via other apps still showing up? :)

Not really; as I know we do not receive a list of completed notifications from the server which would let us filter out notifications from our presently-shown notifications. But this should prevent showing duplicates

[allexzander]

😴 Would be great to have the PR contain multiple small self-explanatory commits, in the future, when possible, to ease the review with so many changes.

[allexzander]

@claucambra Just, please don't forget to fix the build

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

91.5% Coverage
0.0% Duplication

[nextcloud-desktop-bot]

AppImage file: nextcloud-PR-4736-1a5fa50fbbfb006232fcc9d92733cdf2e9573056-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.