Re: Review Request 110138: missing checks for size(0, 0) and missing check before freeing pixmap

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110138/#review31652
---



plasma/private/dialogshadows.cpp


this looks slightly like a recursion?
(i think void DialogShadows::Private::freeX11Pixmaps() and void 
DialogShadows::Private::clearPixmaps() are twisted)


- Thomas Lübking


On April 27, 2013, 4:16 a.m., Ömer Fadıl Usta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110138/
> ---
> 
> (Updated April 27, 2013, 4:16 a.m.)
> 
> 
> Review request for kdelibs, Aaron J. Seigo, Martin Gräßlin, and Marco Martin.
> 
> 
> Description
> ---
> 
> I was getting lots of errors while moving my mouse over system tray before 
> this patch.( http://paste.kde.org/730880/ ) 
> Also this patch fixes a forgetten check before freeing a already freed pixmap
> 
> 
> Diffs
> -
> 
>   plasma/private/dialogshadows_p.h a2d62cc 
>   plasma/private/dialogshadows.cpp e787fd4 
> 
> Diff: http://git.reviewboard.kde.org/r/110138/diff/
> 
> 
> Testing
> ---
> 
> Looks like it is working fine
> 
> 
> Thanks,
> 
> Ömer Fadıl Usta
> 
>

Re: Review Request 110138: missing checks for size(0, 0) and missing check before freeing pixmap

[Ömer Fadıl Usta]

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

(Updated April 27, 2013, 11:01 a.m.)


Review request for kdelibs, Aaron J. Seigo, Martin Gräßlin, and Marco Martin.


Description (updated)
---

I was getting lots of errors while moving my mouse over system tray before this 
patch.( https://git.reviewboard.kde.org/r/110029/ ) 
Also this patch fixes a forgetten check before freeing a already freed pixmap


Diffs
-

  plasma/private/dialogshadows_p.h a2d62cc 
  plasma/private/dialogshadows.cpp e787fd4 

Diff: http://git.reviewboard.kde.org/r/110138/diff/


Testing
---

Looks like it is working fine


Thanks,

Ömer Fadıl Usta

Re: Review Request 110138: missing checks for size(0, 0) and missing check before freeing pixmap

[Ömer Fadıl Usta]

> On April 27, 2013, 10:40 a.m., Thomas Lübking wrote:
> >

that part of patch comes from here i think : 
https://git.reviewboard.kde.org/r/110029/
( it was marked as ship it )
[i will wait until it commits than i will send my part. But if you think there 
is a strange behavior on that clearPixmaps and freeX11Maps it will be much 
better to make comment in here : https://git.reviewboard.kde.org/r/110029/ )


- Ömer Fadıl


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110138/#review31652
---


On April 27, 2013, 11:01 a.m., Ömer Fadıl Usta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110138/
> ---
> 
> (Updated April 27, 2013, 11:01 a.m.)
> 
> 
> Review request for kdelibs, Aaron J. Seigo, Martin Gräßlin, and Marco Martin.
> 
> 
> Description
> ---
> 
> I was getting lots of errors while moving my mouse over system tray before 
> this patch.( https://git.reviewboard.kde.org/r/110029/ ) 
> Also this patch fixes a forgetten check before freeing a already freed pixmap
> 
> 
> Diffs
> -
> 
>   plasma/private/dialogshadows_p.h a2d62cc 
>   plasma/private/dialogshadows.cpp e787fd4 
> 
> Diff: http://git.reviewboard.kde.org/r/110138/diff/
> 
> 
> Testing
> ---
> 
> Looks like it is working fine
> 
> 
> Thanks,
> 
> Ömer Fadıl Usta
> 
>

Re: Review Request 110138: missing checks for size(0, 0) and missing check before freeing pixmap

[Thomas Lübking]

> On April 27, 2013, 7:40 a.m., Thomas Lübking wrote:
> > plasma/private/dialogshadows.cpp, line 371
> > 
> >
> > this looks slightly like a recursion?
> > (i think void DialogShadows::Private::freeX11Pixmaps() and void 
> > DialogShadows::Private::clearPixmaps() are twisted)

RR110029 was submitted a week ago.

However I don't see a self-calling freeX11Pixmaps() that does not free the 
pixmaps but clear the caches there... ;-)
(look at your patched version, lines 368-371)


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110138/#review31652
---


On April 27, 2013, 8:01 a.m., Ömer Fadıl Usta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110138/
> ---
> 
> (Updated April 27, 2013, 8:01 a.m.)
> 
> 
> Review request for kdelibs, Aaron J. Seigo, Martin Gräßlin, and Marco Martin.
> 
> 
> Description
> ---
> 
> I was getting lots of errors while moving my mouse over system tray before 
> this patch.( https://git.reviewboard.kde.org/r/110029/ ) 
> Also this patch fixes a forgetten check before freeing a already freed pixmap
> 
> 
> Diffs
> -
> 
>   plasma/private/dialogshadows_p.h a2d62cc 
>   plasma/private/dialogshadows.cpp e787fd4 
> 
> Diff: http://git.reviewboard.kde.org/r/110138/diff/
> 
> 
> Testing
> ---
> 
> Looks like it is working fine
> 
> 
> Thanks,
> 
> Ömer Fadıl Usta
> 
>

Re: Review Request 110138: missing checks for size(0, 0) and missing check before freeing pixmap

[Ömer Fadıl Usta]

> On April 27, 2013, 10:40 a.m., Thomas Lübking wrote:
> > plasma/private/dialogshadows.cpp, line 371
> > 
> >
> > this looks slightly like a recursion?
> > (i think void DialogShadows::Private::freeX11Pixmaps() and void 
> > DialogShadows::Private::clearPixmaps() are twisted)
> 
> Thomas Lübking wrote:
> RR110029 was submitted a week ago.
> 
> However I don't see a self-calling freeX11Pixmaps() that does not free 
> the pixmaps but clear the caches there... ;-)
> (look at your patched version, lines 368-371)

you are right i messed up patches :D give me 1 min.
Also i gived wrong review for checking :D
https://git.reviewboard.kde.org/r/110208/  <-- This will commit today or 
tomorrow
then i will add the otherpart on it.
But i made a mistake while passing that into mine :) sorry about this mess.


- Ömer Fadıl


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110138/#review31652
---


On April 27, 2013, 11:01 a.m., Ömer Fadıl Usta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110138/
> ---
> 
> (Updated April 27, 2013, 11:01 a.m.)
> 
> 
> Review request for kdelibs, Aaron J. Seigo, Martin Gräßlin, and Marco Martin.
> 
> 
> Description
> ---
> 
> I was getting lots of errors while moving my mouse over system tray before 
> this patch.( https://git.reviewboard.kde.org/r/110029/ ) 
> Also this patch fixes a forgetten check before freeing a already freed pixmap
> 
> 
> Diffs
> -
> 
>   plasma/private/dialogshadows_p.h a2d62cc 
>   plasma/private/dialogshadows.cpp e787fd4 
> 
> Diff: http://git.reviewboard.kde.org/r/110138/diff/
> 
> 
> Testing
> ---
> 
> Looks like it is working fine
> 
> 
> Thanks,
> 
> Ömer Fadıl Usta
> 
>

Re: Review Request 110138: missing checks for size(0, 0) and missing check before freeing pixmap

[Ömer Fadıl Usta]

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

(Updated April 27, 2013, 12:04 p.m.)


Review request for kdelibs, Aaron J. Seigo, Martin Gräßlin, and Marco Martin.


Changes
---

In this patch there is 2 patches 1 from : 
https://git.reviewboard.kde.org/r/110208/
and 1 is from mine. I have to combine 2 of them because even first one get a 
"ship it"
it didn't committed yet so i have to add it to show you complete one. Otherwise 
i need
to wait until it committed than make my diff on that one.

So mine part is almost useless but gets away lots of warnings which makes me 
sick of them.


Description
---

I was getting lots of errors while moving my mouse over system tray before this 
patch.( https://git.reviewboard.kde.org/r/110029/ ) 
Also this patch fixes a forgetten check before freeing a already freed pixmap


Diffs (updated)
-

  plasma/private/dialogshadows.cpp e787fd4 
  plasma/private/dialogshadows_p.h a2d62cc 

Diff: http://git.reviewboard.kde.org/r/110138/diff/


Testing
---

Looks like it is working fine


Thanks,

Ömer Fadıl Usta

Re: Review Request 106300: add kimgio WebP image format plugin

[Martin Koller]

> On Sept. 10, 2012, 10:48 p.m., Allan Sandfeld Jensen wrote:
> > kimgio/webp.cpp, lines 67-73
> > 
> >
> > This seems needlessly explicit, why not just copy the whole block into 
> > QImage?

QImage::scanLine docu says "the pixel format depends on the byte order on the 
underlying platform."
So to be sure to create correct rgba tuples, I explicitely pass the values to 
qRgba/qRgb


> On Sept. 10, 2012, 10:48 p.m., Allan Sandfeld Jensen wrote:
> > kimgio/webp.cpp, lines 92-100
> > 
> >
> > Same as for decoding. Why copy it color by color like this, when the 
> > formats are identical?

Same answer as above: scanLine data depends on the machines byte order, but 
WebP has always the same data representation


> On Sept. 10, 2012, 10:48 p.m., Allan Sandfeld Jensen wrote:
> > kimgio/webp.cpp, line 127
> > 
> >
> > Not really required, but it would be nice to support the Size option. 
> > It is quite often used, and should be available through WebPGetInfo()

ok, added


> On Sept. 10, 2012, 10:48 p.m., Allan Sandfeld Jensen wrote:
> > kimgio/webp.cpp, line 159
> > 
> >
> > Should it also require "VP8 " in byte 12 to 16, to protect against new 
> > unsupported versions? 
> > 
> > For some reason I have code that checks the file is atleast 32 bytes 
> > long, I think that is because it is what WePGetInfo requires.

not really. The spec says that the WebP header is 12 bytes and contains what I 
check.
There is an Extended file format which starts with a ‘VP8X’ chunk (can 
currently contain "VP8 " or "VP8L") but as I do not interpret that VP8 data, 
why restrict the reader to it? Maybe a future libwebp supports something else 
than VP8 after the WebP Header ...


- Martin


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106300/#review18829
---


On Sept. 1, 2012, 9:23 p.m., Martin Koller wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106300/
> ---
> 
> (Updated Sept. 1, 2012, 9:23 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> ---
> 
> This patch adds support for the new Google invented WebP image format. See 
> https://developers.google.com/speed/webp/?hl=ru
> 
> The patch is missing a cmake rule to make it optional, though, but I sent a 
> mail to KDE-core list in the hope someone knows...
> 
> 
> This addresses bug 267365.
> http://bugs.kde.org/show_bug.cgi?id=267365
> 
> 
> Diffs
> -
> 
>   kimgio/CMakeLists.txt 26329c0 
>   kimgio/webp.cpp PRE-CREATION 
>   kimgio/webp.desktop PRE-CREATION 
>   kimgio/webp.h PRE-CREATION 
>   mimetypes/kde.xml a82b87c 
> 
> Diff: http://git.reviewboard.kde.org/r/106300/diff/
> 
> 
> Testing
> ---
> 
> some KDE apps, including read/write with a modified kolourpaint (to be able 
> to change the quality)
> 
> 
> Thanks,
> 
> Martin Koller
> 
>

Review Request 110225: Fix KMountPoint::List::findByPath(const QString&): /books is not a sub-path of /book

[Frank Reininghaus]

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

Review request for kdelibs, David Faure and Jekyll Wu.


Description
---

The current algorithm that tries to find out what mount point a path belongs to 
only checks if the first part of the string matches the mount point. The 
problem is that /books is then considered a path inside /book, which is 
obviously wrong.

I propose to fix this by verifying that either the mount point ends with a '/', 
or the first char of the path that does not match the mount point is a '/'. 
I've factored this check out into a separate function to keep the code readable.

Many thanks to Jekyll Wu, who analyzed this bug and found the right place in 
the code.


This addresses bug 193298.
http://bugs.kde.org/show_bug.cgi?id=193298


Diffs
-

  kdecore/io/kmountpoint.cpp aa7a6b1 

Diff: http://git.reviewboard.kde.org/r/110225/diff/


Testing
---

Works for me.


Thanks,

Frank Reininghaus

Re: Review Request 106300: add kimgio WebP image format plugin

[Martin Koller]

> On April 10, 2013, 10:52 p.m., Albert Astals Cid wrote:
> > Martin, can you clarify Allan's comments?

done


- Martin


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106300/#review30892
---


On Sept. 1, 2012, 9:23 p.m., Martin Koller wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106300/
> ---
> 
> (Updated Sept. 1, 2012, 9:23 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> ---
> 
> This patch adds support for the new Google invented WebP image format. See 
> https://developers.google.com/speed/webp/?hl=ru
> 
> The patch is missing a cmake rule to make it optional, though, but I sent a 
> mail to KDE-core list in the hope someone knows...
> 
> 
> This addresses bug 267365.
> http://bugs.kde.org/show_bug.cgi?id=267365
> 
> 
> Diffs
> -
> 
>   kimgio/CMakeLists.txt 26329c0 
>   kimgio/webp.cpp PRE-CREATION 
>   kimgio/webp.desktop PRE-CREATION 
>   kimgio/webp.h PRE-CREATION 
>   mimetypes/kde.xml a82b87c 
> 
> Diff: http://git.reviewboard.kde.org/r/106300/diff/
> 
> 
> Testing
> ---
> 
> some KDE apps, including read/write with a modified kolourpaint (to be able 
> to change the quality)
> 
> 
> Thanks,
> 
> Martin Koller
> 
>