From kde-core-devel Sat Jan 04 17:09:32 2014 From: "Rolf Eike Beer" Date: Sat, 04 Jan 2014 17:09:32 +0000 To: kde-core-devel Subject: Re: Review Request 114737: KInfocenter/OpenGL: reimplement the ReadPipe() function with QProcess Message-Id: <20140104170932.11858.65924 () probe ! kde ! org> X-MARC-Message: https://marc.info/?l=kde-core-devel&m=138885539217101 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============4046467133727548231==" --===============4046467133727548231== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114737/#review46798 ----------------------------------------------------------- Looks fine to me, although I have not tested it. One minor nitpick (that doesn't deserve a new diff): you could make the FileName argument to that function "const QString &". - Rolf Eike Beer On Jan. 4, 2014, 4:54 p.m., Wolfgang Bauer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/114737/ > ----------------------------------------------------------- > > (Updated Jan. 4, 2014, 4:54 p.m.) > > > Review request for kde-workspace and David Stephen Hubner. > > > Repository: kde-workspace > > > Description > ------- > > This patch reimplements the ReadPipe() function by using QProcess instead of popen(). > This should make it more portable. > > As a positive side-effect, this also removes those "sh: lspci: command not found." messages when run in Konsole and lspci is not in the user's path. > > This was suggested on the kde-core-devel mailinglist in November: > http://lists.kde.org/?l=kde-core-devel&m=138407113011843&w=2 > http://lists.kde.org/?l=kde-core-devel&m=138409755820003&w=2 > > > Diffs > ----- > > kinfocenter/Modules/opengl/opengl.cpp 8901957 > > Diff: https://git.reviewboard.kde.org/r/114737/diff/ > > > Testing > ------- > > Ran KInfocenter with lspci in /usr/bin/ (i.e. in the user's path) and /sbin/ (not in the user's path). The OpenGL module showed the 3D accelerator info correctly in both cases. > With lspci removed completely it showed "unknown" as expected. > > > Thanks, > > Wolfgang Bauer > > --===============4046467133727548231== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit

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

Looks fine to me, although I have not tested it.

One minor nitpick (that doesn't deserve a new diff): you could make the FileName argument to that function "const QString &".

- Rolf Eike Beer

On January 4th, 2014, 4:54 p.m. UTC, Wolfgang Bauer wrote:

Review request for kde-workspace and David Stephen Hubner.

By Wolfgang Bauer.

Updated Jan. 4, 2014, 4:54 p.m.

Repository: kde-workspace

Description

This patch reimplements the ReadPipe() function by using QProcess instead of popen().
This should make it more portable.

As a positive side-effect, this also removes those "sh: lspci: command not found." messages when run in Konsole and lspci is not in the user's path.

This was suggested on the kde-core-devel mailinglist in November:
http://lists.kde.org/?l=kde-core-devel&m=138407113011843&w=2
http://lists.kde.org/?l=kde-core-devel&m=138409755820003&w=2

Testing

Ran KInfocenter with lspci in /usr/bin/ (i.e. in the user's path) and /sbin/ (not in the user's path). The OpenGL module showed the 3D accelerator info correctly in both cases.
With lspci removed completely it showed "unknown" as expected.

Diffs

kinfocenter/Modules/opengl/opengl.cpp (8901957)

--===============4046467133727548231==--