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

[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

[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

[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

[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 126738: Remove external dependencies from svgs

[Aleix Pol Gonzalez]

> On Jan. 14, 2016, 5:51 a.m., Dirk Hohndel wrote:
> > this will need to be done for dozens more SVGs - I sent the four that I 
> > needed for my app for review, but if this change gets accepted I'll 
> > volunteer to rewrite all of the SVGs that contain external references. It 
> > just seemed silly to spend hours doing that and then have the project 
> > reject the change :-)
> 
> andreas kainz wrote:
> what you need can it be included in 
> https://git.reviewboard.kde.org/r/126740/ than we may close this request?

My RR doesn't cover this I'm afraid. Also I'm not too sure how fast will be to 
end up adopting this.

I suggest to get this patch in (or a more complete iteration of it with the 
rest of svgs if that's possible).


- Aleix


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


On Jan. 14, 2016, 2:18 a.m., Dirk Hohndel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126738/
> ---
> 
> (Updated Jan. 14, 2016, 2:18 a.m.)
> 
> 
> Review request for KDE Frameworks, andreas kainz and Uri Herrera.
> 
> 
> Repository: breeze-icons
> 
> 
> Description
> ---
> 
> I don't know what I'm doing here. But when using the icons in a QML app I
> get a lot of warnings like this one:
> 
> Could not resolve property : linearGradient4548
> 
> Running the svgs through inkscape and using the "clean up document"
> function results in this commit (and the warnings are gone).
> 
> This may not be the right thing to do but it would be nice to get rid of
> all these warnings when using the icons.
> 
> Signed-off-by: Dirk Hohndel 
> 
> 
> Diffs
> -
> 
>   icons/actions/24/dialog-cancel.svg a2b022a9dc7b7cf97e4c3391b87e7800355d2dcb 
>   icons/actions/24/distribute-horizontal-x.svg 
> 262fcaca937cb72d8393fd5dff9c59651367fe36 
>   icons/actions/24/document-edit.svg afb37ca9624b37d6ac5aa9c94f1b2cef620faff6 
>   icons/actions/24/document-save.svg cee0a3521deb7eb1fcb79266afaa8951f4984b20 
> 
> Diff: https://git.reviewboard.kde.org/r/126738/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dirk Hohndel
> 
>

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

Re: Review Request 126740: Add a script for optimizing svgs

[Alex Richardson]

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




optimize.svg.sh (line 1)


won't work on BSD (/usr/local/bin/bash)

/usr/bin/env bash should work everywhere



optimize.svg.sh (line 4)


if command -v svgo >/dev/null; then



optimize.svg.sh (line 14)


Paths with spaces will break this.

```
find . -name "*.svg" -size 4k -print0 | while IFS= read -r -d '' file; do
svgo -i $file -o $file $ARGS
done
```

(http://mywiki.wooledge.org/BashFAQ/001)



optimize.svg.sh (line 16)


Here an everywhere else
$v -> "$v" https://github.com/koalaman/shellcheck/wiki/SC2086



optimize.svg.sh (line 20)


spaces in a parent dir will break this loop


- Alex Richardson


On Jan. 19, 2016, 10:54 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126740/
> ---
> 
> (Updated Jan. 19, 2016, 10:54 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Repository: breeze-icons
> 
> 
> Description
> ---
> 
> Dirk's review (https://git.reviewboard.kde.org/r/126738/) gave me the idea 
> that right now we're serving right away the svg's from inkscape and there's 
> room for improvement, potentially.
> 
> This patch just introduces a script that optimizes the svg's using `svgo`.
> 
> More could be done, like using gzip files, we can look into that if anyone's 
> interested. In fact, we used to use svgz for the icons, I wonder why that 
> changed. 
> 
> This will change the files in-place rather than as a build step, which is 
> what I considered first. The process to run svgo on every file was about 30 
> minutes to 1h on my system, so I doubt it's really desirable.
> 
> A reduced file size is important because it will greatly reduce disk IO, 
> which is a bottle-neck we have.
> 
> 
> Diffs
> -
> 
>   optimize.svg.sh PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126740/diff/
> 
> 
> Testing
> ---
> 
> ```
> kde-devel@oliver:~/frameworks/breeze-icons (master)$ du -sh icons icons-dark/
> 32M icons
> 32M icons-dark/
> 
> #run the script
> 
> kde-devel@oliver:~/frameworks/breeze-icons (master)$ du -sh icons icons-dark/
> 17M icons
> 17M icons-dark/
> ```
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
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

[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

[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 126738: Remove external dependencies from svgs

[andreas kainz]

> On Jan. 14, 2016, 4:51 vorm., Dirk Hohndel wrote:
> > this will need to be done for dozens more SVGs - I sent the four that I 
> > needed for my app for review, but if this change gets accepted I'll 
> > volunteer to rewrite all of the SVGs that contain external references. It 
> > just seemed silly to spend hours doing that and then have the project 
> > reject the change :-)

what you need can it be included in https://git.reviewboard.kde.org/r/126740/ 
than we may close this request?


- andreas


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


On Jan. 14, 2016, 1:18 vorm., Dirk Hohndel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126738/
> ---
> 
> (Updated Jan. 14, 2016, 1:18 vorm.)
> 
> 
> Review request for KDE Frameworks, andreas kainz and Uri Herrera.
> 
> 
> Repository: breeze-icons
> 
> 
> Description
> ---
> 
> I don't know what I'm doing here. But when using the icons in a QML app I
> get a lot of warnings like this one:
> 
> Could not resolve property : linearGradient4548
> 
> Running the svgs through inkscape and using the "clean up document"
> function results in this commit (and the warnings are gone).
> 
> This may not be the right thing to do but it would be nice to get rid of
> all these warnings when using the icons.
> 
> Signed-off-by: Dirk Hohndel 
> 
> 
> Diffs
> -
> 
>   icons/actions/24/dialog-cancel.svg a2b022a9dc7b7cf97e4c3391b87e7800355d2dcb 
>   icons/actions/24/distribute-horizontal-x.svg 
> 262fcaca937cb72d8393fd5dff9c59651367fe36 
>   icons/actions/24/document-edit.svg afb37ca9624b37d6ac5aa9c94f1b2cef620faff6 
>   icons/actions/24/document-save.svg cee0a3521deb7eb1fcb79266afaa8951f4984b20 
> 
> Diff: https://git.reviewboard.kde.org/r/126738/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dirk Hohndel
> 
>

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

Review Request 126836: DropArea: Fix correctly ignoring dragEnter event with preventStealing

[David Rosca]

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

Review request for KDE Frameworks.


Repository: kdeclarative


Description
---

temporaryInhibitParent must not be called when event is ignored.


Diffs
-

  src/qmlcontrols/draganddrop/DeclarativeDropArea.cpp 20d65e4 

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


Testing
---


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

[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

[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

[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

[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

Jenkins-kde-ci: kdeclarative master kf5-qt5 » Linux,gcc - Build # 72 - Fixed!

[no-reply]

GENERAL INFO

BUILD SUCCESS
Build URL: 
https://build.kde.org/job/kdeclarative%20master%20kf5-qt5/PLATFORM=Linux,compiler=gcc/72/
Project: PLATFORM=Linux,compiler=gcc
Date of build: Thu, 21 Jan 2016 15:07:14 +
Build duration: 1 min 57 sec

CHANGE SET
Revision b67a1ce3a14db308685f2f07ef740bb86ad48de7 by nowrep: (DragDropEvent: 
Add ignore() function)
  change: edit src/qmlcontrols/draganddrop/DeclarativeDropArea.cpp
  change: edit src/qmlcontrols/draganddrop/DeclarativeDragDropEvent.h
  change: edit src/qmlcontrols/draganddrop/DeclarativeDragDropEvent.cpp
Revision 8ec771632fa9a5a83edafcc81af1c2a8b389cc10 by nowrep: (DragArea: 
Implement grabbing delegate item)
  change: edit src/qmlcontrols/draganddrop/DeclarativeDragArea.h
  change: edit src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp


JUNIT RESULTS

Name: (root) Failed: 0 test(s), Passed: 2 test(s), Skipped: 0 test(s), Total: 2 
test(s)

COBERTURA RESULTS

Cobertura Coverage Report
  PACKAGES 5/5 (100%)FILES 19/24 (79%)CLASSES 19/24 (79%)LINE 720/1214 
(59%)CONDITIONAL 318/566 (56%)

By packages
  
autotests
FILES 7/7 (100%)CLASSES 7/7 (100%)LINE 388/470 (83%)CONDITIONAL 
192/370 (52%)
src.kdeclarative
FILES 3/4 (75%)CLASSES 3/4 (75%)LINE 153/314 (49%)CONDITIONAL 
59/98 (60%)
src.kdeclarative.private
FILES 5/5 (100%)CLASSES 5/5 (100%)LINE 24/58 (41%)CONDITIONAL 
6/10 (60%)
src.qmlcontrols.kquickcontrolsaddons
FILES 2/2 (100%)CLASSES 2/2 (100%)LINE 67/118 (57%)CONDITIONAL 
36/50 (72%)
src.quickaddons
FILES 2/6 (33%)CLASSES 2/6 (33%)LINE 88/254 (35%)CONDITIONAL 
25/38 (66%)___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Jenkins-kde-ci: kdeclarative master kf5-qt5 » Linux,gcc - Build # 72 - Fixed!

[no-reply]

GENERAL INFO

BUILD SUCCESS
Build URL: 
https://build.kde.org/job/kdeclarative%20master%20kf5-qt5/PLATFORM=Linux,compiler=gcc/72/
Project: PLATFORM=Linux,compiler=gcc
Date of build: Thu, 21 Jan 2016 15:07:14 +
Build duration: 1 min 57 sec

CHANGE SET
Revision b67a1ce3a14db308685f2f07ef740bb86ad48de7 by nowrep: (DragDropEvent: 
Add ignore() function)
  change: edit src/qmlcontrols/draganddrop/DeclarativeDropArea.cpp
  change: edit src/qmlcontrols/draganddrop/DeclarativeDragDropEvent.h
  change: edit src/qmlcontrols/draganddrop/DeclarativeDragDropEvent.cpp
Revision 8ec771632fa9a5a83edafcc81af1c2a8b389cc10 by nowrep: (DragArea: 
Implement grabbing delegate item)
  change: edit src/qmlcontrols/draganddrop/DeclarativeDragArea.h
  change: edit src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp


JUNIT RESULTS

Name: (root) Failed: 0 test(s), Passed: 2 test(s), Skipped: 0 test(s), Total: 2 
test(s)

COBERTURA RESULTS

Cobertura Coverage Report
  PACKAGES 5/5 (100%)FILES 19/24 (79%)CLASSES 19/24 (79%)LINE 720/1214 
(59%)CONDITIONAL 318/566 (56%)

By packages
  
autotests
FILES 7/7 (100%)CLASSES 7/7 (100%)LINE 388/470 (83%)CONDITIONAL 
192/370 (52%)
src.kdeclarative
FILES 3/4 (75%)CLASSES 3/4 (75%)LINE 153/314 (49%)CONDITIONAL 
59/98 (60%)
src.kdeclarative.private
FILES 5/5 (100%)CLASSES 5/5 (100%)LINE 24/58 (41%)CONDITIONAL 
6/10 (60%)
src.qmlcontrols.kquickcontrolsaddons
FILES 2/2 (100%)CLASSES 2/2 (100%)LINE 67/118 (57%)CONDITIONAL 
36/50 (72%)
src.quickaddons
FILES 2/6 (33%)CLASSES 2/6 (33%)LINE 88/254 (35%)CONDITIONAL 
25/38 (66%)___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Jenkins-kde-ci: kdeclarative master stable-kf5-qt5 » Linux,gcc - Build # 73 - Fixed!

[no-reply]

GENERAL INFO

BUILD SUCCESS
Build URL: 
https://build.kde.org/job/kdeclarative%20master%20stable-kf5-qt5/PLATFORM=Linux,compiler=gcc/73/
Project: PLATFORM=Linux,compiler=gcc
Date of build: Thu, 21 Jan 2016 15:07:14 +
Build duration: 1 min 42 sec

CHANGE SET
Revision b67a1ce3a14db308685f2f07ef740bb86ad48de7 by nowrep: (DragDropEvent: 
Add ignore() function)
  change: edit src/qmlcontrols/draganddrop/DeclarativeDragDropEvent.h
  change: edit src/qmlcontrols/draganddrop/DeclarativeDragDropEvent.cpp
  change: edit src/qmlcontrols/draganddrop/DeclarativeDropArea.cpp
Revision 8ec771632fa9a5a83edafcc81af1c2a8b389cc10 by nowrep: (DragArea: 
Implement grabbing delegate item)
  change: edit src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp
  change: edit src/qmlcontrols/draganddrop/DeclarativeDragArea.h


JUNIT RESULTS

Name: (root) Failed: 0 test(s), Passed: 2 test(s), Skipped: 0 test(s), Total: 2 
test(s)

COBERTURA RESULTS

Cobertura Coverage Report
  PACKAGES 5/5 (100%)FILES 19/24 (79%)CLASSES 19/24 (79%)LINE 720/1214 
(59%)CONDITIONAL 318/566 (56%)

By packages
  
autotests
FILES 7/7 (100%)CLASSES 7/7 (100%)LINE 388/470 (83%)CONDITIONAL 
192/370 (52%)
src.kdeclarative
FILES 3/4 (75%)CLASSES 3/4 (75%)LINE 153/314 (49%)CONDITIONAL 
59/98 (60%)
src.kdeclarative.private
FILES 5/5 (100%)CLASSES 5/5 (100%)LINE 24/58 (41%)CONDITIONAL 
6/10 (60%)
src.qmlcontrols.kquickcontrolsaddons
FILES 2/2 (100%)CLASSES 2/2 (100%)LINE 67/118 (57%)CONDITIONAL 
36/50 (72%)
src.quickaddons
FILES 2/6 (33%)CLASSES 2/6 (33%)LINE 88/254 (35%)CONDITIONAL 
25/38 (66%)___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Jenkins-kde-ci: kdeclarative master stable-kf5-qt5 » Linux,gcc - Build # 73 - Fixed!

[no-reply]

GENERAL INFO

BUILD SUCCESS
Build URL: 
https://build.kde.org/job/kdeclarative%20master%20stable-kf5-qt5/PLATFORM=Linux,compiler=gcc/73/
Project: PLATFORM=Linux,compiler=gcc
Date of build: Thu, 21 Jan 2016 15:07:14 +
Build duration: 1 min 42 sec

CHANGE SET
Revision b67a1ce3a14db308685f2f07ef740bb86ad48de7 by nowrep: (DragDropEvent: 
Add ignore() function)
  change: edit src/qmlcontrols/draganddrop/DeclarativeDragDropEvent.h
  change: edit src/qmlcontrols/draganddrop/DeclarativeDragDropEvent.cpp
  change: edit src/qmlcontrols/draganddrop/DeclarativeDropArea.cpp
Revision 8ec771632fa9a5a83edafcc81af1c2a8b389cc10 by nowrep: (DragArea: 
Implement grabbing delegate item)
  change: edit src/qmlcontrols/draganddrop/DeclarativeDragArea.cpp
  change: edit src/qmlcontrols/draganddrop/DeclarativeDragArea.h


JUNIT RESULTS

Name: (root) Failed: 0 test(s), Passed: 2 test(s), Skipped: 0 test(s), Total: 2 
test(s)

COBERTURA RESULTS

Cobertura Coverage Report
  PACKAGES 5/5 (100%)FILES 19/24 (79%)CLASSES 19/24 (79%)LINE 720/1214 
(59%)CONDITIONAL 318/566 (56%)

By packages
  
autotests
FILES 7/7 (100%)CLASSES 7/7 (100%)LINE 388/470 (83%)CONDITIONAL 
192/370 (52%)
src.kdeclarative
FILES 3/4 (75%)CLASSES 3/4 (75%)LINE 153/314 (49%)CONDITIONAL 
59/98 (60%)
src.kdeclarative.private
FILES 5/5 (100%)CLASSES 5/5 (100%)LINE 24/58 (41%)CONDITIONAL 
6/10 (60%)
src.qmlcontrols.kquickcontrolsaddons
FILES 2/2 (100%)CLASSES 2/2 (100%)LINE 67/118 (57%)CONDITIONAL 
36/50 (72%)
src.quickaddons
FILES 2/6 (33%)CLASSES 2/6 (33%)LINE 88/254 (35%)CONDITIONAL 
25/38 (66%)___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126813: Fix build with older polkit

[David Jarvie]

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



Looks good.

- David Jarvie


On Jan. 21, 2016, 4:24 p.m., Alex Richardson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126813/
> ---
> 
> (Updated Jan. 21, 2016, 4:24 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: polkit-qt-1
> 
> 
> Description
> ---
> 
> Seems like the function exists, but the header declaration is missing
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt bb91bdedc96b8211eb29a1180c2e451dc60fae18 
>   core/polkitqt1-subject.h 03028f636d7efc154138821419a704b661a7aeac 
>   core/polkitqt1-subject.cpp ecb4c0e216d5bacf5dff5cf100611b941d3e8fbd 
>   polkitqt1-config.h.cmake PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126813/diff/
> 
> 
> Testing
> ---
> 
> compiles now
> 
> 
> Thanks,
> 
> Alex Richardson
> 
>

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

Re: Deadlock in kded whith Qt 5.6

[Thiago Macieira]

On Thursday 21 January 2016 08:37:38 David Faure wrote:
> > I didn't see any "invoking message spies" of the debug I left. Are you
> > sure
> > this had the hook installed?
> 
> You mean the messageFilter? That's exactly the issue, that it's not getting
> called. Therefore autoload doesn't happen.
> 
> It might be a consequence of your previous commit (1f6fa1f37a14742dd),
> not necessarily a problem in the current patch under test.

I don't see how. The spy hooks are called before the queue would get 
suspended. We can invert the order to see if it helps, but I don't see how it 
would help or hinder. It would make the code more complex.

Can I limit the spy hook to method calls? That is, exclude signals.

> > For that matter, can you apply the attached patch so we also get feedback
> > on the queued delivery?
> 
> With this patch applied, here's one log for the first call
> (autostart+autoload, doesn't work)
> 
> http://www.davidfaure.fr/2016/first_call.txt

This one says "delivery is suspended" a couple of times. But as I said, the 
spy processing happens *before* this, with conditional:

if (Q_UNLIKELY(qDBusSpyHookList.exists()) && qApp) {

The list exists if you've installed a hook. Is it possible that this is 
happening before the QCoreApplication is created?

Note the race condition: you should install the hook *before* connecting to 
the bus. But even if that were the case, we should still be seeing the spy 
later.

> and one log for the second call (kded already started, autoload works)
> 
> http://www.davidfaure.fr/2016/second_call.txt

This one shows "invoking message spies"
-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
  PGP/GPG: 0x6EF45358; fingerprint:
  E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358

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

Re: Deadlock in kded whith Qt 5.6

[Ralf Habacker]

Am 21.01.2016 um 08:37 schrieb David Faure:

>  
> With this patch applied, here's one log for the first call 
> (autostart+autoload, doesn't work)
>
> http://www.davidfaure.fr/2016/first_call.txt
>
> and one log for the second call (kded already started, autoload works)
>
> http://www.davidfaure.fr/2016/second_call.txt
>
This patches https://codereview.qt-project.org/#/c/146767/1 and 
https://codereview.qt-project.org/#/c/146768/, which prints out message
'serial' and 'reply serial to' on qdbus side, may help to understand the
message flow more easier.

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

Re: Deadlock in kded whith Qt 5.6

[Thiago Macieira]

On Thursday 21 January 2016 08:37:38 David Faure wrote:
> On Wednesday 20 January 2016 08:41:31 Thiago Macieira wrote:
> > On Wednesday 20 January 2016 09:08:38 David Faure wrote:
> > > > Can you run with QDBUS_DEBUG=1 and tell me the messages it prints?
> > 
> > [cut]
> > 
> > I didn't see any "invoking message spies" of the debug I left. Are you
> > sure
> > this had the hook installed?
> 
> You mean the messageFilter? That's exactly the issue, that it's not getting
> called. Therefore autoload doesn't happen.

Another attempt, also untested.
-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
  PGP/GPG: 0x6EF45358; fingerprint:
  E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358
>From 10191ce9c6d31225442fde85c6d4470681d4f511 Mon Sep 17 00:00:00 2001
From: Thiago Macieira 
Date: Tue, 19 Jan 2016 22:29:33 -0800
Subject: [PATCH 1/1] Call out to QtDBus message spies in the main thread

Whenever there are spies installed, we call out to the main thread to
call to the kded/kiod message spies. This allows the spy code to do just
about anything, where previously it was restricted in what it could do
to avoid deadlocking or triggering assertions if it recursed back into
QDBusConnection code in the manager thread.

After the spies are done with, the message is re-inserted into the
QDBusConnection processing pipeline. That means it may be delayed again
if the dispatching is disabled.

Change-Id: I3d11545be52c43119f0f142b0e9d447415c2
---
 src/dbus/qdbusconnection_p.h |  3 ++-
 src/dbus/qdbusintegrator_p.h | 19 ++
 src/dbus/qdbusintegrator.cpp | 48 +---
 3 files changed, 57 insertions(+), 13 deletions(-)

diff --git a/src/dbus/qdbusconnection_p.h b/src/dbus/qdbusconnection_p.h
index f030a3f..c77daf7 100644
--- a/src/dbus/qdbusconnection_p.h
+++ b/src/dbus/qdbusconnection_p.h
@@ -1,7 +1,7 @@
 /
 **
 ** Copyright (C) 2015 The Qt Company Ltd.
-** Copyright (C) 2015 Intel Corporation.
+** Copyright (C) 2016 Intel Corporation.
 ** Contact: http://www.qt.io/licensing/
 **
 ** This file is part of the QtDBus module of the Qt Toolkit.
@@ -281,6 +281,7 @@ private slots:
 
 signals:
 void dispatchStatusChanged();
+void spyHooksFinished(const QDBusMessage );
 void messageNeedsSending(QDBusPendingCallPrivate *pcall, void *msg, int timeout = -1);
 void signalNeedsConnecting(const QString , const QDBusConnectionPrivate::SignalHook );
 bool signalNeedsDisconnecting(const QString , const QDBusConnectionPrivate::SignalHook );
diff --git a/src/dbus/qdbusintegrator_p.h b/src/dbus/qdbusintegrator_p.h
index 2038ac9..e266512 100644
--- a/src/dbus/qdbusintegrator_p.h
+++ b/src/dbus/qdbusintegrator_p.h
@@ -1,6 +1,7 @@
 /
 **
 ** Copyright (C) 2015 The Qt Company Ltd.
+** Copyright (C) 2016 Intel Corporation.
 ** Contact: http://www.qt.io/licensing/
 **
 ** This file is part of the QtDBus module of the Qt Toolkit.
@@ -65,6 +66,7 @@
 QT_BEGIN_NAMESPACE
 
 class QDBusConnectionPrivate;
+class QDBusMessage;
 
 // Really private structs used by qdbusintegrator.cpp
 // Things that aren't used by any other file
@@ -133,6 +135,23 @@ private:
 bool handled;
 };
 
+class QDBusSpyCallEvent : public QMetaCallEvent
+{
+public:
+typedef void (*Hook)(const QDBusMessage&);
+QDBusSpyCallEvent(QDBusConnectionPrivate *cp, const QDBusConnection , const QDBusMessage ,
+  const Hook *hooks, int count)
+: QMetaCallEvent(0, 0, Q_NULLPTR, cp, 0), conn(c), msg(msg), hooks(hooks), hookCount(count)
+{}
+~QDBusSpyCallEvent();
+void placeMetaCall(QObject *) Q_DECL_OVERRIDE;
+
+QDBusConnection conn;   // keeps the refcount in QDBusConnectionPrivate up
+QDBusMessage msg;
+const Hook *hooks;
+int hookCount;
+};
+
 QT_END_NAMESPACE
 
 Q_DECLARE_METATYPE(QDBusSlotCache)
diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp
index dffdfe2..ebf42b0 100644
--- a/src/dbus/qdbusintegrator.cpp
+++ b/src/dbus/qdbusintegrator.cpp
@@ -1,7 +1,7 @@
 /
 **
 ** Copyright (C) 2015 The Qt Company Ltd.
-** Copyright (C) 2015 Intel Corporation.
+** Copyright (C) 2016 Intel Corporation.
 ** Contact: http://www.qt.io/licensing/
 **
 ** This file is part of the QtDBus module of the Qt Toolkit.
@@ -120,8 +120,7 @@ void qdbusDefaultThreadDebug(int action, int condition, QDBusConnectionPrivate *
 qdbusThreadDebugFunc qdbusThreadDebug = 0;
 #endif
 
-typedef void (*QDBusSpyHook)(const QDBusMessage&);
-typedef QVarLengthArray QDBusSpyHookList;
+typedef QVarLengthArray QDBusSpyHookList;
 Q_GLOBAL_STATIC(QDBusSpyHookList, qDBusSpyHookList)
 
 extern "C" {