D28649: [Notifications] Port to upstream QConcatenateTablesProxyModel
broulik added a comment. It's just a `QAbstractListModel`, if any, it does some `columnCount` behavior. I emit `dataChanged`, yes. Will check the actual column count later. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D28649 To: broulik, #plasma Cc: davidre, dfaure, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D28649: [Notifications] Port to upstream QConcatenateTablesProxyModel
dfaure added a comment. Wait, even if the jobs model is empty (no rows), what's its columnCount()? If that's fixed, then it should all be fine. Maybe my analysis (and testcase) is incorrect. Can you check the columnCount() of the Concat model? I assume the notifications model emits dataChanged correctly when the ExpiredRole is set to true? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D28649 To: broulik, #plasma Cc: davidre, dfaure, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D28649: [Notifications] Port to upstream QConcatenateTablesProxyModel
broulik added a comment. :( I see. Thanks! At least by then I'll be able to use `sourceModels()` so I could make the mapping up the chain generic REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D28649 To: broulik, #plasma Cc: davidre, dfaure, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D28649: [Notifications] Port to upstream QConcatenateTablesProxyModel
dfaure added a comment. OK I see why this is happening. KConcatenateProxyModel's columnCount is the number of columns of the first source model. Which leads to strange things if other models have less columns. QConcatenateProxyModel takes the smallest number of columns of all source models, to solve that. But here the jobs model is empty. And empty model shouldn't be taken into account when calculating the number of columns. I'll fix that in QConcatenateProxyModel, but that means you can't use it for another 2 years or so... oh well ;) REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D28649 To: broulik, #plasma Cc: davidre, dfaure, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D28649: [Notifications] Port to upstream QConcatenateTablesProxyModel
davidre added subscribers: dfaure, davidre. davidre added a comment. This causes expired notifications to not vanish any more. This is supposed to work in the following way `ExpiredRole` is assigned `true` and `NotficiationFilterProxyModel` filters expired notifications out. I verified that upon timeout of the timer the data is updated correctly but the filter model behaves in a strange way. Even though filterAcceptsRow returns false the model still includes the index. I added the following debug output to the class: https://phabricator.kde.org/P584 Which results in this debug output QModelIndex(0,0,0x0,QConcatenateTablesProxyModel(0x557a51b00d90)) is expired QModelIndex(-1,-1,0x0,QObject(0x0)) not expired rowCount NotificationManager::NotificationFilterProxyModel(0x557a51f6ab00) QModelIndex(-1,-1,0x0,QObject(0x0)) 1 QModelIndex(0,0,0x0,QConcatenateTablesProxyModel(0x557a51b00d90)) is expired filter returns for filterAcceptsRow(index(0, mapToSource(parent)) false rowCount NotificationManager::NotificationFilterProxyModel(0x557a51f6ab00) QModelIndex(-1,-1,0x0,QObject(0x0)) 1 QModelIndex(0,0,0x0,QConcatenateTablesProxyModel(0x557a51b00d90)) is expired filter returns for filterAcceptsRow(index(0, mapToSource(parent)) false So it still has one row even though the first row should get filtered. Adding @dfaure because he wrote both K/QConcatenate... models REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D28649 To: broulik, #plasma Cc: davidre, dfaure, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D28649: [Notifications] Port to upstream QConcatenateTablesProxyModel
broulik planned changes to this revision. broulik added a comment. Apparently this causes all kinds of breakages :0 REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D28649 To: broulik, #plasma Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D28649: [Notifications] Port to upstream QConcatenateTablesProxyModel
broulik created this revision. broulik added a reviewer: Plasma. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. broulik requested review of this revision. REVISION SUMMARY Now that we can depend on Qt 5.14 TEST PLAN Still get both jobs and notifications. Can still interact with both and mapping is correct REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D28649 AFFECTED FILES libnotificationmanager/notifications.cpp libnotificationmanager/utils.cpp To: broulik, #plasma Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
