Prevent grouping/duplicating notifications and fix "Open" button issue
Needs RevisionPublic

Authored by kapillamba4 on Mar 10 2018, 6:51 AM.

Details

Reviewers
rkflx
Group Reviewers
Spectacle
Summary

BUG: 389999

Fix "Open" button in screenshot notification not responding to click by allowing spectacle to quit after KRun is finished instead of using QTimer::singleShot to quit spectacle.

Close the previous notification before showing next notification.
Set "CloseOnTimeout" and "SkipGrouping" flags for KNotification to prevent grouping and duplicating notifications.

Test Plan

Tested all 3 shortcuts: full screen, active window and rectangular region.

Diff Detail

Repository
R166 Spectacle
Branch
fix-multiple-notifications
Lint
No Linters Available
Unit
No Unit Test Coverage
kapillamba4 requested review of this revision.Mar 10 2018, 6:51 AM
kapillamba4 created this revision.
kapillamba4 added a comment.EditedMar 10 2018, 7:41 AM

Open button does not work sometimes for Spectacle v18.03.70:

After this patch:

kapillamba4 retitled this revision from Fix multiple screenshots openning together through notification, prevent grouping of notification with previous notifications and prevent addition of duplicate notifications to Prevent grouping of screenshot notification with previous screenshot notifications, prevent duplicate notifications and fix "Open" button issue in notification..Mar 10 2018, 7:48 AM
kapillamba4 edited the summary of this revision. (Show Details)
kapillamba4 edited the test plan for this revision. (Show Details)
kapillamba4 added a reviewer: Spectacle.
kapillamba4 added a project: Spectacle.
kapillamba4 added a subscriber: kapillamba4.
rkflx added a subscriber: rkflx.EditedMar 10 2018, 7:52 AM

Thanks so much for your patch, Kapil! I'll have a look as soon as I made progress with reviewing your other patch (this whole notification thing is a bit tricky…).

In the meantime, to make your commit message even more perfect, I can recommend to read this: https://chris.beams.io/posts/git-commit/#limit-50 (IOW, you could shorten your title a bit).

kapillamba4 retitled this revision from Prevent grouping of screenshot notification with previous screenshot notifications, prevent duplicate notifications and fix "Open" button issue in notification. to Prevent grouping/duplicating notifications and fix "Open" button issue.Mar 10 2018, 11:39 AM
kapillamba4 edited the summary of this revision. (Show Details)
kapillamba4 edited the summary of this revision. (Show Details)
kapillamba4 edited the test plan for this revision. (Show Details)

Changed commit message

rkflx requested changes to this revision.Jun 3 2018, 10:15 PM
rkflx added a subscriber: ngraham.

@kapillamba4 Apologies for the extremely long wait, I now looked at your patch (feel free to poke other people in the future if your original reviewer is too busy…).

It seems like your patch fixes multiple things, so it would be better if you could split your patch into separate Diffs too (see also https://community.kde.org/Infrastructure/Phabricator#.22Please_do_that_in_a_separate_commit.22).


Close the previous notification before showing next notification.

Hm, I'm not sure about that. Don't you want to see multiple notifications if you took multiple screenshots?

I'd prefer not replacing the old notification, to be more in line with how users are normally expecting Plasma notifications to work.


"SkipGrouping" flags for KNotification to prevent grouping

That part looks good to me, however I believe the only thing you should set for this is the SkipGrouping flag, with no additional changes required elsewhere.

@ngraham I don't use notifications much, but I guess it would make sense not to group them here? They are only grouped by capture mode anyway, not for Spectacle as a whole. Every separate notification now would have their own thumbnail, whereas without the patch Plasma would show the thumbnail only for the last notification. Thanks for your advice!


BUG: 389999

This seems about a generic Plasma issue, and only Comment 3 is about Spectacle, i.e. showing 3 notifications for only 2 screenshots. I guess it would be better to change this to CCBUG.

However, adding Qt::UniqueConnection (which I think is the change concerning that topic?) looks a bit odd. Shouldn't we prevent calling connect multiple times in the first place? Try whether doSave(savePath) from the lines below can already send the notifications for us, depending on mNotify, and whether this will be enough in all situations.


Fix "Open" button in screenshot notification not responding to click

I don't really understand how to reproduce the problem from your test plan. Could you add detailed steps to reproduce, so I can check how it works before and after your patch? Ideally in terms of Spectacle's command line arguments, which should be similar to the global shortcuts.

This revision now requires changes to proceed.Jun 3 2018, 10:15 PM

Yeah, I think skipGrouping makes sense in Spectacle's context, but not the other flag.

It seems like your patch fixes multiple things, so it would be better if you could split your patch into separate Diffs too (see also https://community.kde.org/Infrastructure/Phabricator#.22Please_do_that_in_a_separate_commit.22).

@kapillamba4 Do you still plan on working on this, or should someone else take over? (We've just got a new bug report for one of the issues.)


"SkipGrouping" flags for KNotification to prevent grouping

That part looks good to me, however I believe the only thing you should set for this is the SkipGrouping flag, with no additional changes required elsewhere.

This might be https://bugs.kde.org/show_bug.cgi?id=396741.

@rkflx i think someone else should take over

@rkflx i think someone else should take over

Okay, thanks for letting us know. As for Bug 396741, it seems Kai already fixed it in Plasma, so nothing to do for Spectacle anymore.

kapillamba4 added a comment.EditedJul 23 2018, 10:05 AM

Fix "Open" button in screenshot notification not responding to click

I don't really understand how to reproduce the problem from your test plan. Could you add detailed steps to reproduce, so I can check how it works before and after your patch? Ideally in terms of Spectacle's command line arguments, which should be similar to the global shortcuts.

In the first screen recording you can see that open button does not work sometimes, it might be because of QTimer:singleShot

Fix "Open" button in screenshot notification not responding to click

I don't really understand how to reproduce the problem from your test plan. Could you add detailed steps to reproduce, so I can check how it works before and after your patch? Ideally in terms of Spectacle's command line arguments, which should be similar to the global shortcuts.

In the first screen recording you can see that open button does not work sometimes, it might be because of QTimer:singleShot

I cannot really see what you are doing in your video, that's why it's better to put written instructions in the test plan. I can only assume that you are using the global shortcut +Print to trigger a Fullscreen screenshot. If I activate this shortcut two times in a row and the click on Open (i.e. the first part of your video), Gwenview opens (unlike in your video). IOW, I cannot reproduce, so likely you are doing something different before or during what your video shows.

Surely there is an issue, as shown in your video, but "does not work sometimes" is not precise enough to allow me to verify the issue and the fix. Ideally there would be exact and concise steps to reproduce the problem also on my machine.

rkflx resigned from this revision.Aug 25 2018, 6:42 AM