Re: Review Request 126804: DragArea: Implement grabbing delegate item

2016-01-21 Thread David Rosca 


> On Jan. 21, 2016, 11:10 a.m., Sebastian Kügler wrote:
> > src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp, line 311
> > 
> >
> > So this means that we either pass an empty image, or an outdated one? 
> > I'm not entirely following the code, but I think the image should be 
> > redrawn when the drag starts, as the data may change and we definitely want 
> > to show the currently dragged data.

No, at this point, the drag should be started. But if we have delegate 
(QQuickItem), we must first render the image before starting the drag. If the 
delegate is null, we start the drag immediately and as image use either 
delegateImage or guess it from mimeData (this code path is not changed).


> On Jan. 21, 2016, 11:10 a.m., Sebastian Kügler wrote:
> > src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp, line 390
> > 
> >
> > Q_FOREACH and const&

But the value is not used here, so the plain `for` could be faster? (no 
accessing the data in container).

Originally, it was Q_FOREACH, but it looked really weird to me. But sure, if 
you think it would be better, I'll change it back.


> On Jan. 21, 2016, 11:10 a.m., Sebastian Kügler wrote:
> > src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp, line 395
> > 
> >
> > DPI-dependent, could be shared with the above

Multiply with `window()->devicePixelRatio()` ?


- David


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


On Jan. 18, 2016, 9 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126804/
> ---
> 
> (Updated Jan. 18, 2016, 9 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdeclarative
> 
> 
> Description
> ---
> 
> Implement grabbing image of delegate with QQuickItem::grabToImage.
> 
> 
> Diffs
> -
> 
>   src/qmlcontrols/draganddrop/DeclarativeDragArea.h 32092ab 
>   src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp ee78ff9 
> 
> Diff: https://git.reviewboard.kde.org/r/126804/diff/
> 
> 
> Testing
> ---
> 
> QQuickItem::grabToImage is async, not sure how safe is to delay the start of 
> drag. It seems to work without any issues though.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126804: DragArea: Implement grabbing delegate item

2016-01-21 Thread Sebastian Kügler 


> On Jan. 21, 2016, 11:10 a.m., Sebastian Kügler wrote:
> > src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp, line 395
> > 
> >
> > DPI-dependent, could be shared with the above
> 
> David Rosca wrote:
> Multiply with `window()->devicePixelRatio()` ?

Yup, that'll work.


- Sebastian


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


On Jan. 18, 2016, 9 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126804/
> ---
> 
> (Updated Jan. 18, 2016, 9 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdeclarative
> 
> 
> Description
> ---
> 
> Implement grabbing image of delegate with QQuickItem::grabToImage.
> 
> 
> Diffs
> -
> 
>   src/qmlcontrols/draganddrop/DeclarativeDragArea.h 32092ab 
>   src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp ee78ff9 
> 
> Diff: https://git.reviewboard.kde.org/r/126804/diff/
> 
> 
> Testing
> ---
> 
> QQuickItem::grabToImage is async, not sure how safe is to delay the start of 
> drag. It seems to work without any issues though.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126804: DragArea: Implement grabbing delegate item

2016-01-21 Thread Sebastian Kügler 


> On Jan. 21, 2016, 11:10 a.m., Sebastian Kügler wrote:
> > src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp, line 311
> > 
> >
> > So this means that we either pass an empty image, or an outdated one? 
> > I'm not entirely following the code, but I think the image should be 
> > redrawn when the drag starts, as the data may change and we definitely want 
> > to show the currently dragged data.
> 
> David Rosca wrote:
> No, at this point, the drag should be started. But if we have delegate 
> (QQuickItem), we must first render the image before starting the drag. If the 
> delegate is null, we start the drag immediately and as image use either 
> delegateImage or guess it from mimeData (this code path is not changed).

Ow right, delegateImage gets set by the the user? If so, the delegateImage 
should be preferred to our rendered grab, no?


- Sebastian


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


On Jan. 18, 2016, 9 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126804/
> ---
> 
> (Updated Jan. 18, 2016, 9 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdeclarative
> 
> 
> Description
> ---
> 
> Implement grabbing image of delegate with QQuickItem::grabToImage.
> 
> 
> Diffs
> -
> 
>   src/qmlcontrols/draganddrop/DeclarativeDragArea.h 32092ab 
>   src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp ee78ff9 
> 
> Diff: https://git.reviewboard.kde.org/r/126804/diff/
> 
> 
> Testing
> ---
> 
> QQuickItem::grabToImage is async, not sure how safe is to delay the start of 
> drag. It seems to work without any issues though.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126804: DragArea: Implement grabbing delegate item

2016-01-21 Thread David Rosca 


> On Jan. 21, 2016, 11:10 a.m., Sebastian Kügler wrote:
> > src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp, line 311
> > 
> >
> > So this means that we either pass an empty image, or an outdated one? 
> > I'm not entirely following the code, but I think the image should be 
> > redrawn when the drag starts, as the data may change and we definitely want 
> > to show the currently dragged data.
> 
> David Rosca wrote:
> No, at this point, the drag should be started. But if we have delegate 
> (QQuickItem), we must first render the image before starting the drag. If the 
> delegate is null, we start the drag immediately and as image use either 
> delegateImage or guess it from mimeData (this code path is not changed).
> 
> Sebastian Kügler wrote:
> Ow right, delegateImage gets set by the the user? If so, the 
> delegateImage should be preferred to our rendered grab, no?

Yes, and also `delegate` is set by user. I think we should prefer `delegate` as 
it provides the exact image as the item being grabbed. But then again, it 
doesn't really matter (though it should be noted in docs) - user can choose to 
just don't set `delegate`.


- David


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


On Jan. 18, 2016, 9 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126804/
> ---
> 
> (Updated Jan. 18, 2016, 9 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdeclarative
> 
> 
> Description
> ---
> 
> Implement grabbing image of delegate with QQuickItem::grabToImage.
> 
> 
> Diffs
> -
> 
>   src/qmlcontrols/draganddrop/DeclarativeDragArea.h 32092ab 
>   src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp ee78ff9 
> 
> Diff: https://git.reviewboard.kde.org/r/126804/diff/
> 
> 
> Testing
> ---
> 
> QQuickItem::grabToImage is async, not sure how safe is to delay the start of 
> drag. It seems to work without any issues though.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126804: DragArea: Implement grabbing delegate item

2016-01-21 Thread Sebastian Kügler 

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



Ow nice, I recall working on this during the initial Plasma5 porting and 
suitably neglected it after failing to get it to work nicely. Seems now we have 
much more reasonable Qt API to make this work nicely. (Or perhaps you're just 
smarter than I am. :))

Couple of comments inline, but looks already pretty good.


src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp (line 251)


So this means that we either pass an empty image, or an outdated one? I'm 
not entirely following the code, but I think the image should be redrawn when 
the drag starts, as the data may change and we definitely want to show the 
currently dragged data.



src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp (line 303)


This should be DPI-dependent



src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp (line 316)


Q_FOREACH and const&



src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp (line 321)


DPI-dependent, could be shared with the above


- Sebastian Kügler


On Jan. 18, 2016, 9 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126804/
> ---
> 
> (Updated Jan. 18, 2016, 9 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdeclarative
> 
> 
> Description
> ---
> 
> Implement grabbing image of delegate with QQuickItem::grabToImage.
> 
> 
> Diffs
> -
> 
>   src/qmlcontrols/draganddrop/DeclarativeDragArea.h 32092ab 
>   src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp ee78ff9 
> 
> Diff: https://git.reviewboard.kde.org/r/126804/diff/
> 
> 
> Testing
> ---
> 
> QQuickItem::grabToImage is async, not sure how safe is to delay the start of 
> drag. It seems to work without any issues though.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126804: DragArea: Implement grabbing delegate item

2016-01-21 Thread Sebastian Kügler 


> On Jan. 21, 2016, 11:10 a.m., Sebastian Kügler wrote:
> > src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp, line 390
> > 
> >
> > Q_FOREACH and const&
> 
> David Rosca wrote:
> But the value is not used here, so the plain `for` could be faster? (no 
> accessing the data in container).
> 
> Originally, it was Q_FOREACH, but it looked really weird to me. But sure, 
> if you think it would be better, I'll change it back.

Ah, right. Good point.


> On Jan. 21, 2016, 11:10 a.m., Sebastian Kügler wrote:
> > src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp, line 311
> > 
> >
> > So this means that we either pass an empty image, or an outdated one? 
> > I'm not entirely following the code, but I think the image should be 
> > redrawn when the drag starts, as the data may change and we definitely want 
> > to show the currently dragged data.
> 
> David Rosca wrote:
> No, at this point, the drag should be started. But if we have delegate 
> (QQuickItem), we must first render the image before starting the drag. If the 
> delegate is null, we start the drag immediately and as image use either 
> delegateImage or guess it from mimeData (this code path is not changed).
> 
> Sebastian Kügler wrote:
> Ow right, delegateImage gets set by the the user? If so, the 
> delegateImage should be preferred to our rendered grab, no?
> 
> David Rosca wrote:
> Yes, and also `delegate` is set by user. I think we should prefer 
> `delegate` as it provides the exact image as the item being grabbed. But then 
> again, it doesn't really matter (though it should be noted in docs) - user 
> can choose to just don't set `delegate`.

Makes sense, I'll leave it to you which one would be preferred.


- Sebastian


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


On Jan. 18, 2016, 9 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126804/
> ---
> 
> (Updated Jan. 18, 2016, 9 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdeclarative
> 
> 
> Description
> ---
> 
> Implement grabbing image of delegate with QQuickItem::grabToImage.
> 
> 
> Diffs
> -
> 
>   src/qmlcontrols/draganddrop/DeclarativeDragArea.h 32092ab 
>   src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp ee78ff9 
> 
> Diff: https://git.reviewboard.kde.org/r/126804/diff/
> 
> 
> Testing
> ---
> 
> QQuickItem::grabToImage is async, not sure how safe is to delay the start of 
> drag. It seems to work without any issues though.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126804: DragArea: Implement grabbing delegate item

2016-01-21 Thread David Rosca 

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

(Updated Jan. 21, 2016, 2:26 p.m.)


Review request for KDE Frameworks.


Changes
---

Prefer delegateImage + dpi aware image size


Repository: kdeclarative


Description
---

Implement grabbing image of delegate with QQuickItem::grabToImage.


Diffs (updated)
-

  src/qmlcontrols/draganddrop/DeclarativeDragArea.h 32092ab 
  src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp ee78ff9 

Diff: https://git.reviewboard.kde.org/r/126804/diff/


Testing
---

QQuickItem::grabToImage is async, not sure how safe is to delay the start of 
drag. It seems to work without any issues though.


Thanks,

David Rosca

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126804: DragArea: Implement grabbing delegate item

2016-01-21 Thread David Rosca 


> On Jan. 21, 2016, 2:47 p.m., Sebastian Kügler wrote:
> > src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp, line 402
> > 
> >
> > With many urls in the mimedata, this could produce a very wide pixmap. 
> > I suggest either limiting the width by eliding, or doign multiple rows. 
> > This can be done in a subsequent patch. For now, it'd be good enough IMO to 
> > break out of the loop after like 8 icons.

Ok, I'll limit it to 4 urls, so there will be at max 6 icons (in case the 
mimeData has text, html and urls).


- David


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


On Jan. 21, 2016, 2:26 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126804/
> ---
> 
> (Updated Jan. 21, 2016, 2:26 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdeclarative
> 
> 
> Description
> ---
> 
> Implement grabbing image of delegate with QQuickItem::grabToImage.
> 
> 
> Diffs
> -
> 
>   src/qmlcontrols/draganddrop/DeclarativeDragArea.h 32092ab 
>   src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp ee78ff9 
> 
> Diff: https://git.reviewboard.kde.org/r/126804/diff/
> 
> 
> Testing
> ---
> 
> QQuickItem::grabToImage is async, not sure how safe is to delay the start of 
> drag. It seems to work without any issues though.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126804: DragArea: Implement grabbing delegate item

2016-01-21 Thread David Rosca 

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

(Updated Jan. 21, 2016, 3:06 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Changes
---

Submitted with commit 8ec771632fa9a5a83edafcc81af1c2a8b389cc10 by David Rosca 
to branch master.


Repository: kdeclarative


Description
---

Implement grabbing image of delegate with QQuickItem::grabToImage.


Diffs
-

  src/qmlcontrols/draganddrop/DeclarativeDragArea.h 32092ab 
  src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp ee78ff9 

Diff: https://git.reviewboard.kde.org/r/126804/diff/


Testing
---

QQuickItem::grabToImage is async, not sure how safe is to delay the start of 
drag. It seems to work without any issues though.


Thanks,

David Rosca

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126804: DragArea: Implement grabbing delegate item

2016-01-21 Thread Sebastian Kügler 

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


Fix it, then Ship it!





src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp (line 329)


With many urls in the mimedata, this could produce a very wide pixmap. I 
suggest either limiting the width by eliding, or doign multiple rows. This can 
be done in a subsequent patch. For now, it'd be good enough IMO to break out of 
the loop after like 8 icons.


- Sebastian Kügler


On Jan. 21, 2016, 2:26 p.m., David Rosca wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126804/
> ---
> 
> (Updated Jan. 21, 2016, 2:26 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdeclarative
> 
> 
> Description
> ---
> 
> Implement grabbing image of delegate with QQuickItem::grabToImage.
> 
> 
> Diffs
> -
> 
>   src/qmlcontrols/draganddrop/DeclarativeDragArea.h 32092ab 
>   src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp ee78ff9 
> 
> Diff: https://git.reviewboard.kde.org/r/126804/diff/
> 
> 
> Testing
> ---
> 
> QQuickItem::grabToImage is async, not sure how safe is to delay the start of 
> drag. It seems to work without any issues though.
> 
> 
> Thanks,
> 
> David Rosca
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 126804: DragArea: Implement grabbing delegate item

2016-01-18 Thread David Rosca 

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

Review request for KDE Frameworks.


Repository: kdeclarative


Description
---

Implement grabbing image of delegate with QQuickItem::grabToImage.


Diffs
-

  src/qmlcontrols/draganddrop/DeclarativeDragArea.h 32092ab 
  src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp ee78ff9 

Diff: https://git.reviewboard.kde.org/r/126804/diff/


Testing
---

QQuickItem::grabToImage is async, not sure how safe is to delay the start of 
drag. It seems to work without any issues though.


Thanks,

David Rosca

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel