> On Aug. 22, 2014, 1:55 p.m., David Edmundson wrote:
> > applets/clipboard/contents/ui/ClipboardItemDelegate.qml, line 73
> > <https://git.reviewboard.kde.org/r/119866/diff/1/?file=306729#file306729line73>
> >
> >     This item only contains one sub-item which fills the parent.
> >     
> >     it can be deleted, and these properties moved into the ListView

The extra item is actually used now, it contains the image, the background 
overlay for the text, and the text itself.


> On Aug. 22, 2014, 1:55 p.m., David Edmundson wrote:
> > applets/clipboard/contents/ui/ClipboardItemDelegate.qml, line 102
> > <https://git.reviewboard.kde.org/r/119866/diff/1/?file=306729#file306729line102>
> >
> >     IMHO this is clearer as a function outside Component.onCompleted.

We use this pattern pretty consistently (i.e. declaring the slot and the 
connect in the same code block). I like how it keeps pieces that belong 
together in one block without polluting the public namespace. I can move the 
whole preview-get-and-set dance into a separate function, if you prefer that?


> On Aug. 22, 2014, 1:55 p.m., David Edmundson wrote:
> > applets/clipboard/contents/ui/ClipboardItemDelegate.qml, line 94
> > <https://git.reviewboard.kde.org/r/119866/diff/1/?file=306729#file306729line94>
> >
> >     adjusting the listview to be vertically aligned would be better than 
> > each delegate.

The code to do that would be quite a bit more complicated, since we'd have to 
check which item produces the size (there's conditionals, it could be either 
the items in the list, or the buttons, or the text or the image data), 
adjusting the y position here produces both, the cleaner code and less 
codepaths to be tested.


- Sebastian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119866/#review65050
-----------------------------------------------------------


On Aug. 22, 2014, 2:08 p.m., Sebastian Kügler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119866/
> -----------------------------------------------------------
> 
> (Updated Aug. 22, 2014, 2:08 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> -------
> 
> This patch paints previews of copied images, instead of displaying the urls 
> in plain text.
> 
> The display is limited to 4 images right now. The painting of the delegates 
> is in line with the recommendations from the HIG at 
> https://techbase.kde.org/Projects/Usability/HIG/Layout/Image
> 
> It has a few rough edges:
> - not all images get thumbnails -- need to investigate why
> - vertical alignment throughout the list is quite bad, this is the case 
> already, I will fix that separately
> - I want to add an indicator that it's more than 4 files (if applicable), 
> working on that still
> - When no preview is loaded, it should show an icon, almost done, with some 
> fixes
> 
> I'd like some feedback on the code, so that with the remaining issues fixed, 
> it can get in.
> 
> 
> Diffs
> -----
> 
>   applets/clipboard/contents/ui/ClipboardItemDelegate.qml 8eecb80 
>   applets/clipboard/contents/ui/Menu.qml e6821c3 
>   applets/clipboard/contents/ui/clipboard.qml ac784d2 
>   klipper/CMakeLists.txt 660b0d1 
>   klipper/clipboardjob.h b4f5284 
>   klipper/clipboardjob.cpp d84d014 
>   klipper/historyurlitem.cpp fb2a766 
>   klipper/org.kde.plasma.clipboard.operations 813aafa 
> 
> Diff: https://git.reviewboard.kde.org/r/119866/diff/
> 
> 
> Testing
> -------
> 
> Copied images and files in Dolphin, see them showing up in Klipper. Made sure 
> that the previews are only loaded when needed, so no additional memory taken 
> when the systray / klipper popup is not open.
> 
> 
> File Attachments
> ----------------
> 
> Before
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2014/08/20/ebdde483-136e-44da-8735-f98ba88485a9__klipper-before.png
> Klipper with thumbnails
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2014/08/20/65749cc3-3fed-4c9b-9a28-b9791b8e93d8__klipper-thumbnails.png
> Klipper with thumbnails 2nd version
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2014/08/22/4fb72495-0781-4535-8de8-5266652b467a__klipper-after.png
> 
> 
> Thanks,
> 
> Sebastian Kügler
> 
>

_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Reply via email to