Re: Review Request 114219: Do not encode QString to QByteArray and cast it back to QString. This causes problem when there are Unicode characters in ${HOME}

[Martin Gräßlin]

> On Nov. 30, 2013, 1:45 a.m., Thomas Lübking wrote:
> > kcontrol/krdb/krdb.cpp, line 102
> > 
> >
> > QFile::encodeName() seems equal to QString::toLocal8Bit(), 
> > ::decodeName() to ::fromLocal8Bit()
> > 
> > I don't think one can just drop one of them and whether transcoding is 
> > required probably depends on what is done to the string interim.
> > 
> > If at all "KToolInvocation::klauncher()->setLaunchEnv()" would perform 
> > a second decode, so it probaly depends on what that does.
> > 
> > Was "locale charmap" determined by the reporter in the bug?
> > 
> > ---
> > 
> > Secret world domination plan:
> > --
> > #1: classified
> > #2: classified
> > #3: force ASCII as global standard
> > #4: classified
> > #5: classified
> > #6: classified
> > #7: classified
> > #8: classified
> > #9: classified
> > #a: classified
> 
> Yichao Yu wrote:
> encodeName/toLocal8Bit is used to encode a unicode string to a 
> c-string/byte-array representation and decodeName/fromLocal8Bit does the 
> reverse.
> 
> The proper decoding is already done in QFile::decodeName above and the 
> QString already has the right unicode string in it.
> 
> Basically, QString is not a wrapper of arbitrary c-string/byte-array, 
> rather a wrapper of a unicode string so whatever done to a QString before or 
> after should assume it is a valid unicode string and is independent of what 
> encoding (utf8 in the case of dbus) is needed afterward.
> 
> Encode to a byte array and cast it back can only cause wrong encoding in 
> the second conversion and will not affect what is done in setLaunchEnv.
>
> 
> Yichao Yu wrote:
> Or in another word QString has no encoding (well, by which I mean the 
> internal encoding is trasparent to the user), only byte array and c-string 
> has encoding.
>
> 
> Thomas Lübking wrote:
> QString(QByteArray) according to the API doc actually differs between Qt4 
> & 5 (fromAscii -> fromUtf8) but an encoding should not happen nevertheless 
> because:
> 
> 282 void KLauncher::setLaunchEnv(const QString &name, const QString 
> &value)
> 283 {
> 284 #ifndef USE_KPROCESS_FOR_KIOSLAVES
> 285klauncher_header request_header;
> 286QByteArray requestData;
> 287
> requestData.append(name.toLocal8Bit()).append('\0').append(value.toLocal8Bit()).append('\0');
> 
> Also QString(QByteArray) is obvisouly problematic by itself for the 
> apparent 4/5 "incompatibility".
> 
> Yichao Yu wrote:
> I guess you can also put it in this this way (setLaunchEnv have 
> toLocal8Bit inside) although I still think the simplest way is to remember 
> QString -- encode --> QByteArray, QByteArray -- decode --> QString and always 
> to the necessary explicit conversion.
> 
> That's why I hate hate hate this constructor. (and I've already fixed 3-4 
> bugs in KDE due to this constructor.) It might actually be helpful to compile 
> KDE with it commented out and replace everything with explicit conversion.
>
> 
> Yichao Yu wrote:
> I guess you can also put it in this this way (setLaunchEnv have 
> toLocal8Bit inside) although I still think the simplest way is to remember 
> QString -- encode --> QByteArray, QByteArray -- decode --> QString and always 
> to the necessary explicit conversion.
> 
> That's why I hate hate hate this constructor. (and I've already fixed 3-4 
> bugs in KDE due to this constructor.) It might actually be helpful to compile 
> KDE with it commented out and replace everything with explicit conversion.
>
> 
> Yichao Yu wrote:
> I guess you can also put it in this this way (setLaunchEnv have 
> toLocal8Bit inside) although I still think the simplest way is to remember 
> QString -- encode --> QByteArray, QByteArray -- decode --> QString and always 
> to the necessary explicit conversion.
> 
> That's why I hate hate hate this constructor. (and I've already fixed 3-4 
> bugs in KDE due to this constructor.) It might actually be helpful to compile 
> KDE with it commented out and replace everything with explicit conversion.
>
> 
> Yichao Yu wrote:
> ahh sth wrong with my network sorry for the duplicated post...

I just had a look at the history of the specific line of code and tracked it 
back to "This commit was manufactured by cvs2svn" (238ffd07).

The original line was:
QCString value = QFile::encodeName(list.join(":"));

doing several porting conversions via at one step QByteArray to finally 
QString. Given that it used to be a QByteArray the ::encodeName seems 
reasonable, but since it's QString I think that it's save to drop it.


- Martin


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

Re: KMountPoint::probablySlow and cifs mount points

[Kevin Krammer]

On Monday, 2013-11-25, 00:01:41, Mark Gaiser wrote:
> On Sun, Nov 24, 2013 at 10:09 PM, Albert Astals Cid  wrote:

> > Why? If you open a huge file by smb:// it'll copy it to the local file
> > anyway, so why should not we copy it?
> 
> Right. True but should it be like that?
> Lets take playing a movie from smb as an example. If you do that now
> in for example mplayer then kde will simply copy the movie to your
> local drive and start playing it. But should that be the case? I
> consider it a massive usability bug. One that isn't easy to fix. If
> you would mount the same share as cifs then the system "thinks" it's
> local and just plays the movie without copying.

This is more likely a problem with mplayer's .desktop file.
KIO usually hands over the URL when starting the program, unless the 
information it has about the program suggests that the program is not capable 
of handling the URL itself.
To still be able to call the program KIO then first downloads the file and 
then calls the program using the temporary file as the program's argument.
 
> That is how it should be. Copying to your local system is a nasty
> workaround which should imho be prevented.

I think it is also a difference how this is handled.
I.e. if the file is downloaded first and then read locally, then there will be 
a significant delay between starting the "open" operation and content becoming 
available to the user.

However, it might also be possible to consume received data right away and, 
additonally, save it to a local cache file in case the program needs to "go 
back".

Depending on cache policy that would even allow use cases like quickly opening 
a file again if the viewer had been closed accidentally.

Cheers,
Kevin

-- 
Kevin Krammer, KDE developer, xdg-utils developer
KDE user support, developer mentoring


signature.asc
Description: This is a digitally signed message part.

Review Request 114226: Make KLimitedIODevice::bytesAvailable return the numbers of bytesAvaliable to read

[Albert Astals Cid]

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

Review request for kdelibs and David Faure.


Bugs: 328182
http://bugs.kde.org/show_bug.cgi?id=328182


Repository: kdelibs


Description
---

At the moment KLimitedIODevice::bytesAvailable is always returning the size of 
the file, so QIODevice::atEnd never returns true because it thinks there are 
more bytes to read. This makes that if you feed an "invalid" svg like "" to the QImageReader it infinite loops believing there will be 
more stuff to read but then read() always returns 0 but then bytesAvailable 
says there are more and it stays there forever.


Diffs
-

  kdecore/io/klimitediodevice.cpp c93463b 

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


Testing
---

The svg renderer does not end loop anymore


Thanks,

Albert Astals Cid

Re: Less time for KDE... :-)

[Torgny Nyblom]

On Saturday 23 November 2013 17.58.31 Alexander Neundorf wrote:
> Hi,
> 
> for very happy personal reasons (since Tuesday, named David :-) ) I'll have
> significantly less time for KDE in the future.

>From one who know what it's like, your life will never be the same :)

Congratulations!!

/Torgny

Re: Review Request 114219: Do not encode QString to QByteArray and cast it back to QString. This causes problem when there are Unicode characters in ${HOME}

[Yichao Yu]

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

(Updated Nov. 30, 2013, 2:55 p.m.)


Review request for kde-workspace, David Faure, Martin Gräßlin, and Hugo Pereira 
Da Costa.


Bugs: 327919
http://bugs.kde.org/show_bug.cgi?id=327919


Repository: kde-workspace


Description
---

list.join already returns a QString and there is no need to encode it and cast 
back to QString again

P.S. for a patch that applies to both KDE4 and KF5(master for kde-workspace, 
frameworks for kdelibs?) How should I submit review request? Should I add both 
in branch or submit two review request? (But often the patch cannot apply 
directly due to context or file path changes).


Diffs
-

  kcontrol/krdb/krdb.cpp 92d84e9 

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


Testing (updated)
---

Compiles.
Fixes the problem here.
Also fixes the problem for the reporter.


Thanks,

Yichao Yu

Re: Review Request 114226: Make KLimitedIODevice::bytesAvailable return the numbers of bytesAvaliable to read

[David Faure]

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

Ship it!


QIODevice::bytesAvailable() probably changed meanwhile, then.

Please check that the karchive unittests still pass.

- David Faure


On Nov. 30, 2013, 6:40 p.m., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/114226/
> ---
> 
> (Updated Nov. 30, 2013, 6:40 p.m.)
> 
> 
> Review request for kdelibs and David Faure.
> 
> 
> Bugs: 328182
> http://bugs.kde.org/show_bug.cgi?id=328182
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> At the moment KLimitedIODevice::bytesAvailable is always returning the size 
> of the file, so QIODevice::atEnd never returns true because it thinks there 
> are more bytes to read. This makes that if you feed an "invalid" svg like 
> "" to the QImageReader it infinite loops believing there 
> will be more stuff to read but then read() always returns 0 but then 
> bytesAvailable says there are more and it stays there forever.
> 
> 
> Diffs
> -
> 
>   kdecore/io/klimitediodevice.cpp c93463b 
> 
> Diff: http://git.reviewboard.kde.org/r/114226/diff/
> 
> 
> Testing
> ---
> 
> The svg renderer does not end loop anymore
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

Re: Review Request 114226: Make KLimitedIODevice::bytesAvailable return the numbers of bytesAvaliable to read

[Commit Hook]

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


This review has been submitted with commit 
64a7238c3c082b174e47c3896d6eb0f5c52ab461 by Albert Astals Cid to branch 
KDE/4.11.

- Commit Hook


On Nov. 30, 2013, 6:40 p.m., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/114226/
> ---
> 
> (Updated Nov. 30, 2013, 6:40 p.m.)
> 
> 
> Review request for kdelibs and David Faure.
> 
> 
> Bugs: 328182
> http://bugs.kde.org/show_bug.cgi?id=328182
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> At the moment KLimitedIODevice::bytesAvailable is always returning the size 
> of the file, so QIODevice::atEnd never returns true because it thinks there 
> are more bytes to read. This makes that if you feed an "invalid" svg like 
> "" to the QImageReader it infinite loops believing there 
> will be more stuff to read but then read() always returns 0 but then 
> bytesAvailable says there are more and it stays there forever.
> 
> 
> Diffs
> -
> 
>   kdecore/io/klimitediodevice.cpp c93463b 
> 
> Diff: http://git.reviewboard.kde.org/r/114226/diff/
> 
> 
> Testing
> ---
> 
> The svg renderer does not end loop anymore
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

Re: Review Request 114226: Make KLimitedIODevice::bytesAvailable return the numbers of bytesAvaliable to read

[Albert Astals Cid]

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

(Updated Nov. 30, 2013, 9:44 p.m.)


Status
--

This change has been marked as submitted.


Review request for kdelibs and David Faure.


Bugs: 328182
http://bugs.kde.org/show_bug.cgi?id=328182


Repository: kdelibs


Description
---

At the moment KLimitedIODevice::bytesAvailable is always returning the size of 
the file, so QIODevice::atEnd never returns true because it thinks there are 
more bytes to read. This makes that if you feed an "invalid" svg like "" to the QImageReader it infinite loops believing there will be 
more stuff to read but then read() always returns 0 but then bytesAvailable 
says there are more and it stays there forever.


Diffs
-

  kdecore/io/klimitediodevice.cpp c93463b 

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


Testing
---

The svg renderer does not end loop anymore


Thanks,

Albert Astals Cid

Re: Review Request 112009: Update thumbnail support for Microsoft Windows executables and images, and use WINAPI when on Windows.

[Commit Hook]

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


This review has been submitted with commit 
8ce0308a24208eb22ec970e2c76eefec08d476bb by Andrius da Costa Ribas to branch 
KDE/4.12.

- Commit Hook


On Aug. 17, 2013, 1:43 p.m., Andrius da Costa Ribas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112009/
> ---
> 
> (Updated Aug. 17, 2013, 1:43 p.m.)
> 
> 
> Review request for KDE Runtime, kdewin and Pali Rohár.
> 
> 
> Repository: kde-runtime
> 
> 
> Description
> ---
> 
> This patch intends to enable Windows exe/dll thumbnailing by using winapi. It 
> derives from the unsubmitted patch from Pali Rohár from 
> https://svn.reviewboard.kde.org/r/5156/ as a starting point. I've made a few 
> adjustments on the original patch, split that patch into a common part and a 
> icoutils-specific part, and then created the winapi-based part to replace the 
> icoutils one on Windows (porting icoutils to windows wasn't going to be easy).
> 
> 
> Diffs
> -
> 
>   kioslave/thumbnail/CMakeLists.txt b81339b 
>   kioslave/thumbnail/icoutils.h 6468bc1 
>   kioslave/thumbnail/icoutils.cpp 31db85d 
>   kioslave/thumbnail/icoutils_common.cpp PRE-CREATION 
>   kioslave/thumbnail/icoutils_win.cpp PRE-CREATION 
>   kioslave/thumbnail/icoutils_wrestool.cpp PRE-CREATION 
>   kioslave/thumbnail/windowsexecreator.h a407982 
>   kioslave/thumbnail/windowsexecreator.cpp 9e24aee 
>   kioslave/thumbnail/windowsexethumbnail.desktop f10efef 
>   kioslave/thumbnail/windowsimagecreator.h 0b68cc6 
>   kioslave/thumbnail/windowsimagecreator.cpp 08b063d 
> 
> Diff: http://git.reviewboard.kde.org/r/112009/diff/
> 
> 
> Testing
> ---
> 
> Tested on a Windows 7 64-bit machine, with intel compiler (32-bit).
> Tested using ico files and both 32-bit and 64-bit executables and dlls, 
> including jumbo-size icons.
> I've used QLibrary for all winapi functions in order to avoid issues with 
> MinGW compiler, but I don't have a MinGW setup to check.
> 
> Not tested on *nix, but the original patch was not changed except for 
> iterating order in the common part and namespacing.
> 
> 
> File Attachments
> 
> 
> screenshot
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/08/11/Icons.png
> 
> 
> Thanks,
> 
> Andrius da Costa Ribas
> 
>

Re: Review Request 112009: Update thumbnail support for Microsoft Windows executables and images, and use WINAPI when on Windows.

[Andrius da Costa Ribas]

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

(Updated Dec. 1, 2013, 2:17 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Runtime, kdewin and Pali Rohár.


Repository: kde-runtime


Description
---

This patch intends to enable Windows exe/dll thumbnailing by using winapi. It 
derives from the unsubmitted patch from Pali Rohár from 
https://svn.reviewboard.kde.org/r/5156/ as a starting point. I've made a few 
adjustments on the original patch, split that patch into a common part and a 
icoutils-specific part, and then created the winapi-based part to replace the 
icoutils one on Windows (porting icoutils to windows wasn't going to be easy).


Diffs
-

  kioslave/thumbnail/CMakeLists.txt b81339b 
  kioslave/thumbnail/icoutils.h 6468bc1 
  kioslave/thumbnail/icoutils.cpp 31db85d 
  kioslave/thumbnail/icoutils_common.cpp PRE-CREATION 
  kioslave/thumbnail/icoutils_win.cpp PRE-CREATION 
  kioslave/thumbnail/icoutils_wrestool.cpp PRE-CREATION 
  kioslave/thumbnail/windowsexecreator.h a407982 
  kioslave/thumbnail/windowsexecreator.cpp 9e24aee 
  kioslave/thumbnail/windowsexethumbnail.desktop f10efef 
  kioslave/thumbnail/windowsimagecreator.h 0b68cc6 
  kioslave/thumbnail/windowsimagecreator.cpp 08b063d 

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


Testing
---

Tested on a Windows 7 64-bit machine, with intel compiler (32-bit).
Tested using ico files and both 32-bit and 64-bit executables and dlls, 
including jumbo-size icons.
I've used QLibrary for all winapi functions in order to avoid issues with MinGW 
compiler, but I don't have a MinGW setup to check.

Not tested on *nix, but the original patch was not changed except for iterating 
order in the common part and namespacing.


File Attachments


screenshot
  http://git.reviewboard.kde.org/media/uploaded/files/2013/08/11/Icons.png


Thanks,

Andrius da Costa Ribas