Bug 390864

Summary: Notifications have cropped images
Product: [Plasma] plasmashell Reporter: Vlad Zahorodnii <vlad.zahorodnii>
Component: NotificationsAssignee: Kai Uwe Broulik <kde>
Severity: normal CC: nate, notuxius, plasma-bugs, scott
Priority: NOR    
Version: 5.12.2   
Target Milestone: 1.0   
Platform: Other   
OS: Linux   
Latest Commit: Version Fixed In: 5.16.0
Attachments: A notification with cropped screen shot.
Full screen shot

Description Vlad Zahorodnii 2018-02-21 19:35:39 UTC
Created attachment 110880 [details]
A notification with cropped screen shot.

Steps to reproduce:
* take a screen shot with spectacle

* thumbnail of the screen shot with preserved aspect ratio

It makes sense to crop an image when its height is way bigger than width, but not with aspect ratios 4:3 or 16:9.
Comment 1 Vlad Zahorodnii 2018-02-21 19:36:16 UTC
Created attachment 110881 [details]
Full screen shot
Comment 2 Kai Uwe Broulik 2018-02-21 22:17:48 UTC
This was done on purpose, I didn't want padding around it, it's mostly a pretty area to grab the file for dropping it elsewhere.
Comment 3 Nate Graham 2018-02-21 22:53:27 UTC
Hmm, usability vs aesthetics. I wonder if there's a way to satisfy both sides if we put on our thinking caps.
Comment 4 Alexander Mentyu 2018-02-22 07:17:51 UTC
Perhaps an overlay with 'A screenshot was saved as ... to your Pictures folder' and top right context menu button could appear after hovering over screenshot preview image in the notification
Comment 5 Vlad Zahorodnii 2018-02-22 14:40:08 UTC
(In reply to Kai Uwe Broulik from comment #2)
> This was done on purpose, I didn't want padding around it

Images have fixed height, right? Why don't make the height dynamic and if it exceeds some threshold(like width, for example, i.e. aspect ratio < 1) then crop.
Comment 6 Scott Harvey 2018-02-27 01:45:58 UTC
I'll take a look at dynamically sizing the notification popup and/or image area based on the size of the capture. I personally feel that the whole image should be shown; otherwise it gives the impression that the screengrab was inaccurate or incomplete.
Comment 7 Kai Uwe Broulik 2019-05-09 09:33:08 UTC
Git commit 3c52fc3bc05e0f351d4998ad54edae1a4e21a043 by Kai Uwe Broulik.
Committed on 09/05/2019 at 09:29.
Pushed by broulik into branch 'master'.

Merge branch 'broulik/libnotificationmanager'

This includes:
libnotificationmanager https://phabricator.kde.org/D20265
New notification plasmoid: https://phabricator.kde.org/D20266
as well as dataengine compat: https://phabricator.kde.org/D20490 and https://phabricator.kde.org/D20491
Related: bug 222470, bug 402144, bug 405570, bug 391646, bug 401819, bug 400811, bug 392669, bug 390143, bug 374099, bug 360990, bug 346458, bug 398926, bug 390152, bug 342355, bug 402391, bug 399697, bug 400871, bug 398580
FIXED-IN: 5.16.0