Add "Open containing folder" button to the notification
Needs ReviewPublic

Authored by ngraham on Sep 24 2017, 4:14 PM.

Details

Summary

Add an "Open containing folder" button to the notification that pops up when Spectacle saves a screenshot, which opens the containing folder in a new window and selects the file using KIO::highlightInFileManager()

Test Plan

Tested in KDE Neon. All functionality works.

Diff Detail

Repository
R166 Spectacle
Branch
arcpatch-D7971
Lint
No Linters Available
Unit
No Unit Test Coverage
ngraham created this revision.Sep 24 2017, 4:14 PM
ngraham edited the test plan for this revision. (Show Details)
ngraham added a project: Spectacle.
ngraham updated this revision to Diff 19870.Sep 24 2017, 4:17 PM

Updated button text to say "Show in folder" for clarity

ngraham edited the test plan for this revision. (Show Details)Sep 24 2017, 4:19 PM
ngraham retitled this revision from Add a "Show in Folder" button to the notification to Add a "Show in folder" button to the notification.Sep 24 2017, 4:21 PM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the summary of this revision. (Show Details)Sep 24 2017, 4:24 PM
broulik added a subscriber: broulik.EditedSep 24 2017, 4:38 PM

Just right click the preview in the notification. Also, there's a KIO::highlightInFileManager({url}) that does what you did and also works with e.g. Nautilus.

ngraham added a comment.EditedSep 24 2017, 4:40 PM

That's a hidden UI, though. We should endeavor to have basic features like this exposed with a visible UI. It's not like there's a lack of space in the notification that prevents adding this button.

And awesome, I'll use KIO::highlightInFileManager({url})

I'd actually rather want to get rid of the "Open" button (but for that we would need to detect if the notification server supports previews which we currently can't). And I've been through a gazillion discussions on how context menus are supposedly bad and I'm tired of that.

ngraham added a comment.EditedSep 24 2017, 4:47 PM

I'm not trying to wade into a political battle, I just want to make this feature obvious and visible, so that most users will see it. The right-click context menu is fine, but it's hidden. I don't think most people would think to right-click on the image in a notification; it's not the typical kind of UI element that triggers the "this is right-clickable!" part of your brain. Having both a hidden power-user right-click menu as well as big friendly buttons for the most basic features seems like a solution that can make everyone happy.

Or maybe even a "More Actions" button that reveals the menu.

ngraham updated this revision to Diff 19871.Sep 24 2017, 4:54 PM

Switched to using KIO::highlightInFileManager, which simplified the code quite a bit

I'll add a visible "menu" button to the notification preview just like the volume applet has for its outputs. I think this would please us both? ;)

A visible menu button would be perfect, since it would expose even more of the currently hidden features!. Maybe title it "More actions..." or something. With that, I don't think there would be any need for this patch.

So let me know when that's in and I'll happily close this revision.

ngraham edited the summary of this revision. (Show Details)Sep 24 2017, 6:51 PM

But if that may be a long time coming, could we land this in the meantime as a temporary compromise until then? When you're ready, the KNotification::action2Activated signal would already be there.

I'll try to squeeze that into Plasma 5.11, stay tuned :)

If your patch isn't going to get in soon, is there any chance we can merge now and replace it with your code when you're ready?

ngraham retitled this revision from Add a "Show in folder" button to the notification to Add a "Open containing folder" button to the notification.Oct 22 2017, 9:24 PM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)
ngraham updated this revision to Diff 21143.Oct 22 2017, 9:25 PM

Use the text "Open Containing folder" for consistency; still holding out hope for this patch :)

ngraham edited the summary of this revision. (Show Details)Oct 22 2017, 9:26 PM
ngraham retitled this revision from Add a "Open containing folder" button to the notification to Add "Open containing folder" button to the notification.
ngraham edited the summary of this revision. (Show Details)Oct 22 2017, 9:29 PM

I'm still not a huge fan of this extra long button… I have a patch for Plasma 5.12 to add a menu button that has only one remaining issue. I'll put it on Phab so we can continue from there?

Sure, let's do that!

kossebau resigned from this revision.Nov 24 2017, 11:53 PM

Has that other patch for Plasma 5.12 already been uploaded, so can this patch be marked as dropped? Resigning myself though anway, as I have no UI vision for Spectacle myself, just fixed some behaviour on filename creation.

No, it hasn't. I'd really like to at least get this in if we can't get Kai's patch in.

We now have D9255 (thanks @broulik!) which adds a hamburger menu button to the thumbnail image in the notification. I still think this patch makes sense since you get to access the function with only one click using a very obvious and visible button. If nobody agrees, I'll abandon it, but I'd like to get some more opinions first.

abetts added a subscriber: abetts.Dec 13 2017, 3:41 PM

I like it! Sometimes you don't know where your screenshots went.

D9255 has landed which I find nicer than a big fat button :/

I agree with @broulik although I personally always open the containing folder instead of opening the screenshot directly in Gwenview. But the second button just adds too much clutter to the message. On the contrary I would support removing the default action button and do this action by a left click directly on the preview. But @broulik already explained in D9255 why this might be difficult.

So -1 from me.

although I personally always open the containing folder instead of opening the screenshot directly in Gwenview

Me too. A lot of VDG people do the same. That's why I put this patch together. If this isn't the right approach, maybe we should brainstorm a better way to get the same thing. The context/hamburger menu just feels so indirect for so common an action.

Perhaps we could remove the "Open" button and have this be the only button?

ngraham added a reviewer: VDG.Dec 13 2017, 4:16 PM
fabianr added a subscriber: fabianr.Feb 8 2018, 1:16 PM

Could the buttons be placed at the bottom of the notification?
See https://community.kde.org/Plasma/Notifications#Action

rkflx added a subscriber: rkflx.Feb 12 2018, 4:45 PM

Blast from the past... friendly ping!

I still think this would be a worthwhile improvement for Spectacle's notification.

zzag added a subscriber: zzag.May 25 2018, 8:22 PM
ngraham updated this revision to Diff 35221.May 30 2018, 10:14 PM

Re-base on master

Friendly ping again! Spectacle folks, any opinions?

Of course with git master I hit https://bugs.kde.org/show_bug.cgi?id=389694 100% of the times when a notification would otherwise be shown, so for the moment it's all moot...

ngraham updated this revision to Diff 35222.May 30 2018, 10:17 PM

Revert unintentional whitespace change

rkflx added a comment.Jun 1 2018, 8:34 PM

I'd actually rather want to get rid of the "Open" button (but for that we would need to detect if the notification server supports previews which we currently can't).

+2, although more for technical reasons. This would allow us to solve 389694 and thus get rid of all sorts of issues resulting from stray Spectacle instances left running in the background, e.g. Print not working anymore.

There is a fundamental reason for that: Spectacle must quit on it's own, relying on KNotification::action1Activated to tell it to quit does not work reliably in practice (and even in theory this is not something to recommend). And before this comes up as a suggestion: No, we cannot add a timeout, because that would mean after the timeout the Open button would cease to function.

If we want to show buttons, their actions should be handled by the notification itself, without requiring a callback to Spectacle. I guess this would need changes in KNotification, e.g. something like a flag for setUrls which could indicate "show an Open button for those URLs". I don't know whether that's in conflict with any specification, though, so we may have to live with what the hamburger menu offers.

Or should we remove the "Öffnen/Open" button?

Good idea. However, the "Open" button is created by the application itself and there's no way of knowing to the application whether the notification server supports that thumbnail strip that is proprietary to Plasma and non-standard. While FDO specifies a "Capabilities" property that a client can query to see what it supports, that isn't accessible through KNotifications. In short: if we remove the "Open" button on other desktops the user won't get any way of accessing the screenshot.

Two ideas to maybe get this going again:

  • On Wayland, Spectacle will likely be tied to KWin anyway (unless there will be a generic screenshot API?). Thus, we probably can assume Plasma and either remove the button or use a Plasma-specific button.
  • We could detect Spectacle is running on Plasma, and do the same as above.

Thoughts?

Spectacle folks, any opinions?

As for the buttons in general: For me, a single Open Containing Folder button would be enough, because clicking on the preview to initiate Open seems pretty natural. However, please note that I only want to have buttons once the bug is fixed. Having no buttons would also be fine with me.

In D7971#272308, @rkflx wrote:

As for the buttons in general: For me, a single Open Containing Folder button would be enough, because clicking on the preview to initiate Open seems pretty natural.

I could live with that. We might want to make the cursor turn into a pointing hand when it's over the preview, to make clear that it'll do something on single-click.

However, please note that I only want to have buttons once the bug is fixed. Having no buttons would also be fine with me.

If the Open button is the root cause of our troubles, then by all means, let's remove it for now until and unless we can fix the issue you described above.

One-year-old ping. :) I still think this would be a nice improvement.