Re: Review Request 126161: OS X housekeeping

[David Faure]

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



src/kdeinit/kinit.cpp (line 1619)
<https://git.reviewboard.kde.org/r/126161/#comment60863>

Yes if you have to run a separate process which will then dlopen the 
kdeinit module, the whole purpose of kdeinit is moot. You might as well 
simplify your life by getting rid of most of the  Q_OS_OSX code in this file 
and simply acting as if the kdeinit module doesn't exist, on Q_OS_OSX.

The existing code to fallback to executing the binary directly will do 
exactly the same as your generic proxy, possibly even faster (no dlopen).


- David Faure


On Nov. 24, 2015, 11:03 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126161/
> ---
> 
> (Updated Nov. 24, 2015, 11:03 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: kinit
> 
> 
> Description
> ---
> 
> This patch addresses several issues with the OS X adaptations:
> 
> -1 replaces the obsolete Q_OS_MAC with Q_OS_OSX
> -2 builds the relevant applications `nongui` instead of as app bundles
> -3 turns klauncher into an "agent" by setting `LSUIElement` to true 
> programmatically
> -4 ports a patch that has been in MacPorts' `port:kdelibs4` since October 
> 14th 2009, which prevents a kdeinit crash that is caused by calling exec 
> after `fork()` in an application that has used non-POSIX APIs and/or calling 
> those APIs in the exec'ed application. This patch (originally by MacPorts 
> developers Jeremy Lainé and Jeremy Lavergne) rearranges call order and uses a 
> proxy application to do the actual exec.
> 
> 
> Diffs
> -
> 
>   src/kdeinit/CMakeLists.txt f94db71 
>   src/kdeinit/kdeinit5_proxy.cpp PRE-CREATION 
>   src/kdeinit/kinit.cpp a18008a 
>   src/klauncher/CMakeLists.txt 746edfa 
>   src/klauncher/klauncher.h e155f72 
>   src/klauncher/klauncher.cpp 8b3d343 
>   src/klauncher/klauncher_main.cpp f69aaa5 
>   src/start_kdeinit/CMakeLists.txt 46d6cb3 
> 
> Diff: https://git.reviewboard.kde.org/r/126161/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.9.5 with Qt 5.5.1 and KF5rameworks 5.16.0 . With this patch, 
> starting `kded5` will launch kdeinit5 and klauncher5 as expected, but 
> `kdeinit5 --kded` does not yet launch `kded5`. This is probably acceptable 
> for typical KF5 use on OS X (kded5 can be launched as a login item or as a 
> LaunchAgent) but I will have another look at why the kded isn't started.
> 
> I am not yet able to perform further testing; practice will for instance have 
> to show whether point 2 above needs revision (apps that need to be installed 
> as app bundles).
> 
> Similarly it will have to be seen whether point 3 above has any drawbacks. 
> Applications running as agents do not show up in the Dock or App Switcher. 
> Thus, klauncher will not be able to "turn itself into" an application that 
> does have a full GUI presence with my current modification. I don't know if 
> that's supposed to be possible though.
> NB: I have been building the KDE4 klauncher in a way that makes it impossible 
> to construct a GUI at all, so I'm not expecting issues in KF5 as long as 
> klauncher's role hasn't evolved too much.
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

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

Re: Review Request 125974: Make KTar KCompressionDevice-friendly

[David Faure]

On Tuesday 24 November 2015 23:38:28 David Faure wrote:
> On Monday 23 November 2015 12:11:12 Luiz Romário Santana Rios wrote:
> > 2015-11-21 21:02 GMT-03:00 David Faure :
> > >
> > > This is an automatically generated e-mail. To reply, visit: 
> > > https://git.reviewboard.kde.org/r/125974/
> > >
> > > On November 7th, 2015, 9:25 a.m. UTC, David Faure wrote:
> > >
> > > BTW why create a KTar on top of a KCompressionDevice? KTar is able to 
> > > handle compression automatically all by itself... and that's exactly the 
> > > case where it will use a tempfile for the uncompressed data, making 
> > > seeking work. This patch is unnecessary if you use KTar the intended way: 
> > > if the filename doesn't end with .gz or .bz2, specify the mimetype 
> > > explicitly in the KTar constructor.
> > >
> > > On November 21st, 2015, 7:11 p.m. UTC, Romário Rios wrote:
> > >
> > > KCompressionDevice is supposed to be used when you want to extract data 
> > > from a QIODevice directly instead of a file -- KTar only handles 
> > > compression automatically if you pass it a filename. ATM, 
> > > KCompressionDevice doesn't seem to work properly with many QIODevices -- 
> > > not even with QBuffer.
> > >
> > > KCompressionDevice should certainly work on top of a QBuffer. I just 
> > > committed a unittest that shows it working (but of course there might be 
> > > a bug in some other way to use it, feel free to show me in which case it 
> > > doesn't work).
> > 
> > Please take a look at my patch from the #125941 review request:
> > https://git.reviewboard.kde.org/r/125941/diff/2#2
> > It seems none of the test*BufferedNetworkReplyDevice tests work.
> 
> Ah, I see. I debugged the issue and came up with this patch:
> 
> diff --git a/src/kcompressiondevice.cpp b/src/kcompressiondevice.cpp
> index 77d7deb..aeefe6a 100644
> --- a/src/kcompressiondevice.cpp
> +++ b/src/kcompressiondevice.cpp
> @@ -196,7 +196,7 @@ bool KCompressionDevice::seek(qint64 pos)
>  return d->filter->device()->reset();
>  }
>  
> -if (ioIndex > pos) { // we can start from here
> +if (ioIndex < pos) { // we can start from here
>  pos = pos - ioIndex;
>  } else {
>  // we have to start from 0 ! Ugly and slow, but better than the 
> previous
> 
> 
> This fixes the runtime warnings but somehow openArchive returns false, I'm 
> confused as to why,
> but it's time for bed, more next time.

Found another bug in this same method, clearly KCompressionDevice::seek had 
never
been called before your unittest ;)

Here's the fix:
http://commits.kde.org/karchive/b31b66f4ac0f0941fbf784cc6aeba7190dbd78bf
Now the *Buffered methods of your unittests pass. Can you commit that unittest,
at least the *Buffered methods? Don't forget to add a license+copyright header.
Ah and rename these methods to remove "NetworkReply" from their name,
they really test KCompressionDevice with QBuffer, there's no networkreply in 
the mix.

Now that things work with a QBuffer, we can talk about QNetworkReply again :)

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 126164: Request proper dbus name for kioexec

[David Faure]

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

Ship it!


"in kde" -> you meant in kde4 (kde-runtime4 more precisely).

This is a valid fix. We could also look at making kinit not require dbus 
registration from kioexec; usually kinit gets this information from the desktop 
file of the app, but for kioexec there's no desktop file, so it must be 
hardcoded somewhere.

- David Faure


On Nov. 25, 2015, 8:20 a.m., Xuetian Weng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126164/
> ---
> 
> (Updated Nov. 25, 2015, 8:20 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 353037
> https://bugs.kde.org/show_bug.cgi?id=353037
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Dolphin make a synchronous calls to klauncher and klauncher asks kdeinit to 
> start kioexec. The call is only replied when kioexec's dbus name appear on 
> the dbus.
> 
> Possibly in kde it uses kapplication and kdbusservice name is not added when 
> porting to qt5/kf5.
> 
> 
> Diffs
> -
> 
>   src/kioexec/CMakeLists.txt 91284a3 
>   src/kioexec/main.cpp 68ed374 
> 
> Diff: https://git.reviewboard.kde.org/r/126164/diff/
> 
> 
> Testing
> ---
> 
> Dolphin now does not freeze in this case.
> 
> 
> Thanks,
> 
> Xuetian Weng
> 
>

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

Re: Review Request 126161: OS X housekeeping

[David Faure]

> On Nov. 25, 2015, 7:39 a.m., David Faure wrote:
> > src/kdeinit/kinit.cpp, line 1621
> > <https://git.reviewboard.kde.org/r/126161/diff/1/?file=418134#file418134line1621>
> >
> > Yes if you have to run a separate process which will then dlopen the 
> > kdeinit module, the whole purpose of kdeinit is moot. You might as well 
> > simplify your life by getting rid of most of the  Q_OS_OSX code in this 
> > file and simply acting as if the kdeinit module doesn't exist, on Q_OS_OSX.
> > 
> > The existing code to fallback to executing the binary directly will do 
> > exactly the same as your generic proxy, possibly even faster (no dlopen).
> 
> René J.V. Bertin wrote:
> You're undoubtedly right, I considered doing this myself. It would amount 
> to making the `--no-fork` option the default, no?
> 
> I don't feel comfortable/ready for that right now, without having had a 
> working equivalent to (the patched) kdeinit4 out there in the wild for 
> observation. Can we leave your suggestion for a 2nd round of housekeeping and 
> cleanup?

Not at all, --no-fork is about kdeinit's own startup, not about the way it 
starts other applications.

In general I don't see much purpose in pushing complex code if we confirm it to 
serve no purpose at all.
But I looked a bit further into it, and in fact kinit's launch() does 
fork+dlopen or fork+exec, depending on whether the kdeinit module was found.

So if fork + exec is a problem on OSX, then indeed that needs to be addressed
But if your patch does fork + exec_helper + dlopen, then that is unnecessarily 
complex.

The simple version of what I have in mind is 
http://www.davidfaure.fr/2015/kinit.osx.cpp.diff i.e. never using QLibrary on 
OSX. Would that work?


- David


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


On Nov. 24, 2015, 11:03 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126161/
> ---
> 
> (Updated Nov. 24, 2015, 11:03 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: kinit
> 
> 
> Description
> ---
> 
> This patch addresses several issues with the OS X adaptations:
> 
> -1 replaces the obsolete Q_OS_MAC with Q_OS_OSX
> -2 builds the relevant applications `nongui` instead of as app bundles
> -3 turns klauncher into an "agent" by setting `LSUIElement` to true 
> programmatically
> -4 ports a patch that has been in MacPorts' `port:kdelibs4` since October 
> 14th 2009, which prevents a kdeinit crash that is caused by calling exec 
> after `fork()` in an application that has used non-POSIX APIs and/or calling 
> those APIs in the exec'ed application. This patch (originally by MacPorts 
> developers Jeremy Lainé and Jeremy Lavergne) rearranges call order and uses a 
> proxy application to do the actual exec.
> 
> 
> Diffs
> -
> 
>   src/kdeinit/CMakeLists.txt f94db71 
>   src/kdeinit/kdeinit5_proxy.cpp PRE-CREATION 
>   src/kdeinit/kinit.cpp a18008a 
>   src/klauncher/CMakeLists.txt 746edfa 
>   src/klauncher/klauncher.h e155f72 
>   src/klauncher/klauncher.cpp 8b3d343 
>   src/klauncher/klauncher_main.cpp f69aaa5 
>   src/start_kdeinit/CMakeLists.txt 46d6cb3 
> 
> Diff: https://git.reviewboard.kde.org/r/126161/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.9.5 with Qt 5.5.1 and KF5rameworks 5.16.0 . With this patch, 
> starting `kded5` will launch kdeinit5 and klauncher5 as expected, but 
> `kdeinit5 --kded` does not yet launch `kded5`. This is probably acceptable 
> for typical KF5 use on OS X (kded5 can be launched as a login item or as a 
> LaunchAgent) but I will have another look at why the kded isn't started.
> 
> I am not yet able to perform further testing; practice will for instance have 
> to show whether point 2 above needs revision (apps that need to be installed 
> as app bundles).
> 
> Similarly it will have to be seen whether point 3 above has any drawbacks. 
> Applications running as agents do not show up in the Dock or App Switcher. 
> Thus, klauncher will not be able to "turn itself into" an application that 
> does have a full GUI presence with my current modification. I don't know if 
> that's supposed to be possible though.
> NB: I have been building the KDE4 klauncher in a way that makes it impossible 
> to construct a GUI at all, so I'm not expecting issues in KF5 as long as 
> klauncher's role hasn't evolved too much.
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

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

Re: Review Request 125974: Make KTar KCompressionDevice-friendly

[David Faure]

On Wednesday 25 November 2015 12:21:11 Luiz Romário Santana Rios wrote:
> 
> Btw, the commit 0f0230f7d2feeca7ed00072e7b17b24c14f53698 ("Fix clang
> warnings") makes the compilation fail in my machine. In the
> KGzipFilter::setInBuffer() method, you change a C-cast to (Bytef *) to
> a reinterpret_cast, but you're attributing it to
> d->zStream.next_in, which is a char *. To make it work, I had to
> change it to const_cast(reinterpret_cast *>(data)). Is there a better way to do this.

d->zStream.next_in is not a char * when ZLIB_CONST is defined.

My /usr/include/zconf.h says

#if defined(ZLIB_CONST) && !defined(z_const)
#  define z_const const
#else
#  define z_const
#endif

and /usr/include/zlib.h says
 z_const Bytef *next_in;  

That's zlib-devel-1.2.8, what's your zlib version?
I saw other projects defining ZLIB_CONST when googling so I assumed
it had been there for a very very long time, but maybe I'm wrong about that.
In that case I'll revert that const stuff indeed.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 126161: OS X housekeeping

[David Faure]

> On Nov. 25, 2015, 7:39 a.m., David Faure wrote:
> > src/kdeinit/kinit.cpp, line 1621
> > <https://git.reviewboard.kde.org/r/126161/diff/1/?file=418134#file418134line1621>
> >
> > Yes if you have to run a separate process which will then dlopen the 
> > kdeinit module, the whole purpose of kdeinit is moot. You might as well 
> > simplify your life by getting rid of most of the  Q_OS_OSX code in this 
> > file and simply acting as if the kdeinit module doesn't exist, on Q_OS_OSX.
> > 
> > The existing code to fallback to executing the binary directly will do 
> > exactly the same as your generic proxy, possibly even faster (no dlopen).
> 
> René J.V. Bertin wrote:
> You're undoubtedly right, I considered doing this myself. It would amount 
> to making the `--no-fork` option the default, no?
> 
> I don't feel comfortable/ready for that right now, without having had a 
> working equivalent to (the patched) kdeinit4 out there in the wild for 
> observation. Can we leave your suggestion for a 2nd round of housekeeping and 
> cleanup?
> 
> David Faure wrote:
> Not at all, --no-fork is about kdeinit's own startup, not about the way 
> it starts other applications.
> 
> In general I don't see much purpose in pushing complex code if we confirm 
> it to serve no purpose at all.
> But I looked a bit further into it, and in fact kinit's launch() does 
> fork+dlopen or fork+exec, depending on whether the kdeinit module was found.
> 
> So if fork + exec is a problem on OSX, then indeed that needs to be 
> addressed
> But if your patch does fork + exec_helper + dlopen, then that is 
> unnecessarily complex.
> 
> The simple version of what I have in mind is 
> http://www.davidfaure.fr/2015/kinit.osx.cpp.diff i.e. never using QLibrary on 
> OSX. Would that work?
> 
> René J.V. Bertin wrote:
> Well, all I can say is that with that patch nothing crashes, `kdeinit5 
> --kded` still launches kded5 but as a result I now no longer see something 
> like (in `ps` output)
> 
> ```
> 12980 1 400c   0  33  0  2510184   6716 -  Ss 
>  0 ?? 0:00.03 /opt/local/bin/kdeinit5 --suicide --nofork
> 12981 12980 4004   0  48  0  2641864  14856 -  S  
>  0 ?? 0:00.12 /opt/local/libexec/kde5/kf5/klauncher --fd=9 
> libkdeinit5_klauncher
> ```
> 
> but
> 
> ```
> 13211 1 400c   0  33  0  2527592   6724 -  Ss 
>  0 ?? 0:00.02 /opt/local/bin/kdeinit5 --suicide --nofork
> 13225  1076 4006   0  31  0  2432948576 -  S+ 
>  0 ttys0040:00.00 fgrep kdeinit5
> ```
> 
> And `kwrapper5 /path/to/kwrite` now longer launches an kwrite process 
> that can be killed via `killall kwrite` or equivalent. All that is probably 
> to be expected, but that latter observation does feel like a regression of 
> sorts to me.
> 
> 
> BTW, I noticed that kded5 will have to be turned into an agent too, it 
> has no business showing up in the Dock.

Yes, killall only works on linux because of the proc_settitle stuff.

I think my approach would "fix" that "regression" for killall kwrite because it 
would be a real fork'ed+exec'ed process.

You are seeing the drawbacks of the kdeinit mechanism (e.g. killall, and 
probably what the `ps` entry looks like for kwrite, too) without benefiting 
from its advantages (faster startup), if you have to go through a helper 
process.


- David


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


On Nov. 25, 2015, 4:19 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126161/
> ---
> 
> (Updated Nov. 25, 2015, 4:19 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: kinit
> 
> 
> Description
> ---
> 
> This patch addresses several issues with the OS X adaptations:
> 
> -1 replaces the obsolete Q_OS_MAC with Q_OS_OSX
> -2 builds the relevant applications `nongui` instead of as app bundles
> -3 turns klauncher into an "agent" by setting `LSUIElement` to true 
> programmatically
> -4 ports a patch that has been in MacPorts' `port:kdelibs4` since October 
> 1

Re: Review Request 125974: Make KTar KCompressionDevice-friendly

[David Faure]

On Wednesday 25 November 2015 14:54:49 Luiz Romário Santana Rios wrote:
> >
> > My /usr/include/zconf.h says
> >
> > #if defined(ZLIB_CONST) && !defined(z_const)
> > #  define z_const const
> > #else
> > #  define z_const
> > #endif
> 
> Mine doesn't define anything like that.
> 
> >
> > and /usr/include/zlib.h says
> >  z_const Bytef *next_in;
> 
> Mine says:
> typedef struct z_stream_s {
> Bytef*next_in;  /* next input byte */
> 
> >
> > That's zlib-devel-1.2.8, what's your zlib version?
> 
> Mine is 1.2.3.4.

OK, fix committed. 
http://commits.kde.org/karchive/e0875ab83a93f225c7af1285960017dd1933

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 125941: Add KCompressionDevice tests to KArchive

[David Faure]

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

Ship it!


I would have used less member variables in the unittest, but ok nevermind, it's 
just a test.

Thanks for the test, let's have it in.


autotests/CMakeLists.txt (line 6)
<https://git.reviewboard.kde.org/r/125941/#comment60877>

This should probably be made optional, it's an unexpected dependency for 
KArchive.



autotests/kcompressiondevicetest.cpp (line 32)
<https://git.reviewboard.kde.org/r/125941/#comment60878>

not really needed for this QBuffer-based test, but ok, it's just a first 
step towards making QNetworkReply work, let's commit what works now.


- David Faure


On Nov. 25, 2015, 3:12 p.m., Romário Rios wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125941/
> ---
> 
> (Updated Nov. 25, 2015, 3:12 p.m.)
> 
> 
> Review request for KDE Frameworks, Aleix Pol Gonzalez and David Faure.
> 
> 
> Repository: karchive
> 
> 
> Description
> ---
> 
> I recently noticed that using KTar with KCompressionDevice and QNetworkReply 
> did not work, so I'm adding some tests to confirm that
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 1b2e819 
>   autotests/kcompressiondevicetest.h PRE-CREATION 
>   autotests/kcompressiondevicetest.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/125941/diff/
> 
> 
> Testing
> ---
> 
> The tests run and show that KCompressionDevice works properly with QBuffer 
> when used with KTar
> 
> 
> Thanks,
> 
> Romário Rios
> 
>

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

Re: Review Request 125941: Add KCompressionDevice tests to KArchive

[David Faure]

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



autotests/CMakeLists.txt (line 44)
<https://git.reviewboard.kde.org/r/125941/#comment60880>

sorry I just realized something else: these calls to "tar" will break on 
Windows.

I would suggest to either
1) add small archives to git, or
2) create those from code using KArchive

Of course there's also easy option 3: find_program(TAR tar)
if (Qt5Network_FOUND AND TAR)
...
and then using ${TAR} instead of tar, to be consistent.



autotests/CMakeLists.txt (line 53)
<https://git.reviewboard.kde.org/r/125941/#comment60881>

this one should be in
if (LIBLZMA_FOUND)



autotests/kcompressiondevicetest.cpp (line 135)
<https://git.reviewboard.kde.org/r/125941/#comment60882>

#if HAVE_XZ_SUPPORT
...
#else
    QSKIP("...")
#endif


- David Faure


On Nov. 25, 2015, 9:19 p.m., Romário Rios wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125941/
> ---
> 
> (Updated Nov. 25, 2015, 9:19 p.m.)
> 
> 
> Review request for KDE Frameworks, Aleix Pol Gonzalez and David Faure.
> 
> 
> Repository: karchive
> 
> 
> Description
> ---
> 
> I recently noticed that using KTar with KCompressionDevice and QNetworkReply 
> did not work, so I'm adding some tests to confirm that
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 1b2e819 
>   autotests/kcompressiondevicetest.h PRE-CREATION 
>   autotests/kcompressiondevicetest.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/125941/diff/
> 
> 
> Testing
> ---
> 
> The tests run and show that KCompressionDevice works properly with QBuffer 
> when used with KTar
> 
> 
> Thanks,
> 
> Romário Rios
> 
>

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

Re: Review Request 125941: Add KCompressionDevice tests to KArchive

[David Faure]

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



autotests/CMakeLists.txt (line 44)
<https://git.reviewboard.kde.org/r/125941/#comment60884>

Oh wow cmake integrates code for this based on libarchive, I didn't know 
that.

I stand corrected.


- David Faure


On Nov. 25, 2015, 9:19 p.m., Romário Rios wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125941/
> ---
> 
> (Updated Nov. 25, 2015, 9:19 p.m.)
> 
> 
> Review request for KDE Frameworks, Aleix Pol Gonzalez and David Faure.
> 
> 
> Repository: karchive
> 
> 
> Description
> ---
> 
> I recently noticed that using KTar with KCompressionDevice and QNetworkReply 
> did not work, so I'm adding some tests to confirm that
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 1b2e819 
>   autotests/kcompressiondevicetest.h PRE-CREATION 
>   autotests/kcompressiondevicetest.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/125941/diff/
> 
> 
> Testing
> ---
> 
> The tests run and show that KCompressionDevice works properly with QBuffer 
> when used with KTar
> 
> 
> Thanks,
> 
> Romário Rios
> 
>

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

Re: Failure while executing KTar::open while using KCompressionDevice as the device

[David Faure]

On Saturday 21 November 2015 15:50:31 Luiz Romário Santana Rios wrote:
> , or make the waitFor* calls and warn the user
> that passing a QIODevice which is not yet fully ready to
> KCompressionDevice might make KTar::open() block.

That would work I guess. In a unittest or a non-gui tool
or a secondary thread, blocking might be fine.
I assume by "warn" you mean in the documentation, not at runtime.

> A suggestion I made in the IRC channel was this: add a constructor
> argument -- or a flag -- called "Buffered" or "Cached"; if enabled,
> the data stream from the QIODevice would be buffered or copied to a
> tempfile, then that buffer or file would be extracted; if disabled,
> the QIODevice would be accessed directly for its data; then, make it
> enabled by default.

This all sounds quite complex compared to just waitFor*, no?
The buffering is already done by QNetworkReply, I don't see much point
in adding another buffer on top.

> Another thing: KCompressionDevice only seems to support tar files
> (.gz, .bz2, and .xz), so why would KZip be an issue anyway?

KZip uses KCompressionDevice internally, to decode zlib-encoded data.
But I lost context of why we were talking about this.
It's indeed different because it's not "KZip on top of a compressed QIODevice",
that wouldn't make sense indeed.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 126161: OS X housekeeping

[David Faure]

On Wednesday 25 November 2015 16:45:25 René J.V. Bertin wrote:
> 
> No, with "my" fix, applications started through kwrapper appear as individual 
> entries in `ps` listings, with your fix only the `kwrapper5 /path/to/command` 
> entry shows up.

I don't see how that's possible.
If kdeinit forks, surely you see a separate process for that.
kwrapper merely sends a message to kdeinit, asking it to execute the 
application, it has no code to execute the app itself.
Are you sure you're not missing an entry in the ps list?

> Also, if your fix does a "real fork + exec", how come it doesn't run afoul of 
> the limitations imposed on that on OS X?

I don't understand this reasoning. Your patch does fork+exec too, except that 
it executes the helper executable
(which then loads the kdeinit module) instead of executing the kwrite 
executable. I don't see how that makes any difference to OSX ?

> Only because it doesn't actually load `l` (the result of QLibrary(libpath)), 
> meaning the crash will return the day kdeinit5 itself does something 
> off-limits? The helper from my fix is probably much less likely to develop 
> such symptoms. And even if it does (through Qt) it would be much easier to 
> cure (make it not use Qt at all but only POSIX APIs).

I don't understand the difference between "execute kwrite" and "execute a proxy 
executable that dlopens kwrite".

> For the rest, using a helper does seem to give a better "emulation" of what 
> `kdeinit5` does on Linux, no?

Not at all, kdeinit on linux does fork+dlopen, no exec.
But my point is exactly that: if fork+dlopen is a problem on OSX, then don't do 
it, do fork+exec. That's what you do,
but then why exec something that will dlopen, instead of exec the real thing?

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 125988: [KUrlCompletion] Add autocompletion for '.' input which offers all hidden files/folders

[David Faure]

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



src/widgets/kurlcompletion.cpp (line 531)
<https://git.reviewboard.kde.org/r/125988/#comment60885>

are you sure this hunk is necessary?
QUrl has no special handling of "." in paths.
So m_fileName = m_kurl.fileName() would also just set it to ".", like your 
special code does.

(and if that's right then I don't see the purpose of the two new vars...)

Wasn't only the last hunk of the patch that really made a difference?


- David Faure


On Nov. 9, 2015, 8:30 a.m., Emmanuel Pescosta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125988/
> ---
> 
> (Updated Nov. 9, 2015, 8:30 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 354981
> https://bugs.kde.org/show_bug.cgi?id=354981
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Currently KUrlCompletion only offers autocompletion for hidden folders when 
> you input at least one additional character after the dot. 
> With this patch all hidden folders will be offered when only a dot is present.
> 
> This behaviour is test covered.
> 
> 
> Diffs
> -
> 
>   autotests/kurlcompletiontest.cpp ca8563c 
>   src/widgets/kurlcompletion.cpp c6764e4 
> 
> Diff: https://git.reviewboard.kde.org/r/125988/diff/
> 
> 
> Testing
> ---
> 
> All test cases for KUrlCompletion pass
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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

Re: Review Request 126189: Support https and other URL schemas for "home page" property in KAboutData constructor.

[David Faure]

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


Most of this looks good, but I'm not sure about the "2 components" parts of it. 
 www.mydomain.co.uk would lead to "co.uk" ?
I'd suggest not limiting to two, it's unrelated to the bug you're fixing anyway

- David Faure


On Nov. 27, 2015, 11:14 p.m., Michael Pyne wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126189/
> ---
> 
> (Updated Nov. 27, 2015, 11:14 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 355508
> https://bugs.kde.org/show_bug.cgi?id=355508
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> This commit permits URLs such as "https://www.foo.org"; (i.e. with non-http 
> schemas), which bug 355508 points out is something we don't currently 
> support, and expands the test suite to ensure http and https URLs give the 
> same behavior when deriving desktop file names and organization domains.
> 
> The existing code seems to operate the stripping the first URL host component 
> (e.g. www.foo.org -> foo.org) for the purposes of generating an organization 
> domain or desktop file name. I'm not sure if that was intentional or not, but 
> I found it easier to just limit to two components instead (e.g. 
> www.product.foo.org -> foo.org). Everything else should be pretty 
> straightforward for review I think... much of the code change is simply 
> because I tried to fix by making the current code scheme-independent instead 
> of just adding a '|| scheme == "https"'.
> 
> I also modified the API docs to reflect the change. I will add the 
> appropriate @since based on when I get a Ship It! ;)
> 
> 
> Diffs
> -
> 
>   autotests/kaboutdatatest.cpp 96b3a13 
>   src/lib/kaboutdata.h e9fc56b 
>   src/lib/kaboutdata.cpp de19e6f 
> 
> Diff: https://git.reviewboard.kde.org/r/126189/diff/
> 
> 
> Testing
> ---
> 
> All tests pass in kaboutdatatest, including the extra test cases added to 
> test https:// URLs specifically.
> 
> I also had hacked in some qDebug()s and ran with several KDE apps, all of 
> which generated reasonable organization domains.
> 
> 
> Thanks,
> 
> Michael Pyne
> 
>

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

Re: QSP patch/activator (Review Request 126125: [OS X] make KDE's trash use the OS X trash)

[David Faure]

On Saturday 21 November 2015 17:17:49 René J.V. Bertin wrote:
> That would mean having 2 different Qt5 installs in MacPorts; one for pure Qt 
> applications that are expected to behave "natively"

I still have to see proof that a "pure Qt application" installed with MacPorts
really cares about where QSP points to - apart from the obvious migration issue
if this ever changes.
I keep having the feeling that the simplest solution is to consider MacPorts
a different "platform" than OSX, in terms of paths. For sure the install path of
an app installed via MacPorts will be something like /opt/local while a real
OSX Qt app wouldn't go there. So can't it live with settings in ~/.config and
data files in ~/.local/share etc.?

Otherwise, the next best idea is to get ECM to add your activator to all link 
lines
automatically, e.g. by adding it to CMAKE_EXE_LINKER_FLAGS or whatever.
This for sure beats editing every link line, or trying to guess which frameworks
are going to be "used by everyone".

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Failure while executing KTar::open while using KCompressionDevice as the device

[David Faure]

On Monday 23 November 2015 12:54:01 Luiz Romário Santana Rios wrote:
>  QNetworkReply is a sequential device.

Yes, that's the problem.

> This, as I
> understand it, is only a problem because KCompressionDevice is able to
> open any file it wants in any sequence

No, not KCompressionDevice, but KTar.
Remove compression from the equation to simplify reasoning, think about
a foo.tar download. Each file in the KTar object (after openArchive) can provide
a "limited io device" which allows the app to read from byte N to byte M in the 
.tar.
This kind of API is just fully incompatible with a sequential underlying device.

KCompressionDevice sits between KTar and the underlying device,
and looks at the compressed tar file as one file (remember, .tar.gz is one
compressed .tar file, not a tar of compressed files). Due to the way
zlib/bzip2 works it has to restart from zero if you seek backwards, so 
it introduces a performance issue, but it's not the one making this fully
impossible. The KArchiveFile api is the "issue", if the underlying device
is sequential.

> after all, tars are
> serializable, which means there's no need to seek back to extract a
> stream of data. 

Wrong, when providing a flexible API like "now read this file"
(as you would do when opening files in folders, nothing forces you
to read them in a specific order).

> Knowing that, I can think of two solutions:  [...]

kcompressiondevice isn't where the main problem is. The two solutions are:

1) document that KArchive does not work with sequential devices, i.e. force
the app to put the result into a QBuffer or temp file
2) handle sequential devices automatically within KArchive by creating a 
QBuffer or tempfile automatically.

Option 2 is nicer to app developers, but makes KArchive a bit too complex to my
taste. Plus, one could argue that app developers are in a better position to 
decide
whether a QBuffer is OK (reasonable size) or a temp file is necessary (huge 
files) ?

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 126161: OS X housekeeping

[David Faure]

On Thursday 26 November 2015 10:27:31 René J.V. Bertin wrote:
> Yes, the *helper* does that, from within the newly exec'ed process. It's 
> weird, but apparently the exact "forbidden" thing is "fork - call/load 
> non-POSIX APIs - exec" while "fork - exec - call/load non-POSIX APIs" works.

That confirms my point. fork+dlopen is forbidden, while fork+exec is OK -- 
whether that means exec kwrite or exec the helper proxy.

> >I don't understand the difference between "execute kwrite" and "execute a 
> >proxy executable that dlopens kwrite".
> 
> Maybe the above helps a wee bit?

No it doesn't. It's fork+exec in both cases, both should work.

> Ah. Indeed, we have NOT tried the simple fork+dlopen approach, for some 
> reason (your patch skips the `l.load()` step).

Don't, it's the thing that kdeinit was doing initially (since it's what it does 
on linux, whenever possible, i.e. when a .so is found) and which created 
problems on OSX.

> What's the difference between starting kwrite directly on the commandline (or 
> through execve()), and dlopen'ing it? 

On Linux: it skips the cost of relocations which happen when starting a new 
binary. kdeinit already links to the major shared
libraries, so it's faster to fork and dlopen.

On OSX: it falls into the forbidden case.

> ... if kdeinit5 and/or kwrapper5 are supposed to be able to launch() shared 
> libraries containing kdeinitmain or kdemain.

You seem to be missing one point: every application which provides a .so 
("kdeinit module") *also* provides a binary, always.
The kdeinit module is only an optimization. Every app needs to have a standard 
executable, for other means of starting it.
The entry point into kdeinit is "start kwrite please". kdeinit decides whether 
to do that using the kdeinit module (.so) or the executable.
So it's perfectly fine to only support executables. That's also what happens on 
Windows.
I never realized you could pass a shared lib to kwrapper5, that is definitely 
not the intended usage, and I can tell you, nobody does this ;)

 > That appears to be the case indeed: `kwrapper5 /path/to/kwrite` is faster 
 > than `kwrapper5 /path/to/libkdeinit4_kwrite.dylib`.
 > A bit surprising, because someone still has to open that dylib even when 
 > exec'ing `kwrite` ... am I missing something?

Yes but I bet it means some symbol lookups happen at runtime rather than at 
link time. I'm not enough of a lowlevel
mac toolchain expert to give a fully satisfying explanation, but I'm not very 
surprised.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 126161: OS X housekeeping

[David Faure]

On Tuesday 01 December 2015 09:53:06 René J.V. Bertin wrote:
> > The entry point into kdeinit is "start kwrite please". kdeinit decides 
> > whether to do that using the kdeinit module (.so) or the executable.
> 
> So what you're saying is that we should be fine (by design) if I remove the 
> bit where kdeinit checks to see if it should use the kdeinit module?
> For now I've only been looking at launch(), observed it can be called with a 
> shared library and then ensured that that'd work on OS X too.
> I suppose what you are implying is that I should look up the caller path to 
> see where kdeinit verifies if there is a kdeinit module for the tool it has 
> been requested to start, and disable that part?

Yes - which is exactly what my suggested patch does, AFAICS.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 126189: Support https and other URL schemas for "home page" property in KAboutData constructor.

[David Faure]

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

Ship it!


Ship It!

- David Faure


On Dec. 2, 2015, 3:51 a.m., Michael Pyne wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126189/
> ---
> 
> (Updated Dec. 2, 2015, 3:51 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 355508
> https://bugs.kde.org/show_bug.cgi?id=355508
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> ---
> 
> This commit permits URLs such as "https://www.foo.org"; (i.e. with non-http 
> schemas), which bug 355508 points out is something we don't currently 
> support, and expands the test suite to ensure http and https URLs give the 
> same behavior when deriving desktop file names and organization domains.
> 
> The existing code seems to operate the stripping the first URL host component 
> (e.g. www.foo.org -> foo.org) for the purposes of generating an organization 
> domain or desktop file name. I'm not sure if that was intentional or not, but 
> I found it easier to just limit to two components instead (e.g. 
> www.product.foo.org -> foo.org). Everything else should be pretty 
> straightforward for review I think... much of the code change is simply 
> because I tried to fix by making the current code scheme-independent instead 
> of just adding a '|| scheme == "https"'.
> 
> I also modified the API docs to reflect the change. I will add the 
> appropriate @since based on when I get a Ship It! ;)
> 
> 
> Diffs
> -
> 
>   src/lib/kaboutdata.h e9fc56b 
>   src/lib/kaboutdata.cpp de19e6f 
>   autotests/kaboutdatatest.cpp 96b3a13 
> 
> Diff: https://git.reviewboard.kde.org/r/126189/diff/
> 
> 
> Testing
> ---
> 
> All tests pass in kaboutdatatest, including the extra test cases added to 
> test https:// URLs specifically.
> 
> I also had hacked in some qDebug()s and ran with several KDE apps, all of 
> which generated reasonable organization domains.
> 
> 
> Thanks,
> 
> Michael Pyne
> 
>

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

Re: Review Request 126170: [OS X] make kded5 an agent, and build it as a regular application instead of an app bundle

[David Faure]

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


Please kind in mind that kded must be able to pop up dialogs, though.
(cookie dialog, SSL cert messagebox + dialog, etc. etc.).

If making it an "agent" doesn't prevent it from showing GUI elements now and 
then, then no problem.

- David Faure


On Nov. 25, 2015, 6:12 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126170/
> ---
> 
> (Updated Nov. 25, 2015, 6:12 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: kded
> 
> 
> Description
> ---
> 
> There should be no reason to build `kded5` as an app bundle on OS X, and with 
> recent feedback in mind I postulated that marking it "nongui" 
> (`ecm_mark_nongui_application`) would be acceptable on other platforms too.
> 
> Additionally, `kded5` doesn't have any more reason to appear in the Dock or 
> app switcher, on OS X, so I have added the code to make it an agent.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 4b9a5ff 
>   src/CMakeLists.txt 5e95df8 
>   src/kded.cpp 6929d7d 
> 
> Diff: https://git.reviewboard.kde.org/r/126170/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.9.5 with Qt 5.5.1 and FWs 5.16.0 .
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

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

Re: Review Request 126149: [Icon widget] Bring back properties dialog

[David Faure]

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



applets/icon/plugin/icon_p.cpp (line 203)
<https://git.reviewboard.kde.org/r/126149/#comment60948>

PreferLocalFile is only good for displaying to the user. Otherwise you have 
something which is "a path or a URL", and most API doesn't work with that.



applets/icon/plugin/icon_p.cpp (line 205)
<https://git.reviewboard.kde.org/r/126149/#comment60949>

This only works with local files, so it should be in a if 
(m_url.isLocalFile()) block, and use m_url.toLocalFile(). The current code 
would break on Windows at least, and possibly in case of filenames with '#'.

Put the result of toLocalFile() in a temporary QString, you'll need further 
down as well.



applets/icon/plugin/icon_p.cpp (line 211)
<https://git.reviewboard.kde.org/r/126149/#comment60950>

Yes, don't ;)

Actually this is a local-to-local copy, you could just use QFile::copy if 
you want it synchronous.



applets/icon/plugin/icon_p.cpp (line 239)
<https://git.reviewboard.kde.org/r/126149/#comment60951>

why commented out?


- David Faure


On Nov. 26, 2015, 8:15 p.m., Kai Uwe Broulik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126149/
> ---
> 
> (Updated Nov. 26, 2015, 8:15 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Bugs: ?
> https://bugs.kde.org/show_bug.cgi?id=?
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> ---
> 
> This brings back the KPropertiesDialog to modify an icon's appearance. This 
> has been requested at multiple occasions. This has been adapted from the 
> Plasma 4 icon code.
> 
> 
> Diffs
> -
> 
>   applets/icon/package/contents/ui/main.qml 9286b94 
>   applets/icon/plugin/icon_p.h dd7963c 
>   applets/icon/plugin/icon_p.cpp e086870 
> 
> Diff: https://git.reviewboard.kde.org/r/126149/diff/
> 
> 
> Testing
> ---
> 
> Dropped a file from my home onto the desktop -> dialog from the actual file, 
> allowing to rename it. The applet reflected the changes.
> 
> Dropped an application from kickoff to the desktop -> dialog from a copy of 
> the desktop file, allowing to change its icon and description. The applet 
> reflected the changes.
> 
> 
> Thanks,
> 
> Kai Uwe Broulik
> 
>

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

Re: Updating techbase wiki page according to KF5 policies

[David Faure]

On Tuesday 01 December 2015 22:36:52 Alex Merry wrote:
> On 2015-12-01 15:34, Martin Walch wrote:
> > On Saturday, November 14, 2015 06:08:55 PM Alex Merry wrote:
> >> On 2015-11-14 01:21, Martin Walch wrote:
> >> > Alright, so I have created a stub at
> >> >
> >> > https://techbase.kde.org/Policies/Frameworks_Coding_Style
> >> >
> >> > ...
> >> >
> >> > Qt includes
> >> > * For Qt #includes omit the module name and only use the class name.
> >> 
> >> These are all still applicable.
> > 
> > Does the same pattern apply to Frameworks includes?
> > For example:
> > 
> > KFileItem instead of KIOCore/KFileItem
> > and
> > KIO/HostInfo instead of KIOCore/KIO/HostInfo
> > 
> > and of course not something like kfileitem.h or kio/hostinfo.h
> 
> Yes, although good luck trying to explain that, given the inconsistency 
> between  and 

This is not inconsistent at all. The first one is about the class KIO::HostInfo,
the second one is about the class KFileItem.
The header file is consistent with the class that it defines.

>  is only 
> possible by fluke, really, as a side effect of where we install the 
> version headers.

Well it's the same as , i.e. an include that starts
with the module name. But for the same reasons as in Qt, this is not 
recommended.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 126245: Cookie dialogue: make Accept/Reject buttons work, and other fixes

[David Faure]

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

Ship it!


Yep, looks 100% correct.

I'm surprised that there are still people using this dialog though, I was 
considering removing it :-). The default settings (for many years) are to not 
show this dialog, I assume you change the settings so that it is shown?

- David Faure


On Dec. 4, 2015, 5:16 p.m., Jonathan Marten wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126245/
> ---
> 
> (Updated Dec. 4, 2015, 5:16 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> The "You received a cookie from..." dialogue appears, but there are a number 
> of things that do not work as intended.  This patch corrects them.
> 
> 1. The first two buttons were presumably intended to be "Accept" and "Reject" 
> as for KDE4, but they actually said "Reject" and "No".  This was a simple 
> cut/paste error.
> 
> 2. Clicking either of these buttons did nothing.  They needed to be connected 
> to accept() and reject() in order to make the exec() called from 
> KCookieServer::checkCookies() return a result.
> 
> 3. The "Set or modify the cookie information" button text was too wide, 
> making the dialogue width far wider than needed for the cookie information.  
> The dialogue looks better with this changed back to "Details" (with the same 
> icon as for KDE4) with the full text in a tooltip.
> 
> 4. The state of the hide/show details was not being saved correctly.  Using 
> !isHidden() instead of isVisible() gets the correct information.
> 
> 
> Diffs
> -
> 
>   src/ioslaves/http/kcookiejar/kcookiewin.cpp 56a283f 
> 
> Diff: https://git.reviewboard.kde.org/r/126245/diff/
> 
> 
> Testing
> ---
> 
> Built kio with these changes, checked operation of the cookie dialogue in 
> Konqueror.
> 
> 
> Thanks,
> 
> Jonathan Marten
> 
>

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

Re: KNotifications filters

[David Faure]

On Thursday 24 September 2015 20:35:24 Jeremy Whiting wrote:
>  If so I guess the logical place for it would be a TTS group in
> kdeglobals, does that make sense?
 
Stay away from kdeglobals :-)
It forces every app to parse that stuff even if they don't need it.

Framework-specific config can go into a framework-specific config file,
it doesn't have to be kdeglobals.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 125762: External extractor plugin support for KFileMetaData

[David Faure]

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

Ship it!


another round of low-level code review; but I'd say you can push afterwards, 
given the lack of objections on the overall mechanism.


src/extractors/externalextractor.cpp (line 85)
<https://git.reviewboard.kde.org/r/125762/#comment61010>

m_extractors(),  would do the same, no need to copy from a 
default-constructed instance.



src/extractors/externalextractor.cpp (line 86)
<https://git.reviewboard.kde.org/r/125762/#comment61009>

this is used only here in this method, so it doesn't need to be a member 
variable



src/extractors/externalextractor.cpp (line 91)
<https://git.reviewboard.kde.org/r/125762/#comment61012>

const QString &extractor



src/extractors/externalextractor.cpp (line 94)
<https://git.reviewboard.kde.org/r/125762/#comment61011>

maybe with a timeout? + error handling?


- David Faure


On Oct. 24, 2015, 12:19 p.m., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125762/
> ---
> 
> (Updated Oct. 24, 2015, 12:19 p.m.)
> 
> 
> Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: kfilemetadata
> 
> 
> Description
> ---
> 
> This patch introduces support for external metadata extractors in 
> KFileMetaData
> 
> The external extractors themselves can be written in any language, provided 
> that it can be executed as a standalone executable (compiled or script with a 
> hashbang), with command line arguments, and can output data to stdout.
> 
> The extractors are executed like so:
> 
> * `extractor --mimetypes` - outputs a list of mimetypes supported by the 
> extractor, one per line.
> * `extractor filename` - outputs a json document with the metadata. The keys 
> are such that they can be directly used with PropertyInfo::fromName().
> 
> At the KFileMetaData end, an additional internal plugin (ExternalExtractor) 
> is provided that forms a conduit between external extractors and the internal 
> API. This plugin looks for executables called 
> kfilemetadata_extractor_ in /usr/bin to find external extractors, 
> and executes them with the --mimetypes arg to find the list of mimetypes each 
> extractor supports. ExternalExtractor then claims to support all of these 
> mimetypes, and then delegates to the extractor executable when doing the 
> actual extraction.
> 
> 
> Diffs
> -
> 
>   README.md 19b1a26 
>   src/extractors/CMakeLists.txt 5dd223e 
>   src/extractors/externalextractor.h PRE-CREATION 
>   src/extractors/externalextractor.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/125762/diff/
> 
> 
> Testing
> ---
> 
> Tested with the sample executable file extractor (as attched, written in 
> python) with the dump manual test in KFileMetaData. Works.
> 
> 
> File Attachments
> 
> 
> kfilemetadata_extractor_executable
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2015/10/23/146b657f-31d9-4117-a82f-ef966a6339d4__kfilemetadata_extractor_executable
> 
> 
> Thanks,
> 
> Boudhayan Gupta
> 
>

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

Re: Review Request 125800: OS X build and warning fix

[David Faure]

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

Ship it!


Ship It!

- David Faure


On Oct. 27, 2015, 11:01 p.m., Samuel Gaist wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125800/
> ---
> 
> (Updated Oct. 27, 2015, 11:01 p.m.)
> 
> 
> Review request for KDE Frameworks and Ivan Romanov.
> 
> 
> Repository: qca
> 
> 
> Description
> ---
> 
> The cmake find file was missing, but CoreFoundation being a system
> framework, no need to search for it.
> 
> OS X: Update code to not use deprecated functions/data structure
> 
> CSSM_DATA and SecCertificateGetData have been deprecated since 10.7.
> This patch uses SecCertificateCopyData which is the official
> replacement.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt dbce08272deb284f3cb0fd09ccfec4ab93ce3e23 
>   src/CMakeLists.txt a60bda0bf3d8bad7523ee7d50798c8cdb6c2eccb 
>   src/qca_systemstore_mac.cpp 9349c90c8258d3d1f00a3796d93f94c09f5b1f2a 
> 
> Diff: https://git.reviewboard.kde.org/r/125800/diff/
> 
> 
> Testing
> ---
> 
> Build on OS X 10.8
> 
> Check that qcatool-qt5 keystore list-stores returns the same results
> 
> 
> Thanks,
> 
> Samuel Gaist
> 
>

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

Re: Review Request 125885: Support socks5 proxy in KTcpSocket

[David Faure]

> On Nov. 2, 2015, 8:08 a.m., David Faure wrote:
> > Ah I see, forget what I said about HTTP proxy.
> > 
> > About the missing argument, it would indeed be useful for this code:
> > http://lxr.kde.org/source/extragear/network/konversation/src/irc/server.cpp
> > Add a separate method, for BC reasons.
> 
> Xuetian Weng wrote:
> After you mentioned it, I actually test the http proxy (proxy server I 
> used is polipo) and pure tcp, and it does support tcp connection over http 
> proxy.
> 
> I'd like to be more careful for other users of ktcpsocket in ioslaves.
> 
> KTcpSocket does not know the protocol that this TcpSocket will use. But 
> http ioslave uses setApplicationProxy to set the proxy. Use connectionHost 
> with policy = AutoProxy may make http uses socks instead http proxy by 
> default. Same applies to ftp ioslave and maybe others ioslave use 
> TCPSlaveBase.
> 
> I wonder if there should also be some protocol hint argument of 
> connectHost, or http and ftp ioslave should use policy = ManualProxy.

I lost track a little bit about all this; feel free to push this and/or make an 
alternative patch ;)


- David


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


On Oct. 30, 2015, 11:26 p.m., Xuetian Weng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125885/
> -------
> 
> (Updated Oct. 30, 2015, 11:26 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 342402
> https://bugs.kde.org/show_bug.cgi?id=342402
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Automatically set socks5 proxy in KTcpSocket.
> 
> 
> Diffs
> -
> 
>   src/core/ktcpsocket.h ffa3f0b 
>   src/core/ktcpsocket.cpp fde35a7 
> 
> Diff: https://git.reviewboard.kde.org/r/125885/diff/
> 
> 
> Testing
> ---
> 
> Test with akonadi imap agent, connect through socks5 proxy.
> 
> 
> Thanks,
> 
> Xuetian Weng
> 
>

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

Re: Review Request 125988: [KUrlCompletion] Add autocompletion for '.' input which offers all hidden files/folders

[David Faure]

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


OK I debugged this further, and I found why this didn't match my expectations 
of the QUrl behaviour:

QUrl::fromLocalFile("/tmp/.") leads to file:///tmp rather than file:///tmp/. I 
consider this a bug (when NormalizePathSegments isn't set), I'll discuss it 
with Thiago and possibly fix it in Qt.

- David Faure


On Nov. 28, 2015, 9:36 p.m., Emmanuel Pescosta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125988/
> ---
> 
> (Updated Nov. 28, 2015, 9:36 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 354981
> https://bugs.kde.org/show_bug.cgi?id=354981
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Currently KUrlCompletion only offers autocompletion for hidden folders when 
> you input at least one additional character after the dot. 
> With this patch all hidden folders will be offered when only a dot is present.
> 
> This behaviour is test covered.
> 
> 
> Diffs
> -
> 
>   autotests/kurlcompletiontest.cpp ca8563c 
>   src/widgets/kurlcompletion.cpp c6764e4 
> 
> Diff: https://git.reviewboard.kde.org/r/125988/diff/
> 
> 
> Testing
> ---
> 
> All test cases for KUrlCompletion pass
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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

Re: Review Request 125988: [KUrlCompletion] Add autocompletion for '.' input which offers all hidden files/folders

[David Faure]

> On Dec. 6, 2015, 11:19 a.m., David Faure wrote:
> > OK I debugged this further, and I found why this didn't match my 
> > expectations of the QUrl behaviour:
> > 
> > QUrl::fromLocalFile("/tmp/.") leads to file:///tmp rather than 
> > file:///tmp/. I consider this a bug (when NormalizePathSegments isn't set), 
> > I'll discuss it with Thiago and possibly fix it in Qt.

Urgh, it's my own commit aba336c2b4ad8926dc8a000718bbb7f8a6d5a72d in qurl.cpp 
...


- David


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


On Nov. 28, 2015, 9:36 p.m., Emmanuel Pescosta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125988/
> ---
> 
> (Updated Nov. 28, 2015, 9:36 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 354981
> https://bugs.kde.org/show_bug.cgi?id=354981
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Currently KUrlCompletion only offers autocompletion for hidden folders when 
> you input at least one additional character after the dot. 
> With this patch all hidden folders will be offered when only a dot is present.
> 
> This behaviour is test covered.
> 
> 
> Diffs
> -
> 
>   autotests/kurlcompletiontest.cpp ca8563c 
>   src/widgets/kurlcompletion.cpp c6764e4 
> 
> Diff: https://git.reviewboard.kde.org/r/125988/diff/
> 
> 
> Testing
> ---
> 
> All test cases for KUrlCompletion pass
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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

Re: Review Request 125988: [KUrlCompletion] Add autocompletion for '.' input which offers all hidden files/folders

[David Faure]

> On Dec. 6, 2015, 11:19 a.m., David Faure wrote:
> > OK I debugged this further, and I found why this didn't match my 
> > expectations of the QUrl behaviour:
> > 
> > QUrl::fromLocalFile("/tmp/.") leads to file:///tmp rather than 
> > file:///tmp/. I consider this a bug (when NormalizePathSegments isn't set), 
> > I'll discuss it with Thiago and possibly fix it in Qt.
> 
> David Faure wrote:
> Urgh, it's my own commit aba336c2b4ad8926dc8a000718bbb7f8a6d5a72d in 
> qurl.cpp ...

Suggested fix posted to https://codereview.qt-project.org/143134


- David


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


On Nov. 28, 2015, 9:36 p.m., Emmanuel Pescosta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125988/
> -------
> 
> (Updated Nov. 28, 2015, 9:36 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 354981
> https://bugs.kde.org/show_bug.cgi?id=354981
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Currently KUrlCompletion only offers autocompletion for hidden folders when 
> you input at least one additional character after the dot. 
> With this patch all hidden folders will be offered when only a dot is present.
> 
> This behaviour is test covered.
> 
> 
> Diffs
> -
> 
>   autotests/kurlcompletiontest.cpp ca8563c 
>   src/widgets/kurlcompletion.cpp c6764e4 
> 
> Diff: https://git.reviewboard.kde.org/r/125988/diff/
> 
> 
> Testing
> ---
> 
> All test cases for KUrlCompletion pass
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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

Re: Review Request 126038: Add protocol info to KIO plugin metadata

[David Faure]

> On Nov. 12, 2015, 4:12 p.m., Christoph Cullmann wrote:
> > Hi,
> > 
> > I did the same in https://git.reviewboard.kde.org/r/125869/ but I still 
> > need to fix the i18n issue, first :/
> 
> Andrew McCann wrote:
> Ahh, indeed you did.
> 
> Should I abandon this? or do I 'add' it to yours? somehow.. I'm new to 
> reviewboard.

If what you did is all included in Christoph's request, you can discard this 
one.


- David


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


On Nov. 12, 2015, 12:59 p.m., Andrew McCann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126038/
> ---
> 
> (Updated Nov. 12, 2015, 12:59 p.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Cullmann.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Add protocol info to KIO plugin metadata.  This is necessary to get KDevelop 
> to work on OSX in an app bundle.
> 
> This continues work done by Christoph Cullmann in 
> https://git.reviewboard.kde.org/r/125830/
> 
> Followed the same pattern as the aforementioned diff.
> 
> protocol.json files were generated using the protocoltojson utility.
> 
> 
> Diffs
> -
> 
>   src/ioslaves/file/file.cpp 381e4885bf0e78ceb4b06f1f1657ad4068923a84 
>   src/ioslaves/file/file.json PRE-CREATION 
>   src/ioslaves/ftp/ftp.cpp 2179179e966d97ce24784292ef67812fa1d3361d 
>   src/ioslaves/ftp/ftp.json PRE-CREATION 
>   src/ioslaves/help/ghelp.json PRE-CREATION 
>   src/ioslaves/help/help.json PRE-CREATION 
>   src/ioslaves/help/main.cpp 9939196b4fc4aefd52c2c13717609181429fe891 
>   src/ioslaves/help/main_ghelp.cpp 59c85587af5edf767261a6b3ca66e6f37efecd29 
>   src/ioslaves/trash/kio_trash.cpp 81cc0361d22606f54d31e6ce743cd5fe818a4701 
>   src/ioslaves/trash/kio_trash_win.cpp 
> 9dd857f3dd9aebaef9e8d9e5adfc30adb0c1ae6b 
>   src/ioslaves/trash/trash.json PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126038/diff/
> 
> 
> Testing
> ---
> 
> With this patch applied, my development version of kdevelop on OSX was able 
> to find:
> (logging statements were added by me, but not in this diff)
> 
> ```
> { Found Plugin path  
> "/Users/mccann/src/kde_playground/kdevelop_bundle_dest/Applications/kdevelop.app/Contents/PlugIns/kf5/kio/file.so"
>protocol contains  "file"
> }
> { Found Plugin path  
> "/Users/mccann/src/kde_playground/kdevelop_bundle_dest/Applications/kdevelop.app/Contents/PlugIns/kf5/kio/ftp.so"
>protocol contains  "ftp"
> }
> { Found Plugin path  
> "/Users/mccann/src/kde_playground/kdevelop_bundle_dest/Applications/kdevelop.app/Contents/PlugIns/kf5/kio/ghelp.so"
>protocol contains  "ghelp"
> }
> { Found Plugin path  
> "/Users/mccann/src/kde_playground/kdevelop_bundle_dest/Applications/kdevelop.app/Contents/PlugIns/kf5/kio/help.so"
>protocol contains  "help"
> }
> { Found Plugin path  
> "/Users/mccann/src/kde_playground/kdevelop_bundle_dest/Applications/kdevelop.app/Contents/PlugIns/kf5/kio/http.so"
>protocol contains  "http"
>protocol contains  "https"
>protocol contains  "webdav"
>protocol contains  "webdavs"
> }
> { Found Plugin path  
> "/Users/mccann/src/kde_playground/kdevelop_bundle_dest/Applications/kdevelop.app/Contents/PlugIns/kf5/kio/trash.so"
>protocol contains  "trash"
> }
> ```
> 
> 
> Thanks,
> 
> Andrew McCann
> 
>

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

Re: about QSP::RuntimeLocation and XDG_RUNTIME_DIR on OS X/MacPorts

[David Faure]

On Monday 23 November 2015 17:32:28 René J. V. Bertin wrote:
> Hi,
> 
> Freedesktop/XDG-compliant applications may use a user-specific location for 
> runtime "stuff" like sockets. On Linux, this location usually is something 
> like 
> /run/user/uid. It is typically defined globally through the XDG_RUNTIME_DIR 
> env. 
> variable, with a fallback if that variable doesn't exist.
> 
> I'm not certain if MacPorts has some kind of policy for handling this 
> particular 
> location. The only beginning of an answer I have for now is the presence of a 
> few ~/.cache/keyring-X directories, which may correspond to the similarly 
> named keyring directories in /run/user/uid on my Linux rig.
> 
> Context: Qt5's QStandardPaths::RuntimeLocation return value. It defaults to 
> ~/Library/Application Support, which I think is fine for "Apple native mode" 
> but 
> not for an XDG-compliant mode like the one I intend to provide.
> Alternatives I see:
> - $TMPDIR/runtime-username (calculated using the QSP RuntimeLocation code 
> used 
> on other Unix hosts)
> - ~/.cache (to align with what Gnome *may* use).
> 
> I tend to prefer the 1st alternative to stick closest to what happens on 
> Linux, 
> but only if Qt5-based and non-Qt5-based applications and libraries need NOT 
> agree on the location.
> 
> Thoughts?

I don't think this reasoning is right. All you need is a directory matching the
requirements (owned by the user, 0700 permissions, and on a filesystem
that supports sockets). It really doesn't matter what the path to that 
directory is,
and it definitely doesn't matter if the app is in "XDG" or "native" mode, unless
you can find an example where a Qt-based app and a non-Qt-based app
communicate over a socket in $XDG_RUNTIME_DIR, on Linux.

Let's see in KDE Frameworks, RuntimeLocation is used for
- the kdeinit socket
- the kdesud socket
- the kio app/slave socket
- the socket between kio_http and kio_http_cache_cleaner.

As I suspected, none of this needs interoperability with the rest of the XDG 
world.

So it doesn't matter where these sockets are put on Mac OS X, as long
as the security requirements are met.
Therefore "~/Library/Application Support" seems fine to me.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: OS X : org.kde.klauncher5 was not provided by any .service files

[David Faure]

On Tuesday 24 November 2015 00:27:01 René J.V. Bertin wrote:
> 
> "KLauncher could not be reached via D-Bus. Error when calling 
> kdeinit_exec_wait:\nThe name org.kde.klauncher5 was not provided by any 
> .service files\n"

kdeinit5 is supposed to fork and exec klauncher5 on startup, and then 
klauncher5 registers to DBus.

Ignore the mention of ".service files" that's DBus' way of saying "this process 
is not running, so I tried to autostart it and didn't find how to".
But in this case, we don't use autostart, so the problem is "not running".

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 126170: [OS X] make kded5 an agent, and build it as a regular application instead of an app bundle

[David Faure]

> On Dec. 2, 2015, 7:51 a.m., David Faure wrote:
> > Please kind in mind that kded must be able to pop up dialogs, though.
> > (cookie dialog, SSL cert messagebox + dialog, etc. etc.).
> > 
> > If making it an "agent" doesn't prevent it from showing GUI elements now 
> > and then, then no problem.
> 
> René J.V. Bertin wrote:
> With the earlier approach of setting `LSUIElement` that is guaranteed to 
> be the case.
> 
> I just checked; launching Qt's Assistant with 
> `QT_MAC_DISABLE_FOREGROUND_APPLICATION_TRANSFORM=1` all that changes is that 
> the application remains in the background; it can be brought into the 
> foreground, and it retains its presence in the Dock and app switcher.
> 
> IOW, I'm not really sure I understand why kded5 doesn't retain that 
> presence with `QT_MAC_DISABLE_FOREGROUND_APPLICATION_TRANSFORM` set. It's 
> possible that all the env. variable does is postpone the actions that lead to 
> that presence. If that's true than we'd have to come back to the more 
> appropriate previous revision of this patch.
> 
> OTOH: the only dialogs I have seen under KDE4 that are related to kded 
> (unknown cert) were posted when kded4 was *not* running. Ditto for cookie 
> related things. Under what circumstances is kded supposed to present a GUI?

Here is an easy way to test this: do the same change for kiod in kio (it's like 
a mini kded) and then
cd kio/tests ; ./listjobtest ftp://t...@upload.kde.org
should bring up a password dialog.

Except that with Qt 5.6 from git here (from some time ago) it asserts in DBus 
(looking into that now)... but hopefully you have Qt 5.5 ?


- David


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


On Nov. 25, 2015, 6:12 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126170/
> ---
> 
> (Updated Nov. 25, 2015, 6:12 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: kded
> 
> 
> Description
> ---
> 
> There should be no reason to build `kded5` as an app bundle on OS X, and with 
> recent feedback in mind I postulated that marking it "nongui" 
> (`ecm_mark_nongui_application`) would be acceptable on other platforms too.
> 
> Additionally, `kded5` doesn't have any more reason to appear in the Dock or 
> app switcher, on OS X, so I have added the code to make it an agent.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 4b9a5ff 
>   src/CMakeLists.txt 5e95df8 
>   src/kded.cpp 6929d7d 
> 
> Diff: https://git.reviewboard.kde.org/r/126170/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.9.5 with Qt 5.5.1 and FWs 5.16.0 .
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

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

Re: Review Request 126184: Editing toolbars in KXmlGuiWindow sometimes deletes all UI plugged actions

[David Faure]

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



src/kedittoolbar.cpp (line 770)
<https://git.reviewboard.kde.org/r/126184/#comment61017>

this code path didn't call accept() before.

Please investigate when save can fail, or keep former behaviour (by adding 
"return;").


- David Faure


On Nov. 27, 2015, 1:40 p.m., Jean-Baptiste Mardelle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126184/
> ---
> 
> (Updated Nov. 27, 2015, 1:40 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 352882
> https://bugs.kde.org/show_bug.cgi?id=352882
> 
> 
> Repository: kxmlgui
> 
> 
> Description
> ---
> 
> To Reproduce: in a KXmlGuiWindow, open the Configure Toolbars dialog.
> 1 - Change the main toolbar setting. 
> 2 - Click Apply, the ui is rebuild, and the newToolBarConfig signal is 
> emitted, allowing main app to re-plug its dynamic actions
> 3 - Click Ok, the ui is rebuild but the signal is not emitted, so app cannot 
> re-plug actions, and they are lost.
> 
> The problem only happens if you click "apply" and then "ok". This is due to 
> the fact that when Ok is clicked, the ui is rebuild (m_widget->save triggers 
> ui rebuild), but signal is not emitted.
> 
> My solution is to rebuild ui only if something changed when clicking "ok".
> 
> 
> Diffs
> -
> 
>   src/kedittoolbar.cpp ba4ba0f 
> 
> Diff: https://git.reviewboard.kde.org/r/126184/diff/
> 
> 
> Testing
> ---
> 
> Tested, fixes the reported problem.
> 
> 
> Thanks,
> 
> Jean-Baptiste Mardelle
> 
>

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

Re: kpty/utempter

[David Faure]

On Monday 30 November 2015 20:21:23 René J.V. Bertin wrote:
> I've had a quick look, and it appears that it shouldn't be overly hard to 
> port libutempter to OS X (which has setutxent, pututxline etc. instead of 
> "x-less" counterparts on Linux).
> 
> Question is, how useful is this in KPty?

`utempter is a privileged helper program that writes utmp/wtmp entries for 
unprivileged programs.`

Does utmp or wtmp exist on OSX?
Hmm, yes, but deprecated -- 
https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man5/utmp.5.html
So probably not worth it.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: kpty/utempter

[David Faure]

On Sunday 06 December 2015 22:00:09 René J.V. Bertin wrote:
> On Sunday December 06 2015 21:52:21 David Faure wrote:
> 
> > > Question is, how useful is this in KPty?
> > 
> > `utempter is a privileged helper program that writes utmp/wtmp entries for 
> > unprivileged programs.`
> 
> I can read, you know ;)
> 
> > 
> > Does utmp or wtmp exist on OSX?
> > Hmm, yes, but deprecated -- 
> > https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man5/utmp.5.html
> > So probably not worth it.
> 
> There's utmpx nowadays, which isn't marked as deprecated, and is backwards 
> compatible with utmp:
> https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man5/utmpx.5.html
> 
> Question is, what are we missing out on without it? Is this what kdesu is 
> based on, for instance?

It's just about logging, so "based on" is not really true. I'm pretty sure 
kdesu works even without logging to utmp.

But I don't really need to tell you any of this, given that you can read :-)

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 126170: [OS X] make kded5 an agent, and build it as a regular application instead of an app bundle

[David Faure]

On Sunday 06 December 2015 21:55:44 René J.V. Bertin wrote:
> On Sunday December 06 2015 14:51:40 David Faure wrote:
> 
> > Here is an easy way to test this: do the same change for kiod in kio (it's 
> > like a mini kded) and then
> > cd kio/tests ; ./listjobtest ftp://t...@upload.kde.org
> > should bring up a password dialog.
> 
> OK, hope to get around to doing that tomorrow.
> 
> > 
> > Except that with Qt 5.6 from git here (from some time ago) it asserts in 
> > DBus (looking into that now)... but hopefully you have Qt 5.5 ?
> 
> Yes.
> Is that assert platform-specific? There has been a recent change that 
> apparently removed DBus access from the OS X platform plugin, I hope that's 
> not related? (I can dig out the code review ticket if you missed it.)

No, it's about dbus running in a secondary thread in Qt 5.6.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 123229: Ensure we don't crash when using KIO from non-QApplication process

[David Faure]

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


I don't understand why the move to Q_COREAPP_STARTUP_FUNCTION. That startup 
function is only about creating some singleton, not about registering a KJob 
(which is where the qApp type check happens).

If the startup function needs to see a fully constructed app (why?) - then 
Q_COREAPP_STARTUP_FUNCTION is too early, it's called by the QCoreApplication 
constructor.
But I don't understand what was the problem with the function being called even 
earlier. Do we have a case where a kio job is created before the app is 
constructed? (that sounds wrong).

- David Faure


On Dec. 7, 2015, 1:29 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123229/
> ---
> 
> (Updated Dec. 7, 2015, 1:29 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Prevents the UiDelegate and UiTracker to get initialized, because they'll try 
> to create windows and dialogs when some things happen and these will 
> immediately end in a crash.
> 
> 
> Diffs
> -
> 
>   src/widgets/kdynamicjobtracker.cpp 14924d5 
> 
> Diff: https://git.reviewboard.kde.org/r/123229/diff/
> 
> 
> Testing
> ---
> 
> Ran the tests, my unit test doesn't crash anymore.
> 
> 
> 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 123229: Ensure we don't crash when using KIO from non-QApplication process

[David Faure]

> On April 7, 2015, 3:38 p.m., Aleix Pol Gonzalez wrote:
> > src/widgets/jobuidelegate.cpp, line 391
> > 
> >
> > This is not correct, it's too early to figure out if it's going to have 
> > a QApplication or not. Breaks some test.

Which test? Can you be more specific?


- David


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


On Dec. 7, 2015, 1:29 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123229/
> ---
> 
> (Updated Dec. 7, 2015, 1:29 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Prevents the UiDelegate and UiTracker to get initialized, because they'll try 
> to create windows and dialogs when some things happen and these will 
> immediately end in a crash.
> 
> 
> Diffs
> -
> 
>   src/widgets/kdynamicjobtracker.cpp 14924d5 
> 
> Diff: https://git.reviewboard.kde.org/r/123229/diff/
> 
> 
> Testing
> ---
> 
> Ran the tests, my unit test doesn't crash anymore.
> 
> 
> 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 126295: Fix wrong button in KUrlNavigator issue caused by 9dbe36f734b5b839b2a6a934fad29d639e954498

[David Faure]

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



src/filewidgets/fix-url-navigator.patch (line 1)
<https://git.reviewboard.kde.org/r/126295/#comment61062>

I don't think you wanted to added this patch to the git repo :-)



src/filewidgets/kurlnavigator.cpp (line 361)
<https://git.reviewboard.kde.org/r/126295/#comment61063>

This can be a local path, you can't give it to the QUrl(QString) 
constructor (it would break e.g. when the filename contains '#')

It seems retrievePlacePath() can return either a local path or a URL. I 
would change that to always return a URL, that's much easier to handle 
programmatically. Use toDisplayString(PreferLocalFile) only at the time of 
displaying this to the user.
retrievePlacePath() doing idx+=3 and url.left(idx) is indeed very very 
wrong, in terms of path escaping.


- David Faure


On Dec. 9, 2015, 9:42 p.m., Xuetian Weng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126295/
> ---
> 
> (Updated Dec. 9, 2015, 9:42 p.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Emmanuel Pescosta.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> 9dbe36f734b5b839b2a6a934fad29d639e954498 breaks some assumption on 
> startIndex. The argument index passed to buttonUrl is based number of '/'in 
> full url. Because of that, url like "sftp://a...@b.com/a/b"; is now shown as 
> "sftp:a...@b.com" / "b" / "b" in dolphin url bar.
> 
> This patch changes all relevant code that calls buttonUrl() to use url.path() 
> instead of url.toDisplayString() to count the number of "/".
> 
> 
> Diffs
> -
> 
>   src/filewidgets/fix-url-navigator.patch PRE-CREATION 
>   src/filewidgets/kurlnavigator.cpp 848e98b 
> 
> Diff: https://git.reviewboard.kde.org/r/126295/diff/
> 
> 
> Testing
> ---
> 
> Remote url -> ok
> Local url -> ok
> Remote url in places and try browse some sub folder -> ok
> Local url in places and try browse some sub folder -> ok
> 
> 
> Thanks,
> 
> Xuetian Weng
> 
>

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

Re: Review Request 123229: Ensure we don't crash when using KIO from non-QApplication process

[David Faure]

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


OK, at least it makes more sense to me now. We register at QCoreApp 
construction time, even though it's slightly too early (we would want QApp 
construction time, but no mechanism for that) because doing it earlier is 
useless. And then we check for QApp in the code. OK.


src/widgets/kdynamicjobtracker.cpp (line 89)
<https://git.reviewboard.kde.org/r/123229/#comment61065>

this check should also be done on line 76 where another KWidgetJobTracker 
is being created, then.


- David Faure


On Dec. 7, 2015, 1:29 p.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123229/
> ---
> 
> (Updated Dec. 7, 2015, 1:29 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Prevents the UiDelegate and UiTracker to get initialized, because they'll try 
> to create windows and dialogs when some things happen and these will 
> immediately end in a crash.
> 
> 
> Diffs
> -
> 
>   src/widgets/kdynamicjobtracker.cpp 14924d5 
> 
> Diff: https://git.reviewboard.kde.org/r/123229/diff/
> 
> 
> Testing
> ---
> 
> Ran the tests, my unit test doesn't crash anymore.
> 
> 
> 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 126295: Fix wrong button in KUrlNavigator issue caused by 9dbe36f734b5b839b2a6a934fad29d639e954498

[David Faure]

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



src/filewidgets/kurlnavigator.cpp (line 156)
<https://git.reviewboard.kde.org/r/126295/#comment61132>

Please rename to retrievePlaceUrl, otherwise when looking at the calling 
code it still seems like it returns a path ;)



src/filewidgets/kurlnavigator.cpp (line 368)
<https://git.reviewboard.kde.org/r/126295/#comment61133>

The issue was there already, but I don't see why this calls 
retrievePlacePath() again, it could just do placeUrl.toDisplayString...



src/filewidgets/kurlnavigator.cpp (line 369)
<https://git.reviewboard.kde.org/r/126295/#comment61135>

This comment is misleading. That method doesn't return an empty url for 
local files. The path is empty, that's what you meant, right?

But then no point in calling toDisplayString(PreferLocalFile), if we know 
that for local files we'll need "/" anyway, right?
IIUC this could be simplified/clarified to

if (placeUrl.isLocalFile()) {
dirName = QStringLiteral("/");
} else {
dirName = placeUrl.toDisplayString();
}

no?



src/filewidgets/kurlnavigator.cpp (line 784)
<https://git.reviewboard.kde.org/r/126295/#comment61134>

setPath(QString()) slightly faster


- David Faure


On Dec. 12, 2015, 9:35 a.m., Xuetian Weng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126295/
> ---
> 
> (Updated Dec. 12, 2015, 9:35 a.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Emmanuel Pescosta.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> 9dbe36f734b5b839b2a6a934fad29d639e954498 breaks some assumption on 
> startIndex. The argument index passed to buttonUrl is based number of '/'in 
> full url. Because of that, url like "sftp://a...@b.com/a/b"; is now shown as 
> "sftp:a...@b.com" / "b" / "b" in dolphin url bar.
> 
> This patch changes all relevant code that calls buttonUrl() to use url.path() 
> instead of url.toDisplayString() to count the number of "/".
> 
> 
> Diffs
> -
> 
>   src/filewidgets/kurlnavigator.cpp 848e98b 
> 
> Diff: https://git.reviewboard.kde.org/r/126295/diff/
> 
> 
> Testing
> ---
> 
> Remote url -> ok
> Local url -> ok
> Remote url in places and try browse some sub folder -> ok
> Local url in places and try browse some sub folder -> ok
> 
> 
> Thanks,
> 
> Xuetian Weng
> 
>

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

Re: Review Request 126170: [OS X] make kded5 an agent, and build it as a regular application instead of an app bundle

[David Faure]

On Monday 07 December 2015 16:14:54 René J.V. Bertin wrote:
> On Sunday December 06 2015 14:51:40 David Faure wrote:
> 
> > Here is an easy way to test this: do the same change for kiod in kio (it's 
> > like a mini kded) and then
> > cd kio/tests ; ./listjobtest ftp://t...@upload.kde.org
> > should bring up a password dialog.
> 
> Regardless of what I try (even with the non-agent'ised, non-nongui version), 
> I only get this:
> 
> CD kio/build/tests listjobtest.app/Contents/MacOS/listjobtest 
> "ftp://t...@upload.kde.org";
> Starting listJob for the URL: QUrl("ftp://t...@upload.kde.org";)
> Runtime: 291 milliseconds.
> Press Enter to quit.
> 
> kiod5 is started, but doesn't post a dialog. Doesn't raise/return an error 
> either.
> If there a debug category to activate in order to get more output I haven't 
> found it.

Did you try this on linux with Qt 5.4 or 5.5, to make sure there isn't another 
bug?

I'm still fighting Qt 5.6 with this, dbus-in-a-thread creates lots of 
additional trouble.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: kdelibs4support forces "icons in buttons"?

[David Faure]

On Tuesday 08 December 2015 14:36:14 René J.V. Bertin wrote:
> Hi,
> 
> I have the strong impression that applications using the kdelibs4support fw 
> are forced to show icons in buttons, regardless of the corresponding setting 
> (which is respected elsewhere).
> For some reason it does not happen when I use the native platform plugin on 
> OS X, but if the frameworkintegration platform plugin were responsible I'd 
> expect icons to appear on buttons in all applications, including those not 
> using kdelibs4support.
> 
> What's going on here? As far as I can see kdelibs4support does indeed support 
> the key from kdeglobals; maybe the return value from the functions reading 
> the key value is not used?

Hi René,

This is a development list.
If you want to just report bugs and let others debug it, please use 
http://bugs.kde.org.

No hard feelings, just that everything has a right place.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 125988: [KUrlCompletion] Add autocompletion for '.' input which offers all hidden files/folders

[David Faure]

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


Either that, or if you want the fix to be available before 5.6 is out, wrap 
your fix with #if QT_VERSION < QT_VERSION_CHECK(5, 6, 0).

We can always clean it up once 5.6 is required (i.e. in a few years ;).
And mention the qt fix in a comment of course.

I don't mind which way around you choose to go.

- David Faure


On Nov. 28, 2015, 9:36 p.m., Emmanuel Pescosta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125988/
> ---
> 
> (Updated Nov. 28, 2015, 9:36 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 354981
> https://bugs.kde.org/show_bug.cgi?id=354981
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Currently KUrlCompletion only offers autocompletion for hidden folders when 
> you input at least one additional character after the dot. 
> With this patch all hidden folders will be offered when only a dot is present.
> 
> This behaviour is test covered.
> 
> 
> Diffs
> -
> 
>   autotests/kurlcompletiontest.cpp ca8563c 
>   src/widgets/kurlcompletion.cpp c6764e4 
> 
> Diff: https://git.reviewboard.kde.org/r/125988/diff/
> 
> 
> Testing
> ---
> 
> All test cases for KUrlCompletion pass
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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

Re: RFC: split platformtheme plugin from frameworkintegration and move to kde/workspace

[David Faure]

On Thursday 10 December 2015 10:38:06 Weng Xuetian wrote:
>  if you want to achieve the same feature in some
> other desktop, it would be better to make a fork of
> frameworkintegration.

Not frameworkintegration, just the platformtheme (QPT).

Like some others before, you're confusing frameworkintegration
(the framework, which contains 3 different things) and
the platformtheme plugin (which is one of these things).

There is 0 point in anyone forking the other things like
integrationplugin, which is about KMessageBox being able
to use KConfig when both are installed.

---

Back to the topic: the platformtheme plugin is indeed about "KF5 based apps
integrating better with the plasma desktop, visually". I.e. getting
a KFileDialog in Plasma. The thing is, this could be generalized to
"getting a KFileDialog in plasma or any other workspace based on Qt+KF5"

But I don't even know if e.g. LXQt wants to see the KDE File Dialog or the Qt 
File Dialog.
-> CC'ing Jerome Leclanche for input.

However, as Martin says, the *current* code is only loaded in a plasma
session anyway (Qt looks for e.g. $XDG_CURRENT_DESKTOP=KDE)
so this move won't, in itself, change anything for LXQt users.

Therefore I'm OK with it moving. (*)

I would just like to define the best architecture longer term. If KFileDialog is
wanted outside of plasma, then indeed some code should move back to KF5
(e.g. in KIO) and be shared between the platformthemes. But not the code
that derives from QPA classes since they are not fully public, gah

(*) 
The packagers are going to hate this though, there will be a conflict between
KF 5.16 and Plasma X.Y (the next release), which will only be resolved once
KF 5.17 is out. Any time we move something we go against the idea
of "separate products".
This is never simple to solve. If you make plasma check for the KF version
at cmake time to skip the installation of the plugin, someone then upgrading
from KF 5.16 to KF 5.17 ends up with no plugin at all.
Changing the filename is one option, I guess the theme factory in Qt will
then randomly pick one of the two plugins providing the key "KDE" ?

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 126295: Fix wrong button in KUrlNavigator issue caused by 9dbe36f734b5b839b2a6a934fad29d639e954498

[David Faure]

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

Ship it!


We could findChildren in an autotest to find the buttons and check their number 
and the text on each one.


src/filewidgets/kurlnavigator.cpp (line 371)
<https://git.reviewboard.kde.org/r/126295/#comment61153>

No need for PreferLocalFile anymore, the url is always remote. My code was 
correct :)


- David Faure


On Dec. 12, 2015, 11:11 a.m., Xuetian Weng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126295/
> ---
> 
> (Updated Dec. 12, 2015, 11:11 a.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Emmanuel Pescosta.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> 9dbe36f734b5b839b2a6a934fad29d639e954498 breaks some assumption on 
> startIndex. The argument index passed to buttonUrl is based number of '/'in 
> full url. Because of that, url like "sftp://a...@b.com/a/b"; is now shown as 
> "sftp:a...@b.com" / "b" / "b" in dolphin url bar.
> 
> This patch changes all relevant code that calls buttonUrl() to use url.path() 
> instead of url.toDisplayString() to count the number of "/".
> 
> 
> Diffs
> -
> 
>   src/filewidgets/kurlnavigator.cpp 848e98b 
> 
> Diff: https://git.reviewboard.kde.org/r/126295/diff/
> 
> 
> Testing
> ---
> 
> Remote url -> ok
> Local url -> ok
> Remote url in places and try browse some sub folder -> ok
> Local url in places and try browse some sub folder -> ok
> 
> 
> Thanks,
> 
> Xuetian Weng
> 
>

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

Re: Review Request 125988: [KUrlCompletion] Add autocompletion for '.' input which offers all hidden files/folders

[David Faure]

> On Dec. 12, 2015, 11:12 a.m., David Faure wrote:
> > Either that, or if you want the fix to be available before 5.6 is out, wrap 
> > your fix with #if QT_VERSION < QT_VERSION_CHECK(5, 6, 0).
> > 
> > We can always clean it up once 5.6 is required (i.e. in a few years ;).
> > And mention the qt fix in a comment of course.
> > 
> > I don't mind which way around you choose to go.

Ah right the "fix" isn't just the if/else but also the added member vars. Let's 
not have that in, too messy.

Hmm. But your autotest doesn't pass even with my fix. I'll debug this...


- David


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


On Nov. 28, 2015, 9:36 p.m., Emmanuel Pescosta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125988/
> ---
> 
> (Updated Nov. 28, 2015, 9:36 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 354981
> https://bugs.kde.org/show_bug.cgi?id=354981
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Currently KUrlCompletion only offers autocompletion for hidden folders when 
> you input at least one additional character after the dot. 
> With this patch all hidden folders will be offered when only a dot is present.
> 
> This behaviour is test covered.
> 
> 
> Diffs
> -
> 
>   autotests/kurlcompletiontest.cpp ca8563c 
>   src/widgets/kurlcompletion.cpp c6764e4 
> 
> Diff: https://git.reviewboard.kde.org/r/125988/diff/
> 
> 
> Testing
> ---
> 
> All test cases for KUrlCompletion pass
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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

Re: Review Request 125988: [KUrlCompletion] Add autocompletion for '.' input which offers all hidden files/folders

[David Faure]

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


Ah I needed the last hunk from your patch, too. Now it works. 
http://www.davidfaure.fr/2015/0001-Add-autocompletion-for-.-input-which-offers-all-hidd.patch
 ok with you?

- David Faure


On Nov. 28, 2015, 9:36 p.m., Emmanuel Pescosta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125988/
> ---
> 
> (Updated Nov. 28, 2015, 9:36 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Bugs: 354981
> https://bugs.kde.org/show_bug.cgi?id=354981
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Currently KUrlCompletion only offers autocompletion for hidden folders when 
> you input at least one additional character after the dot. 
> With this patch all hidden folders will be offered when only a dot is present.
> 
> This behaviour is test covered.
> 
> 
> Diffs
> -
> 
>   autotests/kurlcompletiontest.cpp ca8563c 
>   src/widgets/kurlcompletion.cpp c6764e4 
> 
> Diff: https://git.reviewboard.kde.org/r/125988/diff/
> 
> 
> Testing
> ---
> 
> All test cases for KUrlCompletion pass
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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

Re: Review Request 126329: kio: Do not use QStringLiteral with multi strings

[David Faure]

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



src/widgets/kpropertiesdialog.cpp (line 603)
<https://git.reviewboard.kde.org/r/126329/#comment61183>

the extra spaces before the ')'s don't make sense anymore (I guess they 
were for alignment)


- David Faure


On Dec. 12, 2015, 11:08 p.m., Patrick Spendrin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126329/
> ---
> 
> (Updated Dec. 12, 2015, 11:08 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Strings that are separated into multiple parts don't work on Windows
> together with QStringLiteral as the first string is interpreted as a
> wide (16bit) string, and the second one as a narrow (8bit) string.
> Replacing with QString::fromLatin1 is the easiest solution keeping
> the code layout the same, joining the strings does work too though.
> 
> 
> Diffs
> -
> 
>   autotests/dataprotocoltest.cpp 9fe238fdbb0e9682141772d423a64edd5621921b 
>   src/core/ksambashare.cpp a3f84ac3971141e687d9ab17e0131a66db34ed5a 
>   src/filewidgets/kfileplacesmodel.cpp 
> b409c1b1617f97f3cdbc79a2c76110a5f9449398 
>   src/ioslaves/help/kio_help.cpp cb27a77b22fe378a126d985621985265edb93767 
>   src/widgets/kpropertiesdialog.cpp 0ff506273a10dba238fefc5c552c71434681285e 
> 
> Diff: https://git.reviewboard.kde.org/r/126329/diff/
> 
> 
> Testing
> ---
> 
> Windows.
> 
> 
> Thanks,
> 
> Patrick Spendrin
> 
>

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

Re: Review Request 126329: kio: Do not use QStringLiteral with multi strings

[David Faure]

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

Ship it!


Ship It!

- David Faure


On Dec. 13, 2015, 10:04 p.m., Patrick Spendrin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126329/
> ---
> 
> (Updated Dec. 13, 2015, 10:04 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Strings that are separated into multiple parts don't work on Windows
> together with QStringLiteral as the first string is interpreted as a
> wide (16bit) string, and the second one as a narrow (8bit) string.
> Replacing with QString::fromLatin1 is the easiest solution keeping
> the code layout the same, joining the strings does work too though.
> 
> 
> Diffs
> -
> 
>   autotests/dataprotocoltest.cpp 9fe238fdbb0e9682141772d423a64edd5621921b 
>   src/core/ksambashare.cpp a3f84ac3971141e687d9ab17e0131a66db34ed5a 
>   src/filewidgets/kfileplacesmodel.cpp 
> b409c1b1617f97f3cdbc79a2c76110a5f9449398 
>   src/ioslaves/help/kio_help.cpp cb27a77b22fe378a126d985621985265edb93767 
>   src/widgets/kpropertiesdialog.cpp 0ff506273a10dba238fefc5c552c71434681285e 
> 
> Diff: https://git.reviewboard.kde.org/r/126329/diff/
> 
> 
> Testing
> ---
> 
> Windows.
> 
> 
> Thanks,
> 
> Patrick Spendrin
> 
>

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

Re: Review Request 126312: Add xcb variant for static KStartupInfo::sendFoo methods

[David Faure]

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

Ship it!


Ship It!

- David Faure


On Dec. 14, 2015, 7:08 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126312/
> ---
> 
> (Updated Dec. 14, 2015, 7:08 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> Adds for each of sendStartup, sendChange, sendFinish an xcb variant
> and deprecates the XLib variant. In addition the static variants which
> are not windowing system specific delegate to the new xcb variant to
> share more code paths.
> 
> 
> Diffs
> -
> 
>   src/kstartupinfo.h dfcd42797d887ca6d43161f8c3b767ad436e5116 
>   src/kstartupinfo.cpp a97b8b5416ca67a083960a76093933fb098327a5 
> 
> Diff: https://git.reviewboard.kde.org/r/126312/diff/
> 
> 
> Testing
> ---
> 
> autotest still passes
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

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

Re: Review Request 126339: remove kdewin dependency

[David Faure]

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

Ship it!


Ship It!

- David Faure


On Dec. 14, 2015, 10:37 a.m., Patrick Spendrin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126339/
> ---
> 
> (Updated Dec. 14, 2015, 10:37 a.m.)
> 
> 
> Review request for KDE Frameworks and Martin Tobias Holmedahl Sandsmark.
> 
> 
> Repository: khtml
> 
> 
> Description
> ---
> 
> This removes the direct kdewin dependency by replacing problematic
> function calls (uname, snprintf) with their Qt counterparts.
> Some leftover unix header includes removed too.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 51fe02a01c8166046d1c6085ec4a5b6e617e1fea 
>   src/ecma/kjs_binding.cpp 5e122f0f0d70e6734565e7ab205d14a92c79d287 
>   src/ecma/kjs_navigator.cpp 2f7be8d11e5af84e08ac640ccbc97f70aeac8abd 
>   src/ecma/kjs_proxy.h 24abd1e1bac0932f8829f02185953142c74aadc8 
>   src/ecma/kjs_proxy.cpp 20430f48fce986ca654c49c5771ad839845f11ab 
>   src/khtml_pagecache.cpp 8e1841f6b0e816dfd8faa76f2191b269c4716011 
>   src/khtml_part.cpp adbcd800a526e9f8fd92a553e62ee64791872938 
>   src/kmultipart/kmultipart.cpp 1ad3bbb9b6d6a022799d5ef85f426fc9f911d45b 
> 
> Diff: https://git.reviewboard.kde.org/r/126339/diff/
> 
> 
> Testing
> ---
> 
> Windows, Linux compiles.
> 
> 
> Thanks,
> 
> Patrick Spendrin
> 
>

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

Re: Review Request 126330: use Qt defines for portability

[David Faure]

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


I vote for generate_export_header.

- David Faure


On Dec. 12, 2015, 11:12 p.m., Patrick Spendrin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126330/
> ---
> 
> (Updated Dec. 12, 2015, 11:12 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdeclarative
> 
> 
> Description
> ---
> 
> far better solution would be to use generate_export_header, if you prefer
> that, please let me know.
> 
> 
> Diffs
> -
> 
>   src/calendarevents/calendarevents_export.h 
> a4c6f87e62c02a4ed34d0ebff00e0a729357952f 
> 
> Diff: https://git.reviewboard.kde.org/r/126330/diff/
> 
> 
> Testing
> ---
> 
> Windows.
> 
> 
> Thanks,
> 
> Patrick Spendrin
> 
>

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

Re: about ki18n/locales: installing only a subset?

[David Faure]

On Tuesday 15 December 2015 11:19:29 René J.V. Bertin wrote:
> Hi,
> 
> The KDE4 approach to internationalisation (sic :)) had the huge advantage 
> that it was rather trivial to provide installer packages for individual 
> languages so users could install only those few languages they would actually 
> use.
> It seems that with KF5 we have gotten back in the situation where you get 
> every possible language installed. Now that may be nice for the occasional 
> office prank, but it'll end up amounting to a significant disk overhead (the 
> one that comes with lots of small files) for something that has absolutely no 
> use for the vast majority of users.
> 
> Is there some central mechanism to control which languages are installed? 
> Looking at KF5I18NMacros it would seem that there's only an external "central 
> mechanism"; binning the unwanted files before building...

Yep, remove them if you don't want them, but then your users are going to 
complain, unless you provide them separately.

The overhead is minimal and allows for modularity.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: lib/x86_64-linux-gnu/libKF5FileMetaData.so | lib/libKF5FileMetaData.3.dylib

[David Faure]

Fixed, it was an oversight when converting the lib into a KF5 framework.

> On Tuesday December 15 2015 18:55:52 René J.V. Bertin wrote:
> > for packaging purposes it would be preferable if all libraries (symlinks) 
> > that are recorded in dependents' dependency lists are of the form 
> > libKF5*.5.dylib or libKF5*.so.5 (KFileMetadata is the only deviant one).

Not exactly, there's also 

../bluez-qt/CMakeLists.txt:27:SOVERSION 6
../kactivities/CMakeLists.txt:129:  SOVERSION 1
../modemmanager-qt/CMakeLists.txt:40:SOVERSION 6)
../networkmanager-qt/CMakeLists.txt:40:SOVERSION 6)

> PS : KDE4's kfilemetadata has a compatibility/so version 4 ...

Doesn't matter, it had a different library name (no KF5 in the name).

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 126326: kdbusaddons: Do not use QStringLiteral with multi strings

[David Faure]

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

Ship it!


Ship It!

- David Faure


On Dec. 12, 2015, 10:58 p.m., Patrick Spendrin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126326/
> ---
> 
> (Updated Dec. 12, 2015, 10:58 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdbusaddons
> 
> 
> Description
> ---
> 
> Strings that are separated into multiple parts don't work on Windows
> together with QStringLiteral as the first string is interpreted as a
> wide (16bit) string, and the second one as a narrow (8bit) string.
> Replacing with QString::fromLatin1 is the easiest solution keeping
> the code layout the same, joining the strings would work too though.
> 
> 
> Diffs
> -
> 
>   src/kdbusservice.cpp d17bfd2b971d462cc9ddea37624f1e71c7d0f16e 
> 
> Diff: https://git.reviewboard.kde.org/r/126326/diff/
> 
> 
> Testing
> ---
> 
> Windows.
> 
> 
> Thanks,
> 
> Patrick Spendrin
> 
>

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

Re: Review Request 126325: kwidgetsaddons: Do not use QStringLiteral with multi strings

[David Faure]

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

Ship it!


Ship It!

- David Faure


On Dec. 13, 2015, 10:38 p.m., Patrick Spendrin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126325/
> ---
> 
> (Updated Dec. 13, 2015, 10:38 p.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> Strings that are separated into multiple parts don't work on Windows
> together with QStringLiteral as the first string is interpreted as a
> wide (16bit) string, and the second one as a narrow (8bit) string.
> Replacing with QString::fromLatin1 is the easiest solution keeping
> the code layout the same, joining the strings would work too though.
> 
> 
> Diffs
> -
> 
>   src/kmessagewidget.cpp b8070880dc2e7e9fd8c6c9be7f0087d4da83b413 
>   tests/kmessageboxtest.cpp 73e5f7acf297eb9fb39bbcaf03f79ff9e17e29ac 
> 
> Diff: https://git.reviewboard.kde.org/r/126325/diff/
> 
> 
> Testing
> ---
> 
> Windows.
> 
> 
> Thanks,
> 
> Patrick Spendrin
> 
>

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

Re: about ki18n/locales: installing only a subset?

[David Faure]

On Wednesday 16 December 2015 11:20:38 René J.V. Bertin wrote:
> On Wednesday December 16 2015 09:03:53 David Faure wrote:
> 
> > Yep, remove them if you don't want them, but then your users are going to 
> > complain, unless you provide them separately.
> 
> I think I implied in my original message that I consider providing them 
> separately the ideal solution ;)
> Sadly such an approach is far from trivial to implement (all the more with 
> MacPorts) if the source files are scattered over individual packages instead 
> of being available as complete bundles like they used to be. Or am I missing 
> something?

They are copied into the frameworks at release time. So you can also grab them 
from
svn://anonsvn.kde.org/home/kde/trunk/l10n-kf5 directly (warning, this is huge)
See release-tools/update_l10n.sh for the script I use to fetch only the 
frameworks translations
in all languages (rather than all translations and all docbooks for all apps in 
all languages)

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 126324: [MSWin/OS X] save and restore window geometry instead of only size (WIP/Suggestion)

[David Faure]

> On Dec. 17, 2015, 4:16 p.m., Martin Gräßlin wrote:
> > src/gui/kwindowconfig.h, lines 38-39
> > 
> >
> > That doesn't match the method name. It's saveWindowSize, not 
> > saveWindowGeometry. It's highly unexpected that saveWindowSize saves the 
> > position.
> > 
> > If you want that: please introduce a new saveWindowGeometry method.
> 
> René J.V. Bertin wrote:
> I was afraid someone was going to say that, which is why I tried to argue 
> that it's highly unexpected from a user viewpoint that only window size is 
> saved and not position. How often would it happen that a developer is "highly 
> surprised" in a *negative* way that window size AND position are restored on 
> a platform where this is the default behaviour?
> 
> I have nothing against introducing a pair of new methods, but how is that 
> supposed to be done in transparent fashion? I do have a lot against a need to 
> change all dependent software to call those methods (maintenance burden and 
> all that).
> 
> Counter proposal: replace save/restoreWindowSize with 
> save/restoreWindowGeometry everywhere, with a platform-specific 
> interpretation of what exactly geometry encompasses. Much less surprise 
> there, just a bit more need to read the documentation. Are these functions 
> ever called intentionally outside of what I suppose is a more or less 
> automatic feature that takes care of restoring window, erm, layout (saving is 
> clearly automatic).
> 
> René J.V. Bertin wrote:
> Just to be clear: if I am going to introduce restore/saveWindowGeometry 
> methods they'll replace the WindowSize variants on OS X or at least those 
> will then use a different KConfig key to avoid conflicts. 
> I'd also be dropping the MS Windows part of the patch (as this is not a 
> decision I want to make for a platform I don't use).
> 
> But please consider this: that KConfig key has been called `geometry` for 
> a long time. Where exactly is the surprise, that restore/saveWindowSize never 
> did what the key they operate with suggests, or that they have always been 
> using an inaptly named key? For me the answer is straightforward and based on 
> what users will expect...
> 
> Martin Gräßlin wrote:
> I leave it to the maintainers. On API I maintain I would say no to 
> something changing the semantics like that.
> 
> René J.V. Bertin wrote:
> As I just wrote in reply to a message from Valorie, I have absolutely no 
> issue with maintaining methods for saving and restoring only window size, for 
> code that somehow requires that. I'd guess that such code would probably 
> enforce the intended window position itself *after* restoring window size 
> (because that operation *can* affect window position), but in the end that's 
> (indeed) up to the code's developers to decide.
> 
> IOW, I'm perfectly willing to discuss a better solution in which the 
> burden to ensure that window save/restore works as "natively" as possible on 
> each platform is shared. The best way to do that is of course to have a 
> single pair of methods that have platform-specific implementations.
> 
> As far as I'm concerned such a solution might even be prepared completely 
> in KConfig/gui before changes are made everywhere else to deploy that new 
> solution. In that case I would for instance run temporary local (MacPorts) 
> patches that replace saveWindowSize/restoreWindowSize with wrappers for 
> saveWindowGeometry/restoreWindowGeometry.
> 
> Side-observation: OS X (Cocoa) provides a `[NSWindow 
> setFrameAutosaveName:]` method, i.e. it avoids reference to specifics like 
> size or geometry completely.
> 
> That method also provides another thought that could be taken into 
> consideration if it is decided to evolve this part of the frameworks, 
> something I'd be interested in collaborating on. Currently, there is no 
> support for saving and restoring multiple windows per application. That may 
> be more or less sufficient when applications always follow a MDI approach, 
> but even if they do that still doesn't make them applications that are active 
> only in a single instance. Example: KDevelop. One might expect that opening a 
> given, pre-existing session (collection of open projects) restores the main 
> window geometry (size and/or position) that used previously for that session, 
> rather than the geometry used by whatever KDevelop session was run last. On 
> OS X that would be done with something like `[NSWindow 
> setFrameautosaveName:[window representedFile]]`, where `[NSWindow 
> representedFile]` corresponds to `QWindow::filePath` (but AFAICS those are 
> not coupled in Qt5).
> 
> I already had a quick look, but realised I don't know if the KConfig 
> mechanism has facilities to handle cleanup of stale/obsolete key/value 
> entries.

Note that most apps use this via the higher-level 
KMainWi

Re: Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

[David Faure]

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



src/kdeui/kpushbutton.cpp (line 256)
<https://git.reviewboard.kde.org/r/126308/#comment61479>

This patch looks wrong because KPushButton can be used outside of "dialog 
button boxes", while the styleHint is supposed to be only about dialog button 
boxes.

QPushButton::sizeHint does this:
bool showButtonBoxIcons = 
qobject_cast(parentWidget())
  && 
style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons);
which is a solution for testing the parent widget.

I still don't fully understand the issue though, at painting time both 
QPushButton and KPushButton call QStyle's CE_PushButton, so I don't see why 
these two would be working differently.
Is this a workaround for KPushButton which doesn't fix QPushButton?


- David Faure


On Dec. 11, 2015, 4:26 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126308/
> ---
> 
> (Updated Dec. 11, 2015, 4:26 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks, Qt KDE, Hugo 
> Pereira Da Costa, and Yichao Yu.
> 
> 
> Repository: kdelibs4support
> 
> 
> Description
> ---
> 
> KF5 applications have long had a habit of drawing icons on buttons even when 
> this feature was turned off in the user's setting. This was mostly noticeable 
> in applications built on kdelibs4support.
> 
> It seems that the actual culprit is in Qt's QPushButton implementation 
> (https://bugreports.qt.io/browse/QTBUG-49887), but it is possible to work 
> around it in `KPushButton::paintEvent`, by removing the icon (forcing it to 
> the null icon) in the option instance, before handing off control to the 
> painter.
> 
> 
> Diffs
> -
> 
>   src/kdeui/kdialogbuttonbox.cpp 0f6649b 
>   src/kdeui/kpushbutton.cpp 98534fa 
> 
> Diff: https://git.reviewboard.kde.org/r/126308/diff/
> 
> 
> Testing
> ---
> 
> On Kubuntu 14.04 and OS X 10.9.5 with Qt 5.5.1 and KF5 frameworks 5.16.0 .
> 
> I have not yet verified if there are other classes where this modification 
> would be relevant too.
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

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

Re: lib/x86_64-linux-gnu/libKF5FileMetaData.so | lib/libKF5FileMetaData.3.dylib

[David Faure]

On Wednesday 16 December 2015 17:42:29 šumski wrote:
> On Wednesday 16 of December 2015 09:08:03 David Faure wrote:
> > Fixed, it was an oversight when converting the lib into a KF5 framework.
> 
> But this is a BiC change...

One that is generally allowed (not like removing a virtual method and NOT 
upgrading the so number)
but which indeed creates more work for packagers (need to ship the old as well 
as the new lib,
so that apps linking to the old lib keep working).

-> reverted, not worth the trouble.

Thanks for the note.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 126324: [MSWin/OS X] save and restore window geometry instead of only size (WIP/Suggestion)

[David Faure]

> On Dec. 17, 2015, 4:16 p.m., Martin Gräßlin wrote:
> > src/gui/kwindowconfig.h, lines 38-39
> > <https://git.reviewboard.kde.org/r/126324/diff/4/?file=422749#file422749line38>
> >
> > That doesn't match the method name. It's saveWindowSize, not 
> > saveWindowGeometry. It's highly unexpected that saveWindowSize saves the 
> > position.
> > 
> > If you want that: please introduce a new saveWindowGeometry method.
> 
> René J.V. Bertin wrote:
> I was afraid someone was going to say that, which is why I tried to argue 
> that it's highly unexpected from a user viewpoint that only window size is 
> saved and not position. How often would it happen that a developer is "highly 
> surprised" in a *negative* way that window size AND position are restored on 
> a platform where this is the default behaviour?
> 
> I have nothing against introducing a pair of new methods, but how is that 
> supposed to be done in transparent fashion? I do have a lot against a need to 
> change all dependent software to call those methods (maintenance burden and 
> all that).
> 
> Counter proposal: replace save/restoreWindowSize with 
> save/restoreWindowGeometry everywhere, with a platform-specific 
> interpretation of what exactly geometry encompasses. Much less surprise 
> there, just a bit more need to read the documentation. Are these functions 
> ever called intentionally outside of what I suppose is a more or less 
> automatic feature that takes care of restoring window, erm, layout (saving is 
> clearly automatic).
> 
> René J.V. Bertin wrote:
> Just to be clear: if I am going to introduce restore/saveWindowGeometry 
> methods they'll replace the WindowSize variants on OS X or at least those 
> will then use a different KConfig key to avoid conflicts. 
> I'd also be dropping the MS Windows part of the patch (as this is not a 
> decision I want to make for a platform I don't use).
> 
> But please consider this: that KConfig key has been called `geometry` for 
> a long time. Where exactly is the surprise, that restore/saveWindowSize never 
> did what the key they operate with suggests, or that they have always been 
> using an inaptly named key? For me the answer is straightforward and based on 
> what users will expect...
> 
> Martin Gräßlin wrote:
> I leave it to the maintainers. On API I maintain I would say no to 
> something changing the semantics like that.
> 
> René J.V. Bertin wrote:
> As I just wrote in reply to a message from Valorie, I have absolutely no 
> issue with maintaining methods for saving and restoring only window size, for 
> code that somehow requires that. I'd guess that such code would probably 
> enforce the intended window position itself *after* restoring window size 
> (because that operation *can* affect window position), but in the end that's 
> (indeed) up to the code's developers to decide.
> 
> IOW, I'm perfectly willing to discuss a better solution in which the 
> burden to ensure that window save/restore works as "natively" as possible on 
> each platform is shared. The best way to do that is of course to have a 
> single pair of methods that have platform-specific implementations.
> 
> As far as I'm concerned such a solution might even be prepared completely 
> in KConfig/gui before changes are made everywhere else to deploy that new 
> solution. In that case I would for instance run temporary local (MacPorts) 
> patches that replace saveWindowSize/restoreWindowSize with wrappers for 
> saveWindowGeometry/restoreWindowGeometry.
> 
> Side-observation: OS X (Cocoa) provides a `[NSWindow 
> setFrameAutosaveName:]` method, i.e. it avoids reference to specifics like 
> size or geometry completely.
> 
> That method also provides another thought that could be taken into 
> consideration if it is decided to evolve this part of the frameworks, 
> something I'd be interested in collaborating on. Currently, there is no 
> support for saving and restoring multiple windows per application. That may 
> be more or less sufficient when applications always follow a MDI approach, 
> but even if they do that still doesn't make them applications that are active 
> only in a single instance. Example: KDevelop. One might expect that opening a 
> given, pre-existing session (collection of open projects) restores the main 
> window geometry (size and/or position) that used previously for that session, 
> rather than the geometry used by whatever KDevelop session was run last. On 
> OS X that would be done with something like `[NSWindow 
> setFrameautosaveName:[window

Re: Review Request 126324: [MSWin/OS X] save and restore window geometry instead of only size (WIP/Suggestion)

[David Faure]

> On Dec. 17, 2015, 4:16 p.m., Martin Gräßlin wrote:
> > src/gui/kwindowconfig.h, lines 38-39
> > <https://git.reviewboard.kde.org/r/126324/diff/4/?file=422749#file422749line38>
> >
> > That doesn't match the method name. It's saveWindowSize, not 
> > saveWindowGeometry. It's highly unexpected that saveWindowSize saves the 
> > position.
> > 
> > If you want that: please introduce a new saveWindowGeometry method.
> 
> René J.V. Bertin wrote:
> I was afraid someone was going to say that, which is why I tried to argue 
> that it's highly unexpected from a user viewpoint that only window size is 
> saved and not position. How often would it happen that a developer is "highly 
> surprised" in a *negative* way that window size AND position are restored on 
> a platform where this is the default behaviour?
> 
> I have nothing against introducing a pair of new methods, but how is that 
> supposed to be done in transparent fashion? I do have a lot against a need to 
> change all dependent software to call those methods (maintenance burden and 
> all that).
> 
> Counter proposal: replace save/restoreWindowSize with 
> save/restoreWindowGeometry everywhere, with a platform-specific 
> interpretation of what exactly geometry encompasses. Much less surprise 
> there, just a bit more need to read the documentation. Are these functions 
> ever called intentionally outside of what I suppose is a more or less 
> automatic feature that takes care of restoring window, erm, layout (saving is 
> clearly automatic).
> 
> René J.V. Bertin wrote:
> Just to be clear: if I am going to introduce restore/saveWindowGeometry 
> methods they'll replace the WindowSize variants on OS X or at least those 
> will then use a different KConfig key to avoid conflicts. 
> I'd also be dropping the MS Windows part of the patch (as this is not a 
> decision I want to make for a platform I don't use).
> 
> But please consider this: that KConfig key has been called `geometry` for 
> a long time. Where exactly is the surprise, that restore/saveWindowSize never 
> did what the key they operate with suggests, or that they have always been 
> using an inaptly named key? For me the answer is straightforward and based on 
> what users will expect...
> 
> Martin Gräßlin wrote:
> I leave it to the maintainers. On API I maintain I would say no to 
> something changing the semantics like that.
> 
> René J.V. Bertin wrote:
> As I just wrote in reply to a message from Valorie, I have absolutely no 
> issue with maintaining methods for saving and restoring only window size, for 
> code that somehow requires that. I'd guess that such code would probably 
> enforce the intended window position itself *after* restoring window size 
> (because that operation *can* affect window position), but in the end that's 
> (indeed) up to the code's developers to decide.
> 
> IOW, I'm perfectly willing to discuss a better solution in which the 
> burden to ensure that window save/restore works as "natively" as possible on 
> each platform is shared. The best way to do that is of course to have a 
> single pair of methods that have platform-specific implementations.
> 
> As far as I'm concerned such a solution might even be prepared completely 
> in KConfig/gui before changes are made everywhere else to deploy that new 
> solution. In that case I would for instance run temporary local (MacPorts) 
> patches that replace saveWindowSize/restoreWindowSize with wrappers for 
> saveWindowGeometry/restoreWindowGeometry.
> 
> Side-observation: OS X (Cocoa) provides a `[NSWindow 
> setFrameAutosaveName:]` method, i.e. it avoids reference to specifics like 
> size or geometry completely.
> 
> That method also provides another thought that could be taken into 
> consideration if it is decided to evolve this part of the frameworks, 
> something I'd be interested in collaborating on. Currently, there is no 
> support for saving and restoring multiple windows per application. That may 
> be more or less sufficient when applications always follow a MDI approach, 
> but even if they do that still doesn't make them applications that are active 
> only in a single instance. Example: KDevelop. One might expect that opening a 
> given, pre-existing session (collection of open projects) restores the main 
> window geometry (size and/or position) that used previously for that session, 
> rather than the geometry used by whatever KDevelop session was run last. On 
> OS X that would be done with something like `[NSWindow 
> setFrameautosaveName:[window

Re: Review Request 126506: Fix improper destruction of non-virtual KDirModel subclasses

[David Faure]

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

Ship it!


Ship It!

- David Faure


On Dec. 25, 2015, 12:12 a.m., Michael Pyne wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126506/
> ---
> 
> (Updated Dec. 25, 2015, 12:12 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Noted by Coverity (CID 1019869), and could result in a set of
> partially-destructed objects. Which, in this case, would probably leak
> memory in the event of multiple levels of filesystem hierarchy from the
> root KDirModel, but wouldn't cause any worse problems than that.
> 
> The 'proper' fix would be to add a virtual dtor at the base class but I
> didn't want to add a vtable just for this, so I mirrored the rest of the
> code and utilized the fact that item().isDir() is true for all derived
> classes.
> 
> 
> Diffs
> -
> 
>   src/widgets/kdirmodel.cpp af56a06 
> 
> Diff: https://git.reviewboard.kde.org/r/126506/diff/
> 
> 
> Testing
> ---
> 
> Builds, kdirmodeltest passes.
> 
> 
> Thanks,
> 
> Michael Pyne
> 
>

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

Re: Review Request 126506: Fix improper destruction of non-virtual KDirModel subclasses

[David Faure]

> On Dec. 25, 2015, 1:58 a.m., Aleix Pol Gonzalez wrote:
> > If making the destructor virtual doesn't break ABI, I'd vote for that...

The node classes are internal, so this wouldn't break ABI indeed.

Hmm, right, this would make "delete node" work everywhere. Michael's fix misses 
the other place where a node is deleted:

2 * delete dirNode->m_childNodes.takeAt(r); in 
KDirModelPrivate::_k_slotDeleteItems
1 * in KDirModelPrivate::_k_slotRefreshItems

You're right, I withdraw my Ship It, we need a virtual dtor instead.


- David


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


On Dec. 25, 2015, 12:12 a.m., Michael Pyne wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126506/
> ---
> 
> (Updated Dec. 25, 2015, 12:12 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Noted by Coverity (CID 1019869), and could result in a set of
> partially-destructed objects. Which, in this case, would probably leak
> memory in the event of multiple levels of filesystem hierarchy from the
> root KDirModel, but wouldn't cause any worse problems than that.
> 
> The 'proper' fix would be to add a virtual dtor at the base class but I
> didn't want to add a vtable just for this, so I mirrored the rest of the
> code and utilized the fact that item().isDir() is true for all derived
> classes.
> 
> 
> Diffs
> -
> 
>   src/widgets/kdirmodel.cpp af56a06 
> 
> Diff: https://git.reviewboard.kde.org/r/126506/diff/
> 
> 
> Testing
> ---
> 
> Builds, kdirmodeltest passes.
> 
> 
> Thanks,
> 
> Michael Pyne
> 
>

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

Re: Review Request 126506: Fix improper destruction of non-virtual KDirModel subclasses

[David Faure]

> On Dec. 25, 2015, 9:14 a.m., David Faure wrote:
> > Ship It!

withdrawn, see above


- David


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


On Dec. 25, 2015, 12:12 a.m., Michael Pyne wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126506/
> ---
> 
> (Updated Dec. 25, 2015, 12:12 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Noted by Coverity (CID 1019869), and could result in a set of
> partially-destructed objects. Which, in this case, would probably leak
> memory in the event of multiple levels of filesystem hierarchy from the
> root KDirModel, but wouldn't cause any worse problems than that.
> 
> The 'proper' fix would be to add a virtual dtor at the base class but I
> didn't want to add a vtable just for this, so I mirrored the rest of the
> code and utilized the fact that item().isDir() is true for all derived
> classes.
> 
> 
> Diffs
> -
> 
>   src/widgets/kdirmodel.cpp af56a06 
> 
> Diff: https://git.reviewboard.kde.org/r/126506/diff/
> 
> 
> Testing
> ---
> 
> Builds, kdirmodeltest passes.
> 
> 
> Thanks,
> 
> Michael Pyne
> 
>

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

Re: Review Request 126170: [OS X] make kded5 an agent, and build it as a regular application instead of an app bundle

[David Faure]

> On Dec. 2, 2015, 7:51 a.m., David Faure wrote:
> > Please kind in mind that kded must be able to pop up dialogs, though.
> > (cookie dialog, SSL cert messagebox + dialog, etc. etc.).
> > 
> > If making it an "agent" doesn't prevent it from showing GUI elements now 
> > and then, then no problem.
> 
> René J.V. Bertin wrote:
> With the earlier approach of setting `LSUIElement` that is guaranteed to 
> be the case.
> 
> I just checked; launching Qt's Assistant with 
> `QT_MAC_DISABLE_FOREGROUND_APPLICATION_TRANSFORM=1` all that changes is that 
> the application remains in the background; it can be brought into the 
> foreground, and it retains its presence in the Dock and app switcher.
> 
> IOW, I'm not really sure I understand why kded5 doesn't retain that 
> presence with `QT_MAC_DISABLE_FOREGROUND_APPLICATION_TRANSFORM` set. It's 
> possible that all the env. variable does is postpone the actions that lead to 
> that presence. If that's true than we'd have to come back to the more 
> appropriate previous revision of this patch.
> 
> OTOH: the only dialogs I have seen under KDE4 that are related to kded 
> (unknown cert) were posted when kded4 was *not* running. Ditto for cookie 
> related things. Under what circumstances is kded supposed to present a GUI?
> 
> David Faure wrote:
> Here is an easy way to test this: do the same change for kiod in kio 
> (it's like a mini kded) and then
> cd kio/tests ; ./listjobtest ftp://t...@upload.kde.org
> should bring up a password dialog.
> 
> Except that with Qt 5.6 from git here (from some time ago) it asserts in 
> DBus (looking into that now)... but hopefully you have Qt 5.5 ?
> 
> René J.V. Bertin wrote:
> OK, here's a reason NOT to use 
> QT_MAC_DISABLE_FOREGROUND_APPLICATION_TRANSFORM: it has to be unset, and I'm 
> not sure when this would have to/could be done such that the variable is 
> picked up for kded5 itself, but not by anything that is launched subsequently.
> 
> If kded5 is used to start kdeinit5 for instances, everything launched 
> through there will launch in the background.
> 
> So I'm going to go back to the original proposition, because an 
> LSUIElement app is exactly what kded ought to be.

I don't understand what you're saying. kded5 isn't starting any other 
processes, is it?


- David


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


On Dec. 25, 2015, 9:14 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126170/
> ---
> 
> (Updated Dec. 25, 2015, 9:14 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: kded
> 
> 
> Description
> ---
> 
> There should be no reason to build `kded5` as an app bundle on OS X, and with 
> recent feedback in mind I postulated that marking it "nongui" 
> (`ecm_mark_nongui_application`) would be acceptable on other platforms too.
> 
> Additionally, `kded5` doesn't have any more reason to appear in the Dock or 
> app switcher, on OS X, so I have added the code to make it an agent.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 5e95df8 
>   src/kded.cpp c7fdfee 
> 
> Diff: https://git.reviewboard.kde.org/r/126170/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.9.5 with Qt 5.5.1 and FWs 5.16.0 .
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

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

Re: Splitting KActivities, 2nd attempt...

[David Faure]

Sounds good overall, but one question:

On Sunday 08 November 2015 12:04:54 Ivan Čukić wrote:
> Hi everybody,
> 
> We have a few problems at the moment because all the activities
> components are in a single repository. Sometimes dependencies of the
> service creep up into the library, builds break because components
> that are meant only for Plasma start requiring Qt versions that Plasma
> require, while KF5 requirement is lower, and similar.
> 
> The way I want to split the repository is as follows:
> 
> - KActivities framework (essentially a Tier 1, integration or solution, all 
> OSs)
> - kactivitymanagerd (service, kcm settings module, dolphin plugin;
> officially linux-only)

Shouldn't the service be part of the framework?
How usable is the framework without the service?
Compiling is good, but running is even better :-)

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 126149: [Icon widget] Bring back properties dialog

[David Faure]

> On Dec. 29, 2015, 6:07 p.m., Kai Uwe Broulik wrote:
> > I completely screwed up my Kate desktop files :( Would it help if I set 
> > NoDisplay or Hidden on the desktop file copy so it's there for the icon 
> > widget but not shown in the Open With dialogs and so on? Or, if I copied 
> > the desktop file elsewhere (eg. not into the local share applications)?
> > 
> > Also, I should probably delete the desktop file again if the widget is 
> > removed :)

If you set NoDisplay or Hidden in ~/.local/share/applications/kate.desktop then 
this will fully hide kate from your K menu. That's a local override.

Desktop files in the panel or on the desktop should probably be copies, yes (in 
other dirs than ~/.local/share/applications/)


- David


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


On Dec. 21, 2015, 11:31 p.m., Kai Uwe Broulik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126149/
> ---
> 
> (Updated Dec. 21, 2015, 11:31 p.m.)
> 
> 
> Review request for KDE Frameworks and Plasma.
> 
> 
> Bugs: 349177
> https://bugs.kde.org/show_bug.cgi?id=349177
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> ---
> 
> This brings back the KPropertiesDialog to modify an icon's appearance. This 
> has been requested at multiple occasions. This has been adapted from the 
> Plasma 4 icon code.
> 
> 
> Diffs
> -
> 
>   applets/icon/package/contents/ui/main.qml 9286b94 
>   applets/icon/plugin/icon_p.h dd7963c 
>   applets/icon/plugin/icon_p.cpp e086870 
> 
> Diff: https://git.reviewboard.kde.org/r/126149/diff/
> 
> 
> Testing
> ---
> 
> Dropped a file from my home onto the desktop -> dialog from the actual file, 
> allowing to rename it. The applet reflected the changes.
> 
> Dropped an application from kickoff to the desktop -> dialog from a copy of 
> the desktop file, allowing to change its icon and description. The applet 
> reflected the changes.
> 
> 
> Thanks,
> 
> Kai Uwe Broulik
> 
>

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

Re: Review Request 126309: backtrace and demangle for OS X, FreeBSD and Solaris/OpenIndiana

[David Faure]

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


This is kdelibs4support, this code is doomed to disappear and apps are using 
kDebug less and less. Is it worth risking compilation breakages on some systems?

Also I found kBacktrace less and less useful over the years because with hidden 
visibility I get a lot of "???" for non-exported methods. gdb works much better.

- David Faure


On Dec. 10, 2015, 10:10 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126309/
> ---
> 
> (Updated Dec. 10, 2015, 10:10 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: kdelibs4support
> 
> 
> Description
> ---
> 
> This is a "backport" of the patches to `kdebug.cpp` that enable backtrace and 
> demangling support on OS X, FreeBSD and Solaris/OpenIndiana.
> The KDE4 version was discussed here: https://git.reviewboard.kde.org/r/121213/
> 
> It seems that change was never incorporated because of a single open issue 
> for which I never found the time (also given that it seemed a bit overkill).
> 
> My PC-BSD and Indiana VMs are no longer operational; it seems highly likely 
> that the current code still works but if further testing or polishing is 
> required I'll rather remove the specific parts than bring the VMs online 
> again.
> 
> 
> Diffs
> -
> 
>   src/kdecore/kdebug.cpp 6f04dce 
> 
> Diff: https://git.reviewboard.kde.org/r/126309/diff/
> 
> 
> Testing
> ---
> 
> On Kubuntu 14.04 with various gcc versions and clang; OS X 10.6 - 10.9 with 
> gcc and clang, PC-BSD with clang and on Open Indiana. 
> 
> The KDE4 RR raises some doubts concerning checking for only an OS and not 
> compilers (in demangling). I think there is no reason for such doubts: 
> compilers are obliged to co-exist and be compatible nowadays, at least on 
> individual OS families (each platform will have its own default/dominant 
> compiler that is used to build the system libraries). In practice it turns 
> out that gcc and clang use the same C++ mangling scheme. The only difference 
> is in the way `backtrace_symbols()` formats the stack, and that indeed 
> appears to defined the OS rather than by the compiler used.
> (Then again I'm willing to stand corrected by someone who has a Linux system 
> built from scratch with clang and libc++, or possibly a Gnu/BSD set-up :))
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

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

Re: Review Request 126313: Use an xcb for interaction with KStartupInfo

[David Faure]

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

Ship it!


Ship It!

- David Faure


On Dec. 11, 2015, 2:08 p.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126313/
> ---
> 
> (Updated Dec. 11, 2015, 2:08 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kinit
> 
> 
> Description
> ---
> 
> By changing to xcb we can use NETRootInfo to get the current desktop
> and don't need to duplicate the logic. Also it means that more code
> is ported from XLib to xcb and might allow us to drop the XLib
> dependency in future.
> 
> 
> Diffs
> -
> 
>   src/kdeinit/kinit.cpp a18008a11bf00a35aa0cab450180926217cd58f5 
>   src/kdeinit/CMakeLists.txt f94db717f2403b602648286eac243837d8714069 
>   src/config-kdeinit.h.cmake cfcfc61a1bb560ff9a902b89bd3b8fb27273ebf2 
>   CMakeLists.txt eeecab775cfb5dfe12cf9c4991c658cc261ed727 
> 
> Diff: https://git.reviewboard.kde.org/r/126313/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

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

Re: Review Request 126385: PartManager: stop tracking a widget even if it is no longer a top level window

[David Faure]

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

Ship it!


I agree -- except about the "aside" in the commit log:

"since all signal connections to 'this' are removed on destruction anyway". 
They are indeed, but *after* destroyed is emitted. So disconnecting does make a 
difference.

- David Faure


On Dec. 16, 2015, 1:33 p.m., Jonathan Marten wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126385/
> ---
> 
> (Updated Dec. 16, 2015, 1:33 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Bugs: 355711
> https://bugs.kde.org/show_bug.cgi?id=355711
> 
> 
> Repository: kparts
> 
> 
> Description
> ---
> 
> This appears to be the cause of a crash when exiting System Settings.  More 
> information in the bug report, but basically what happens is that the manager 
> keeps track of widgets that it is managing in d->m_managedTopLevelWidgets.  
> If a widget is a top level widget when it is added, but is no longer top 
> level when it is destroyed, it is not removed from the list which results in 
> an access-to-deleted-object in the destructor.
> 
> This change unconditionally removes the widget even if it is no longer top 
> level.  Removing the widget from the list has no ill effects, the list is 
> only actually used in Partmanager::eventFilter() which will never get an 
> event for a deleted widget anyway.
> 
> Aside:  The problematic 'foreach (const QWidget *w, 
> d->m_managedTopLevelWidgets)' loop in PartManager::PartManager() is really 
> superfluous, since all signal connections to 'this' are removed on 
> destruction anyway.
> 
> 
> Diffs
> -
> 
>   src/partmanager.cpp 81bf73f 
> 
> Diff: https://git.reviewboard.kde.org/r/126385/diff/
> 
> 
> Testing
> ---
> 
> Built KParts with this change, and also systemsettings5 with the associated 
> change (see associated review).  Observed no crash when exiting the 
> application.  Also checked correct operation of Konqueror and Kate.
> 
> 
> Thanks,
> 
> Jonathan Marten
> 
>

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

Re: Review Request 126309: backtrace and demangle for OS X, FreeBSD and Solaris/OpenIndiana

[David Faure]

> On Jan. 1, 2016, 3:04 p.m., David Faure wrote:
> > This is kdelibs4support, this code is doomed to disappear and apps are 
> > using kDebug less and less. Is it worth risking compilation breakages on 
> > some systems?
> > 
> > Also I found kBacktrace less and less useful over the years because with 
> > hidden visibility I get a lot of "???" for non-exported methods. gdb works 
> > much better.
> 
> René J.V. Bertin wrote:
> I'd hope that we can avoid the compilation breakages and have no idea of 
> the timescale of planned obsolescence.
> 
> I'm actually still getting pretty useful backtraces in KDE4 applications; 
> I'd have to see how that works out for Qt5 apps (QtCurve uses 
> kdelibs4support; I could use that to get kbacktraces from pretty "deep" 
> places ;) ).
> 
> So how does DrKonqi/KF5 work? Doesn't it use equivalent code if not built 
> on kdelibs4support itself?

My problems where in kde4 apps.

drkonqi attaches a debugger, it does not use a backtrace function.


- David


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


On Dec. 10, 2015, 10:10 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126309/
> ---
> 
> (Updated Dec. 10, 2015, 10:10 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: kdelibs4support
> 
> 
> Description
> ---
> 
> This is a "backport" of the patches to `kdebug.cpp` that enable backtrace and 
> demangling support on OS X, FreeBSD and Solaris/OpenIndiana.
> The KDE4 version was discussed here: https://git.reviewboard.kde.org/r/121213/
> 
> It seems that change was never incorporated because of a single open issue 
> for which I never found the time (also given that it seemed a bit overkill).
> 
> My PC-BSD and Indiana VMs are no longer operational; it seems highly likely 
> that the current code still works but if further testing or polishing is 
> required I'll rather remove the specific parts than bring the VMs online 
> again.
> 
> 
> Diffs
> -
> 
>   src/kdecore/kdebug.cpp 6f04dce 
> 
> Diff: https://git.reviewboard.kde.org/r/126309/diff/
> 
> 
> Testing
> ---
> 
> On Kubuntu 14.04 with various gcc versions and clang; OS X 10.6 - 10.9 with 
> gcc and clang, PC-BSD with clang and on Open Indiana. 
> 
> The KDE4 RR raises some doubts concerning checking for only an OS and not 
> compilers (in demangling). I think there is no reason for such doubts: 
> compilers are obliged to co-exist and be compatible nowadays, at least on 
> individual OS families (each platform will have its own default/dominant 
> compiler that is used to build the system libraries). In practice it turns 
> out that gcc and clang use the same C++ mangling scheme. The only difference 
> is in the way `backtrace_symbols()` formats the stack, and that indeed 
> appears to defined the OS rather than by the compiler used.
> (Then again I'm willing to stand corrected by someone who has a Linux system 
> built from scratch with clang and libc++, or possibly a Gnu/BSD set-up :))
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

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

Re: Review Request 126426: Add a warning color to kwalletd's password dialogs

[David Faure]

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

Ship it!


Ship It!

- David Faure


On Dec. 24, 2015, 5:55 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126426/
> ---
> 
> (Updated Dec. 24, 2015, 5:55 p.m.)
> 
> 
> Review request for KDE Frameworks, KDE Usability and Valentin Rusu.
> 
> 
> Repository: kwallet
> 
> 
> Description
> ---
> 
> Set a "background warning color" when password and its verification do not 
> match, in the KNewPasswordDialog run by kwalletd.
> This adds a new dependency (needed to avoid hardcoding colors), but I think 
> is a nice feature to have.
> See RR 125619 for a screenshot: 
> https://git.reviewboard.kde.org/r/125619/file/2515/
> 
> 
> Diffs
> -
> 
>   src/runtime/kwalletd/CMakeLists.txt 
> ba9e7ba508c74fed1d8101496583061245640aa7 
>   src/runtime/kwalletd/kwalletd.cpp 6da97031e13240a9630cfa7e0dc3cf42575819c4 
> 
> Diff: https://git.reviewboard.kde.org/r/126426/diff/
> 
> 
> Testing
> ---
> 
> Compiles fine.
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

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

Re: Review Request 126429: Fixed all Clazy warnings level 1 and level 2

[David Faure]

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

Ship it!


Ship It!

- David Faure


On Dec. 23, 2015, 7:50 p.m., Artur Puzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126429/
> ---
> 
> (Updated Dec. 23, 2015, 7:50 p.m.)
> 
> 
> Review request for KDE Frameworks, Aleix Pol Gonzalez and Sergio Martins.
> 
> 
> Repository: karchive
> 
> 
> Description
> ---
> 
> Fixed warning: Folder has dtor but not copy-ctor, copy-assignment 
> [-Wclazy-rule-of-three], by adding Q_DISABLE_COPY(Folder)
> Fixed warning: unused variable [-Wunused-const-variable], by commenting 
> unused variables
> 
> 
> Diffs
> -
> 
>   src/k7zip.cpp 7b5e6a3 
> 
> Diff: https://git.reviewboard.kde.org/r/126429/diff/
> 
> 
> Testing
> ---
> 
> Automated tests pass.
> 
> 
> Thanks,
> 
> Artur Puzio
> 
>

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

Re: Review Request 126453: Fix library order

[David Faure]

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

Ship it!


Hmm, OK. It's really not obvious that order matters here (or what the right 
order would be). It all depends on what you have in /usr/include. But ok.

- David Faure


On Dec. 21, 2015, 4:06 p.m., Kevin Funk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126453/
> ---
> 
> (Updated Dec. 21, 2015, 4:06 p.m.)
> 
> 
> Review request for KDE Frameworks, Heiko Becker and Jeremy Whiting.
> 
> 
> Repository: knewstuff
> 
> 
> Description
> ---
> 
> Fixes issues leading to creation of QTBUG-47240
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt cc606444e48b0e519551183c022ccecdac0aa62f 
> 
> Diff: https://git.reviewboard.kde.org/r/126453/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Funk
> 
>

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

Re: Review Request 126474: Port QRegExp to QRegularExpression in kshorturifilter

[David Faure]

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

Ship it!



src/urifilters/shorturi/kshorturifilter.cpp (line 58)
<https://git.reviewboard.kde.org/r/126474/#comment61838>

"despite" sounds like the api docs say that it's not thread safe. AFAICS 
the docs don't say anything either way. I agree that one shouldn't assume 
thread-safety unless explicitly documented, but I think it's just an omission 
in the doc, the whole point of the QRegularExpression API is thread safety.


- David Faure


On Dec. 28, 2015, 2:20 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126474/
> ---
> 
> (Updated Dec. 28, 2015, 2:20 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> A static QRegExp was used but it is not thread safe. QRegularExpression
> seems to be.
> 
> BUG: 352356
> 
> 
> Diffs
> -
> 
>   src/urifilters/shorturi/kshorturifilter.cpp 
> 6002ec6925c0acdd20a053f98baca46863f69fa6 
> 
> Diff: https://git.reviewboard.kde.org/r/126474/diff/
> 
> 
> Testing
> ---
> 
> I ran the autotests which includes urifilter and I've run krunner which uses 
> it extensively.
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

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

Re: KHolidays as Framework (redux)

[David Faure]

On Thursday 24 December 2015 12:28:13 John Layt wrote:
> Hi,
> 
> It's xmas holidays, so it must be time to poke a stick at KHolidays again
> for inclusion as a Framework. As far as I am aware there are no outstanding
> porting issues with KHolidays and it is ready for review to be included as
> a Tier 1 Framework in the next possible release. What's the next step?

Please make sure it passes all of the items in this checklist
https://community.kde.org/Frameworks/CreationGuidelines

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 126507: Fix leaked file description and potential use-after-free in kdelibs4support

[David Faure]

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



src/kdeui/k4style.cpp (line 1213)
<https://git.reviewboard.kde.org/r/126507/#comment61842>

this condition used to be evaluated when tiles was non-null (i.e. key in 
cache). In your patch, key-in-cache goes to the very first branch and not here 
anymore.


- David Faure


On Dec. 25, 2015, 12:16 a.m., Michael Pyne wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126507/
> ---
> 
> (Updated Dec. 25, 2015, 12:16 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs4support
> 
> 
> Description
> ---
> 
> Fix a couple of Coverity issues:
> 
> 1. CID 1175508; file descriptors used in KLockFile could be leaked in
> error conditions. Even when KLockFile sets mustCloseFd, the dtor's impl
> also checks that the lock has been taken, which is only considered true
> if LockOK had been returned in our lock function. Instead close() the fd
> ourselves unless we make it to LockOK.
> 
> 2. CID 117; The standard mis-use of QCache. QCache::insert can, in
> theory, delete our object as soon as we insert it into cache, so we have
> to check for that. Even ::contains() and ::object() can be risky (the
> pointers returned by object() have no lifetime guarantee), but since
> this is GUI code I assume it's only used single-threaded and not
> re-entrant. Otherwise we'd need even more paranoia...
> 
> 
> Diffs
> -
> 
>   src/kdecore/klockfile_unix.cpp 67283a5 
>   src/kdeui/k4style.cpp a1a2ab1 
> 
> Diff: https://git.reviewboard.kde.org/r/126507/diff/
> 
> 
> Testing
> ---
> 
> Everything builds and appears to still work, though it's hard to test K4Style 
> as I'm not sure what uses it right at this point.
> 
> 
> Thanks,
> 
> Michael Pyne
> 
>

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

Re: Splitting KActivities, 2nd attempt...

[David Faure]

On Friday 01 January 2016 18:07:59 Ivan Čukić wrote:
> 
> So, applications that do link to activities will run and work
> uninterrupted even if the service is not present.

OK, sounds good to me. 

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 126474: Port QRegExp to QRegularExpression in kshorturifilter

[David Faure]

> On Jan. 1, 2016, 5:15 p.m., David Faure wrote:
> > src/urifilters/shorturi/kshorturifilter.cpp, line 58
> > <https://git.reviewboard.kde.org/r/126474/diff/2/?file=426362#file426362line58>
> >
> > "despite" sounds like the api docs say that it's not thread safe. 
> > AFAICS the docs don't say anything either way. I agree that one shouldn't 
> > assume thread-safety unless explicitly documented, but I think it's just an 
> > omission in the doc, the whole point of the QRegularExpression API is 
> > thread safety.

I talked to Giuseppe, he'll update the docu to mention thread safety.


- David


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


On Dec. 28, 2015, 2:20 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126474/
> ---
> 
> (Updated Dec. 28, 2015, 2:20 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> A static QRegExp was used but it is not thread safe. QRegularExpression
> seems to be.
> 
> BUG: 352356
> 
> 
> Diffs
> -
> 
>   src/urifilters/shorturi/kshorturifilter.cpp 
> 6002ec6925c0acdd20a053f98baca46863f69fa6 
> 
> Diff: https://git.reviewboard.kde.org/r/126474/diff/
> 
> 
> Testing
> ---
> 
> I ran the autotests which includes urifilter and I've run krunner which uses 
> it extensively.
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

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

Re: Review Request 126403: Get rid of QApplication dependency

[David Faure]

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



src/platforms/xcb/kwindowsystem.cpp (line 59)
<https://git.reviewboard.kde.org/r/126403/#comment61834>

move into the if() to save some work if isDirty==false?



src/platforms/xcb/kwindowsystem.cpp (line 890)
<https://git.reviewboard.kde.org/r/126403/#comment61847>

isn't that "return displayGeometry()?"

If not, then at least something like QRect(QPoint(0,0), 
displayGeometry().size()) (encapsulated in a convenience function) would avoid 
calling displayGeometry() twice.


- David Faure


On Dec. 17, 2015, 3:38 p.m., Thomas Lübking wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126403/
> ---
> 
> (Updated Dec. 17, 2015, 3:38 p.m.)
> 
> 
> Review request for KDE Frameworks, kwin and Albert Astals Cid.
> 
> 
> Bugs: 354811
> https://bugs.kde.org/show_bug.cgi?id=354811
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> summarized, alternative to https://git.reviewboard.kde.org/r/126397/
> 
> NOTICE: this compiles but is otherwise *completely* untested!
> 
> 
> Diffs
> -
> 
>   src/platforms/xcb/kwindowsystem.cpp 9d28704 
> 
> Diff: https://git.reviewboard.kde.org/r/126403/diff/
> 
> 
> Testing
> ---
> 
> no, not the least. esp. not on the bug.
> 
> 
> Thanks,
> 
> Thomas Lübking
> 
>

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

Re: Review Request 126507: Fix leaked file description and potential use-after-free in kdelibs4support

[David Faure]

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



src/kdeui/k4style.cpp (line 1176)
<https://git.reviewboard.kde.org/r/126507/#comment61851>

The double-lookup looks slow and unnecessary.
contains + *object doesn't give you anything that object (followed by a 
dereference if not null) gives you.

If the fear is concurrent access, then contains + *object can still 
dereference a null pointer anyway.

But K4Style runs in the GUI thread always anyway. So I would call object(), 
put that into a pointer, and then if not null, make a copy into a value. This 
avoids the double lookup.


- David Faure


On Jan. 2, 2016, 1:55 a.m., Michael Pyne wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126507/
> ---
> 
> (Updated Jan. 2, 2016, 1:55 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs4support
> 
> 
> Description
> ---
> 
> Fix a couple of Coverity issues:
> 
> 1. CID 1175508; file descriptors used in KLockFile could be leaked in
> error conditions. Even when KLockFile sets mustCloseFd, the dtor's impl
> also checks that the lock has been taken, which is only considered true
> if LockOK had been returned in our lock function. Instead close() the fd
> ourselves unless we make it to LockOK.
> 
> 2. CID 117; The standard mis-use of QCache. QCache::insert can, in
> theory, delete our object as soon as we insert it into cache, so we have
> to check for that. Even ::contains() and ::object() can be risky (the
> pointers returned by object() have no lifetime guarantee), but since
> this is GUI code I assume it's only used single-threaded and not
> re-entrant. Otherwise we'd need even more paranoia...
> 
> 
> Diffs
> -
> 
>   src/kdecore/klockfile_unix.cpp 67283a5 
>   src/kdeui/k4style.cpp a1a2ab1 
> 
> Diff: https://git.reviewboard.kde.org/r/126507/diff/
> 
> 
> Testing
> ---
> 
> Everything builds and appears to still work, though it's hard to test K4Style 
> as I'm not sure what uses it right at this point.
> 
> 
> Thanks,
> 
> Michael Pyne
> 
>

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

Re: Review Request 126505: Do not show a warning color before the user even started typing

[David Faure]

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

Ship it!


Ship It!

- David Faure


On Dec. 27, 2015, 2:40 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126505/
> ---
> 
> (Updated Dec. 27, 2015, 2:40 p.m.)
> 
> 
> Review request for KDE Frameworks, KDE Usability, Christoph Feck, and David 
> Faure.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> As discussed in RR 125619 and 126426, the password verification field (in a 
> KNewPasswordWidget) should not be marked as "wrong" before the user even 
> started typing the verification password.
> 
> The revised approach is the following:
> 
> * The user starts typing something as password, e.g. 1234
> * The user types something else as verification password
> * As soon as the verification is not anymore a prefix of the password (e.g. 
> verification = 122), the warning color is shown.
> * As soon as the verification is a prefix again (e.g. the user deletes the 
> second 2, i.e. verification = 12) the warning color is not shown anymore.
> 
> 
> Diffs
> -
> 
>   autotests/knewpasswordwidgettest.h 43845128adec01aced4353c9f7986b7977829a2a 
>   autotests/knewpasswordwidgettest.cpp 
> 297b88d5f18b9cd37f0d26d94e56f38870756f20 
>   src/knewpasswordwidget.cpp a1b59454a2c2d7c09ac32acec52d3fffa73f77fc 
> 
> Diff: https://git.reviewboard.kde.org/r/126505/diff/
> 
> 
> Testing
> ---
> 
> Autotests assert what described above. Gif pictures would explain the patch 
> better than 1000 words, but I suck at creating them :(
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

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

Re: Review Request 126161: OS X housekeeping

[David Faure]

> On Dec. 25, 2015, 2:42 p.m., René J.V. Bertin wrote:
> > src/kdeinit/kinit_mac.mm, lines 662-666
> > 
> >
> > I'd love to add `[NSApp activateIgnoringOtherApps:YES]` and/or `[NSApp 
> > unhide]` here, preceded by `[NSApplication sharedApplication]` so that the 
> > spawned `executable` appears in front and not behind the windows of the 
> > "parent" application.
> > 
> > I think that's a very sensible thing to do, but sadly those calls are 
> > part of (or call into) the SDKs that are off-limits between a `fork()` and 
> > an `exec()`.
> > 
> > This really makes me reconsider to use a wrapper, because it quickly 
> > grows old 1) having to wait for an expected application to appear, 2) 
> > realise it must have opened somewhere in the background and 3) go dig it up.
> > 
> > If only I could be sure that the spawned application is NOT supposed to 
> > inherit the environment and other context from kdeinit5. In that case I 
> > could rewrite the command to execute so that it uses LaunchServices ... via 
> > a proxy that's already available (`/usr/bin/open`).
> > 
> > NB: this may well be why `[NSProcessInfo setProcessName:]` doesn't 
> > appear to have the intended effect.
> > 
> > So, David, what's worse here — introducing an additional layer or 
> > putting up with applications that play hide and seek?

Starting apps via kdeinit is only an optimization. It's perfectly OK to start 
apps directly with QProcess instead. So yes, the spawned application is NOT 
supposed to inherit the environment from kdeinit5.

As previously stated, I strongly recommend to avoid the whole complication of 
"stuff I can't do between fork and exec" on OSX, but not using fork and exec at 
all (wrapper or not), and just starting the other process directly with 
QProcess. If that doesn't bring the new app to the front, maybe that's a 
QProcess bug or missing feature? If you think about this as a "pure Qt 
application developer", such a developer would expect QProcess to be able to 
start a GUI app and have it pop up to the front. Maybe you can write a small 
test with QProcess, to check if that works out of the box (maybe all your 
troubles actually come from the indirection via kdeinit...)


- David


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


On Nov. 26, 2015, 4:20 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126161/
> ---
> 
> (Updated Nov. 26, 2015, 4:20 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: kinit
> 
> 
> Description
> ---
> 
> This patch addresses several issues with the OS X adaptations:
> 
> -1 replaces the obsolete Q_OS_MAC with Q_OS_OSX
> -2 builds the relevant applications `nongui` instead of as app bundles
> -3 turns klauncher into an "agent" by setting `LSUIElement` to true 
> programmatically
> -4 ports a patch that has been in MacPorts' `port:kdelibs4` since October 
> 14th 2009, which prevents a kdeinit crash that is caused by calling exec 
> after `fork()` in an application that has used non-POSIX APIs and/or calling 
> those APIs in the exec'ed application. This patch (originally by MacPorts 
> developers Jeremy Lainé and Jeremy Lavergne) rearranges call order and uses a 
> proxy application to do the actual exec.
> 
> 
> Diffs
> -
> 
>   src/kdeinit/CMakeLists.txt f94db71 
>   src/kdeinit/kdeinit5_proxy.mm PRE-CREATION 
>   src/kdeinit/kinit.cpp a18008a 
>   src/kdeinit/kinit_mac.mm PRE-CREATION 
>   src/klauncher/CMakeLists.txt 746edfa 
>   src/klauncher/klauncher.h e155f72 
>   src/klauncher/klauncher.cpp 8b3d343 
>   src/klauncher/klauncher_main.cpp f69aaa5 
>   src/start_kdeinit/CMakeLists.txt 46d6cb3 
>   src/wrapper.cpp 95b7ec2 
> 
> Diff: https://git.reviewboard.kde.org/r/126161/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.9.5 with Qt 5.5.1 and KF5rameworks 5.16.0 . With this patch, 
> starting `kded5` will launch kdeinit5 and klauncher5 as expected, but 
> `kdeinit5 --kded` does not yet launch `kded5`. This is probably acceptable 
> for typical KF5 use on OS X (kded5 can be launched as a login item or as a 
> LaunchAgent) but I will have another look at why the kded isn't started.
> 
> I am not yet able to perform further testing; practice will for instance have 
> to show whether point 2 above needs revision (apps that need to be installed 
> as app bundles).
> 
> Similarly it will have to be seen whether point 3 above has any drawbacks. 
> Applications running as agents do not show up in the Dock or App Switcher. 
> Thus

Re: Review Request 126170: [OS X] make kded5 an agent, and build it as a regular application instead of an app bundle

[David Faure]

> On Dec. 2, 2015, 7:51 a.m., David Faure wrote:
> > Please kind in mind that kded must be able to pop up dialogs, though.
> > (cookie dialog, SSL cert messagebox + dialog, etc. etc.).
> > 
> > If making it an "agent" doesn't prevent it from showing GUI elements now 
> > and then, then no problem.
> 
> René J.V. Bertin wrote:
> With the earlier approach of setting `LSUIElement` that is guaranteed to 
> be the case.
> 
> I just checked; launching Qt's Assistant with 
> `QT_MAC_DISABLE_FOREGROUND_APPLICATION_TRANSFORM=1` all that changes is that 
> the application remains in the background; it can be brought into the 
> foreground, and it retains its presence in the Dock and app switcher.
> 
> IOW, I'm not really sure I understand why kded5 doesn't retain that 
> presence with `QT_MAC_DISABLE_FOREGROUND_APPLICATION_TRANSFORM` set. It's 
> possible that all the env. variable does is postpone the actions that lead to 
> that presence. If that's true than we'd have to come back to the more 
> appropriate previous revision of this patch.
> 
> OTOH: the only dialogs I have seen under KDE4 that are related to kded 
> (unknown cert) were posted when kded4 was *not* running. Ditto for cookie 
> related things. Under what circumstances is kded supposed to present a GUI?
> 
> David Faure wrote:
> Here is an easy way to test this: do the same change for kiod in kio 
> (it's like a mini kded) and then
> cd kio/tests ; ./listjobtest ftp://t...@upload.kde.org
> should bring up a password dialog.
> 
> Except that with Qt 5.6 from git here (from some time ago) it asserts in 
> DBus (looking into that now)... but hopefully you have Qt 5.5 ?
> 
> René J.V. Bertin wrote:
> OK, here's a reason NOT to use 
> QT_MAC_DISABLE_FOREGROUND_APPLICATION_TRANSFORM: it has to be unset, and I'm 
> not sure when this would have to/could be done such that the variable is 
> picked up for kded5 itself, but not by anything that is launched subsequently.
> 
> If kded5 is used to start kdeinit5 for instances, everything launched 
> through there will launch in the background.
> 
> So I'm going to go back to the original proposition, because an 
> LSUIElement app is exactly what kded ought to be.
> 
> David Faure wrote:
> I don't understand what you're saying. kded5 isn't starting any other 
> processes, is it?
> 
> René J.V. Bertin wrote:
> kdeinit5 is auto-started as part of what I think is normal behaviour. So 
> if kded5 is started first, that's what starts kdeinit5. Shouldn't it?
> 
> My reasoning here is that users might be interested in the possibility to 
> prepare the KDE runtime infrastructure with an inoffensive and non-invasive 
> daemon process that is part of the infrastructure itself. It is my experience 
> with KDE4/Mac that not doing so, but leaving the bootstrapping until you 
> start some larger and (much) more complex application (or suite; think 
> starting Akonadi) can lead to subtle but annoying stability issues.
> 
> NB: starting kded5 through kdeinit5 does *not* work on OS X. I've had a 
> quick look to understand why, but quickly gave up due to the complexity of 
> the code.

If a user wants to prepare the runtime infrastructure, he/she should run 
kdeinit5, not kded5.
kdeinit5 is the master of everything; kded is just a bunch of on-demand 
services.

Apps started standalone, who then make a dbus call to kded might indeed start 
kded first, which would in turn start kdeinit.
But yeah, unset any env vars in kdeinit that shouldn't be set for the apps it 
starts, that makes sense.


- David


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


On Dec. 25, 2015, 9:14 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126170/
> ---
> 
> (Updated Dec. 25, 2015, 9:14 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: kded
> 
> 
> Description
> ---
> 
> There should be no reason to build `kded5` as an app bundle on OS X, and with 
> recent feedback in mind I postulated that marking it "nongui" 
> (`ecm_mark_nongui_application`) would be acceptable on other platforms too.
> 
> Additionally, 

Re: Review Request 126314: Port klauncher to xcb

[David Faure]

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

Ship it!


Ship It!

- David Faure


On Dec. 11, 2015, 3:10 p.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126314/
> ---
> 
> (Updated Dec. 11, 2015, 3:10 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kinit
> 
> 
> Description
> ---
> 
> Port klauncher to xcb
> 
> 
> Diffs
> -
> 
>   src/klauncher/klauncher.cpp 8b3d343b9fb2c852ea2d6c559f13c96a0467d72b 
>   src/klauncher/CMakeLists.txt 746edfac0b840aa033be219ae5d094689006db6f 
>   src/klauncher/klauncher.h e155f72d76d4f99172646ab797ec8c4dd006ddf7 
> 
> Diff: https://git.reviewboard.kde.org/r/126314/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

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

Re: closest equivalent to KApplicationPrivate::init() ?

[David Faure]

On Saturday 02 January 2016 10:49:59 René J.V. Bertin wrote:
> Morning!
> 
> Is there anything like a closest equivalent to KApplicationPrivate::init() in 
> KF5 Frameworks?

Yes, QApplicationPrivate::init() :-)

> I added a "bring application to the foreground" call (to a KWindowSystem 
> helper function) in there, which alleviated the issue of applications 
> remaining somewhere in the background on OS X. 
> I'm still keeping an eye out for central locations where I could call that 
> helper function.

This would break focus-stealing-prevention.
Apps started while the user is typing elsewhere shouldn't be brought to the 
foreground.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 126170: [OS X] make kded5 an agent, and build it as a regular application instead of an app bundle

[David Faure]

> On Dec. 2, 2015, 7:51 a.m., David Faure wrote:
> > Please kind in mind that kded must be able to pop up dialogs, though.
> > (cookie dialog, SSL cert messagebox + dialog, etc. etc.).
> > 
> > If making it an "agent" doesn't prevent it from showing GUI elements now 
> > and then, then no problem.
> 
> René J.V. Bertin wrote:
> With the earlier approach of setting `LSUIElement` that is guaranteed to 
> be the case.
> 
> I just checked; launching Qt's Assistant with 
> `QT_MAC_DISABLE_FOREGROUND_APPLICATION_TRANSFORM=1` all that changes is that 
> the application remains in the background; it can be brought into the 
> foreground, and it retains its presence in the Dock and app switcher.
> 
> IOW, I'm not really sure I understand why kded5 doesn't retain that 
> presence with `QT_MAC_DISABLE_FOREGROUND_APPLICATION_TRANSFORM` set. It's 
> possible that all the env. variable does is postpone the actions that lead to 
> that presence. If that's true than we'd have to come back to the more 
> appropriate previous revision of this patch.
> 
> OTOH: the only dialogs I have seen under KDE4 that are related to kded 
> (unknown cert) were posted when kded4 was *not* running. Ditto for cookie 
> related things. Under what circumstances is kded supposed to present a GUI?
> 
> David Faure wrote:
> Here is an easy way to test this: do the same change for kiod in kio 
> (it's like a mini kded) and then
> cd kio/tests ; ./listjobtest ftp://t...@upload.kde.org
> should bring up a password dialog.
> 
> Except that with Qt 5.6 from git here (from some time ago) it asserts in 
> DBus (looking into that now)... but hopefully you have Qt 5.5 ?
> 
> René J.V. Bertin wrote:
> OK, here's a reason NOT to use 
> QT_MAC_DISABLE_FOREGROUND_APPLICATION_TRANSFORM: it has to be unset, and I'm 
> not sure when this would have to/could be done such that the variable is 
> picked up for kded5 itself, but not by anything that is launched subsequently.
> 
> If kded5 is used to start kdeinit5 for instances, everything launched 
> through there will launch in the background.
> 
> So I'm going to go back to the original proposition, because an 
> LSUIElement app is exactly what kded ought to be.
> 
> David Faure wrote:
> I don't understand what you're saying. kded5 isn't starting any other 
> processes, is it?
> 
> René J.V. Bertin wrote:
> kdeinit5 is auto-started as part of what I think is normal behaviour. So 
> if kded5 is started first, that's what starts kdeinit5. Shouldn't it?
> 
> My reasoning here is that users might be interested in the possibility to 
> prepare the KDE runtime infrastructure with an inoffensive and non-invasive 
> daemon process that is part of the infrastructure itself. It is my experience 
> with KDE4/Mac that not doing so, but leaving the bootstrapping until you 
> start some larger and (much) more complex application (or suite; think 
> starting Akonadi) can lead to subtle but annoying stability issues.
> 
> NB: starting kded5 through kdeinit5 does *not* work on OS X. I've had a 
> quick look to understand why, but quickly gave up due to the complexity of 
> the code.
> 
> David Faure wrote:
> If a user wants to prepare the runtime infrastructure, he/she should run 
> kdeinit5, not kded5.
> kdeinit5 is the master of everything; kded is just a bunch of on-demand 
> services.
> 
> Apps started standalone, who then make a dbus call to kded might indeed 
> start kded first, which would in turn start kdeinit.
> But yeah, unset any env vars in kdeinit that shouldn't be set for the 
> apps it starts, that makes sense.
> 
> René J.V. Bertin wrote:
> > If a user wants to prepare the runtime infrastructure, he/she should 
> run kdeinit5, not kded5.
> 
> The thing with that is that s/he would then have to launch 2 applications.
> 
> > Apps started standalone, who then make a dbus call to kded might indeed 
> start kded first
> 
> If that is also how `kdeinit5 --kded` starts kded, then that won't work. 
> The KDE daemon has always been tricky on OS X, and it just works best in 
> practice to let that application be the KDE bootstrap utility. We have to 
> take into consideration the fact that the session dbus itself is started 
> asynchronously through launchd, which makes relying on its presence (and 
> being operational) in order to launch other services tricky at best.
> 
> A "bunch of on-demand services" itself started on explicit user demand 
> and wh

Re: closest equivalent to KApplicationPrivate::init() ?

[David Faure]

On Saturday 02 January 2016 13:46:17 René J.V. Bertin wrote:
> On Saturday January 02 2016 12:50:59 David Faure wrote:
> 
> > > I added a "bring application to the foreground" call (to a KWindowSystem 
> > > helper function) in there, which alleviated the issue of applications 
> > > remaining somewhere in the background on OS X. 
> > > I'm still keeping an eye out for central locations where I could call 
> > > that helper function.
> > 
> > This would break focus-stealing-prevention.
> > Apps started while the user is typing elsewhere shouldn't be brought to the 
> > foreground.
> 
> Yeah, well, there's a fine line to walk there. It's not like it never happens 
> on Linux either. In fact, it happens much more often on Linux than with 
> native OS X applications, even if OS X didn't always do so well. You can 
> (again) start an app (via Dock or Finder) and then switch back immediately to 
> whatever other app you were using, and the new app will open just behind the 
> one you switched back to.
> I *think* that this is something LaunchServices takes care of.
> 
> Of course that all doesn't apply to popup messages, password dialogs and the 
> like. Hence the fine line remark: I think everyone is used to focus being 
> stolen in certain circumstances. I won't call it a small price to pay, but 
> you can also consider that a "feature" causing new applications to open in 
> the background systematically is a form of focus stealing - a more annoying 
> one at that because most of the time you don't notice that it's happening.

Sounds to me like you're saying "better bring all apps to front than none".

Maybe, maybe not, but what we need to continue this discussion is hard data.
1) what does QProcess on Mac do (always in front, always in background, depends 
on whether the user is typing)
2) can QProcess be improved
3) can we use QProcess from KF5 instead of kdeinit (because, again, kdeinit's 
only purpose is fork+exec
which is apparently not a good solution on Mac). I'm pretty sure the answer to 
this one is yes.
E.g. if you set bool useKToolInvocation to false in krun.cpp:724, you'll get 
into the code path that uses QProcess.
(which btw shows another reason to ensure QProcess works: kdeinit isn't even 
always used, even on Linux,
see the conditional for useKToolInvocation).

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

test failures on http://ci-logs.kde.flaska.net

[David Faure]

On Saturday 07 November 2015 21:42:33 Jan Kundrát wrote:
> 
> > Can you the value of 
> > QMimeDatabase().mimeTypeForName("text/plain").preferredSuffix()
> > on your system? Here's it's "txt", I suspect it's "doc" on your 
> > system. Not sure why yet
> > though, but let's first check that.

Ah, I found the issue.

2016-01-01 20:36:41,206 [output] FAIL!  : KNewFileMenuTest::test(text file with 
jpeg extension) Compared values are not the same
2016-01-01 20:36:41,206 [output]Actual   (emittedUrl.toLocalFile()): 
"/tmp/knewfilemenutest-oYNW2E/foo.jpg.doc"
2016-01-01 20:36:41,206 [output]Expected (path): 
"/tmp/knewfilemenutest-oYNW2E/foo.jpg.txt"

I can reproduce this if I set XDG_DATA_DIRS to 
/usr/share:/d/kde/inst/kde_frameworks/share:/usr/share
instead of
/d/kde/inst/kde_frameworks/share:/usr/share
(as it normally is on my system).

And indeed the value of XDG_DATA_DIRS in
http://ci-logs.kde.flaska.net/7f/7f50569d59eabc20897f54fb97483bd7f8b89d63/rebuilddep/rebuilddep-kf5-qt55-clang-el7/3cef5be/shell_output.log
is long and messy and has /usr/share in front (so it's viewed as being "on top 
of" the kde mimetype file, while it's supposed to be the other way around).

Can this be fixed in the CI setup?

Thanks.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: closest equivalent to KApplicationPrivate::init() ?

[David Faure]

On Saturday 02 January 2016 15:59:04 René J.V. Bertin wrote:
> On Saturday January 02 2016 14:14:19 David Faure wrote:
> >Maybe, maybe not, but what we need to continue this discussion is hard data.
> >1) what does QProcess on Mac do (always in front, always in background, 
> >depends on whether the user is typing)
> 
> You're probably better aware (or able to understand) than I exactly what 
> QProcess does behind the scenes. I can affirm that it is basically common 
> knowledge that native GUI applications that are started via a call from the 
> exec() family or through system() will open in the layer behind the one 
> occupied by the parent process.
> I don't think the data will get any harder than that, unless we get our hands 
> on the sources of the relevant SDKs.

While I like the general approach of reading source code, what I meant here was 
*testing*.
If it behaves like you want it to behave, that's perfect, no need to understand 
every line of its code to assess that.

> >2) can QProcess be improved
> 
> It should be possible, as long as there are no hard feature requirements that 
> are incompatible with the use of LaunchServices. 
> 
> >3) can we use QProcess from KF5 instead of kdeinit (because, again, 
> >kdeinit's only purpose is fork+exec
> >which is apparently not a good solution on Mac). I'm pretty sure the answer 
> >to this one is yes.
> 
> Yes, but I'm not sure there is an advantage to using QProcess if you only use 
> it to "detach" a new application with which you're not keeping any ties. I 
> don't think QProcess allows much more than that, other than stopping and 
> killing the spawned application (but I should check again). 

Sounds right, for QProcess::startDetached()  (no stdin/stdout/stderr). But the 
question is, how does it behave in terms
of bringing the new app to the background or foreground.

> If that's all true, I don't see why we would wait for a hypothetical QProcess 
> improvement, instead of using a native API directly, at least for GUI 
> applications that live in app bundles.

I very much disagree with this approach. Qt is opensource, if there's something 
to be fixed in QProcess, make a patch,
then it won't be hypothetical. I don't see "starting a GUI application" as a 
KF5-specific need at all.

> >E.g. if you set bool useKToolInvocation to false in krun.cpp:724, you'll get 
> >into the code path that uses QProcess.
> >(which btw shows another reason to ensure QProcess works: kdeinit isn't even 
> >always used, even on Linux,
> >see the conditional for useKToolInvocation).
> 
> Yeah, klaunch does that too (or something similar), I noticed. OTOH, if we 
> streamline kinit_mac.mm so that its launch() function does something like 
> what I outlined above, why would we stick with useKToolInvocation=false. At 
> the least we could put the native code in a common code file that's included 
> by kdeinit and klaunch and family, if we want to avoid the overhead of 
> relaying a launch request via kdeinit.

You missed my point. Whatever you do to kdeinit, when KRun uses the 
"useKToolInvocation==false" code path,
then QProcess will be used anyway, so you have an interest in making that work 
correctly.

As the boolean condition says, this is the case when tempFiles==true (the app 
is responsible for deleting the file after use)
or when runService is called with a "fake" KService (created by code, not in 
ksycoca)
or when suggestedFileName is set (comes from HTTP content-disposition).

But QProcess is used in many other places too (e.g. dropjob when dropping a 
file onto an exe in a filemanager).

> In fact, someone else already asked if there's any point in maintaining 
> kdeinit on a platform where it's main function (caching shared libraries) is 
> moot. Is there?

For starting apps, no, there isn't, that's what I keep telling you

The other use of kdeinit, starting kioslaves in a way that they can be passed 
from a process to another,
still remains. Therefore my recommendation is: don't kill kdeinit, keep it for 
kioslaves, but you don't need
it to start apps. Hence my suggestion of useKToolInvocation=false in krun.cpp, 
and the few other places that
use ktoolinvocation (to talk to klauncher, to talk to kdeinit).

The alternative is indeed to implement app launching in klauncher itself, but 
you still need the other ways
to start apps (anywhere QProcess is used) to work correctly anyway. So why not 
use QProcess everywhere?

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 126507: Fix leaked file description and potential use-after-free in kdelibs4support

[David Faure]

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

Ship it!


Ship It!

- David Faure


On Jan. 2, 2016, 4:02 p.m., Michael Pyne wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126507/
> ---
> 
> (Updated Jan. 2, 2016, 4:02 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs4support
> 
> 
> Description
> ---
> 
> Fix a couple of Coverity issues:
> 
> 1. CID 1175508; file descriptors used in KLockFile could be leaked in
> error conditions. Even when KLockFile sets mustCloseFd, the dtor's impl
> also checks that the lock has been taken, which is only considered true
> if LockOK had been returned in our lock function. Instead close() the fd
> ourselves unless we make it to LockOK.
> 
> 2. CID 117; The standard mis-use of QCache. QCache::insert can, in
> theory, delete our object as soon as we insert it into cache, so we have
> to check for that. Even ::contains() and ::object() can be risky (the
> pointers returned by object() have no lifetime guarantee), but since
> this is GUI code I assume it's only used single-threaded and not
> re-entrant. Otherwise we'd need even more paranoia...
> 
> 
> Diffs
> -
> 
>   src/kdecore/klockfile_unix.cpp 67283a5 
>   src/kdeui/k4style.cpp a1a2ab1 
> 
> Diff: https://git.reviewboard.kde.org/r/126507/diff/
> 
> 
> Testing
> ---
> 
> Everything builds and appears to still work, though it's hard to test K4Style 
> as I'm not sure what uses it right at this point.
> 
> 
> Thanks,
> 
> Michael Pyne
> 
>

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

Re: Review Request 126028: Add support for desktopFileName to NETWinInfo

[David Faure]

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

Ship it!


Sorry, forgot to review this earlier.

The @since version needs to be updated (to 5.19 actually, since I just tagged 
5.18). Unless you shout quickly for a 5.18 respin.

- David Faure


On Nov. 11, 2015, 1:15 p.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126028/
> ---
> 
> (Updated Nov. 11, 2015, 1:15 p.m.)
> 
> 
> Review request for KDE Frameworks, kwin and David Faure.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> Implementation for the new hint _NET_WM_DESKTOP_FILE, see [1].
> Till it's fully standardized going with a KDE prefix, so it's
> _KDE_NET_WM_DESKTOP_FILE. Once it's specified the atom name can
> be exchanged.
> 
> [1] https://mail.gnome.org/archives/wm-spec-list/2015-November/msg0.html
> 
> 
> Diffs
> -
> 
>   autotests/netwininfotestclient.cpp 59a3980ff589fc3de9e479905c191bcbf1747644 
>   src/netwm_def.h 0938e2b28f6b0d425a16748b52a5b8e4704d8af6 
>   src/platforms/xcb/atoms_p.h b5a6e7efc98ff8033c0854041428a7d4b52ffc93 
>   src/platforms/xcb/netwm.h 220340bf0e96d2a186b72e118b601471529aeabf 
>   src/platforms/xcb/netwm.cpp 2335c297bb627065ca9b7e691290bfbdc64bccc3 
>   src/platforms/xcb/netwm_p.h e0645bbfd2c2061d9201fe34160c6201d89f4a89 
> 
> Diff: https://git.reviewboard.kde.org/r/126028/diff/
> 
> 
> Testing
> ---
> 
> tested with a modified kcmshell5 and a modified desktop file.
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

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

Re: closest equivalent to KApplicationPrivate::init() ?

[David Faure]

On Saturday 02 January 2016 19:00:48 René J.V. Bertin wrote:
> > I very much disagree with this approach. Qt is opensource, if there's 
> > something to be fixed in QProcess, make a patch,
> > then it won't be hypothetical. I don't see "starting a GUI application" as 
> > a KF5-specific need at all.
> 
> You saw I said "wait for QProcess to be improved", right? I don't expect a 
> patch for QProcess to be incorporated before 5.7.x ...

Patience is a virtue :-)

> Ok, so what about the requirements on QProcess? Anything beyond what's 
> possible with QProcess::startDetached and possibly launch confirmation? 
> Should QProcess figure out transparently (or rather, opaquely :)) whether to 
> use LaunchServices or not, or should the developer be given a way to override 
> it, or even switch to switch to LaunchServices rather than using standard 
> Posix APIs?

Those are implementation details.
IMHO the API shouldn't talk about any of that, but rather have a flag like 
"it's a GUI app that I'm starting", which would hint at using LaunchServices
on OSX is that's the only way to bring it to the front.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

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

Re: Review Request 126620: Fixed most of level 1 and level 2 warnings

[David Faure]

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

Ship it!


Yes, C++11 style connect is fine in KF5.

However QTimer::singleShot has to keep using the SLOT() syntax for now, to Qt 
5.3 compat (until Qt 5.6 is out, then we'll drop Qt 5.3).


src/browserextension.cpp (line 257)
<https://git.reviewboard.kde.org/r/126620/#comment61882>

This won't compile with Qt 5.3, please revert this particular line.


- David Faure


On Jan. 3, 2016, 9:09 p.m., Artur Puzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126620/
> ---
> 
> (Updated Jan. 3, 2016, 9:09 p.m.)
> 
> 
> Review request for KDE Frameworks and Aleix Pol Gonzalez.
> 
> 
> Repository: kparts
> 
> 
> Description
> ---
> 
> Fixed `warning: QString* being called 
> [-Wclazy-qstring-uneeded-heap-allocations]` by changing to `QStringLiteral`-s.
> In `plugin.cpp:96` I haven't fixed the warnings, becouse `QStringLiteral` 
> would give an error. I applied my own optimisation instead.
> Fixed `warning: Don't call QList::first() on temporary 
> [-Wclazy-detaching-temporary]` by changeing to `at(0)`
> Fixed `warning: Reserve candidate [-Wclazy-reserve-candidates]` by adding a 
> simple `reserve`
> Fixed `warning: Old Style Connect [-Wclazy-old-style-connect]` I had to 
> change all of them by hand, becouse of a bug in clazy that I will be 
> reporting. I wasn't able to change 4 Old Style Connects. (3 in 
> `kreadonlypart.cpp`, 1 in `readwritepart.cpp`)
> Fixed `warning: KParts::BrowserOpenOrSaveQuestion has dtor but not copy-ctor, 
> copy-assignment [-Wclazy-rule-of-three]`, by adding `Q_DISABLE_COPY`, becouse 
> `BrowserOpenOrSaveQuestionPrivate` isn't copyable
> Only warnings that I left are the `[-Wclazy-function-args-by-ref]` warnings, 
> becouse they would involve modifying headers.
> 
> 
> Diffs
> -
> 
>   src/browserextension.cpp 4b7252c 
>   src/browseropenorsavequestion.h 85dcfa0 
>   src/browseropenorsavequestion.cpp c1425c1 
>   src/browserrun.cpp 4756511 
>   src/browserrun_p.h 4742598 
>   src/mainwindow.cpp 9b0c3c5 
>   src/part.cpp 1657561 
>   src/partmanager.cpp 81bf73f 
>   src/plugin.cpp 97f7d28 
>   src/scriptableextension.cpp 27676bc 
> 
> Diff: https://git.reviewboard.kde.org/r/126620/diff/
> 
> 
> Testing
> ---
> 
> Automated tests pass.
> Imported note: On my computer I'm runing versions 5.17.0 and it wasn't 
> possible for me to upgrade easly, so I changed the CMakeFiles.txt to compile 
> it. (of course the changed isn't included in this RR)
> 
> 
> Thanks,
> 
> Artur Puzio
> 
>

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

Re: Review Request 126505: Do not show a warning color before the user even started typing

[David Faure]

> On Jan. 2, 2016, 11:33 a.m., David Faure wrote:
> > Ship It!
> 
> Elvis Angelaccio wrote:
> Uhmm, the new tests don't pass locally on my system. I'm fairly sure it's 
> because I have kwidgetsaddons 5.17 globally installed from archlinux repos 
> (indeed, the warning color in my local tests is set as soon as something is 
> typed as password, which is the old behavior). Could someone who has 
> kwidgetsaddons installed from git, verify that these new tests pass? (and if 
> they do, feel free to even commit on my behalf).

You're committing changes to kwidgetaddons, how can it be that you're not 
compiling and testing it?

You should be able to run the test without installing kwidgetaddons, just by 
setting LD_LIBRARY_PATH to point to the dir where the uninstalled lib is.

I thought the RPATH handling made this automatic, even... at least it works 
here:

$ objdump -p ./knewpasswordwidgettest | grep PATH
  RUNPATH  
/d/qt/5/kde/qtbase/lib:/d/kde/build/5/frameworks/kwidgetsaddons/src

$ unset LD_LIBRARY_PATH # because here it points to the install dir, but if you 
use distro packages I guess you didn't set it anyway
$ ldd ./knewpasswordwidgettest | grep -i widgetsaddons
libKF5WidgetsAddons.so.5 => 
/d/kde/build/5/frameworks/kwidgetsaddons/src/libKF5WidgetsAddons.so.5 
(0x7f3f73a92000)


- David


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


On Dec. 27, 2015, 2:40 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126505/
> ---
> 
> (Updated Dec. 27, 2015, 2:40 p.m.)
> 
> 
> Review request for KDE Frameworks, KDE Usability, Christoph Feck, and David 
> Faure.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> As discussed in RR 125619 and 126426, the password verification field (in a 
> KNewPasswordWidget) should not be marked as "wrong" before the user even 
> started typing the verification password.
> 
> The revised approach is the following:
> 
> * The user starts typing something as password, e.g. 1234
> * The user types something else as verification password
> * As soon as the verification is not anymore a prefix of the password (e.g. 
> verification = 122), the warning color is shown.
> * As soon as the verification is a prefix again (e.g. the user deletes the 
> second 2, i.e. verification = 12) the warning color is not shown anymore.
> 
> 
> Diffs
> -
> 
>   autotests/knewpasswordwidgettest.h 43845128adec01aced4353c9f7986b7977829a2a 
>   autotests/knewpasswordwidgettest.cpp 
> 297b88d5f18b9cd37f0d26d94e56f38870756f20 
>   src/knewpasswordwidget.cpp a1b59454a2c2d7c09ac32acec52d3fffa73f77fc 
> 
> Diff: https://git.reviewboard.kde.org/r/126505/diff/
> 
> 
> Testing
> ---
> 
> Autotests assert what described above. Gif pictures would explain the patch 
> better than 1000 words, but I suck at creating them :(
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

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

Re: Review Request 126505: Do not show a warning color before the user even started typing

[David Faure]

> On Jan. 2, 2016, 11:33 a.m., David Faure wrote:
> > Ship It!
> 
> Elvis Angelaccio wrote:
> Uhmm, the new tests don't pass locally on my system. I'm fairly sure it's 
> because I have kwidgetsaddons 5.17 globally installed from archlinux repos 
> (indeed, the warning color in my local tests is set as soon as something is 
> typed as password, which is the old behavior). Could someone who has 
> kwidgetsaddons installed from git, verify that these new tests pass? (and if 
> they do, feel free to even commit on my behalf).
> 
> David Faure wrote:
> You're committing changes to kwidgetaddons, how can it be that you're not 
> compiling and testing it?
> 
> You should be able to run the test without installing kwidgetaddons, just 
> by setting LD_LIBRARY_PATH to point to the dir where the uninstalled lib is.
> 
> I thought the RPATH handling made this automatic, even... at least it 
> works here:
> 
> $ objdump -p ./knewpasswordwidgettest | grep PATH
>   RUNPATH  
> /d/qt/5/kde/qtbase/lib:/d/kde/build/5/frameworks/kwidgetsaddons/src
> 
> $ unset LD_LIBRARY_PATH # because here it points to the install dir, but 
> if you use distro packages I guess you didn't set it anyway
> $ ldd ./knewpasswordwidgettest | grep -i widgetsaddons
> libKF5WidgetsAddons.so.5 => 
> /d/kde/build/5/frameworks/kwidgetsaddons/src/libKF5WidgetsAddons.so.5 
> (0x7f3f73a92000)
> 
> Elvis Angelaccio wrote:
> > $ unset LD_LIBRARY_PATH
> 
> This did the trick! It turns out I actually had LD_LIBRARY_PATH defined:
> 
> $ echo $LD_LIBRARY_PATH
> /home/elvis/GNUstep/Library/Libraries:/usr/lib
> 
> That's why the tests were using the installed library. Thanks David :)

Yep, there's no good reason to ever have /usr/lib in your LD_LIBRARY_PATH. It 
will be found anyway since it's the system fallback, but if you set it 
explicitly then RPATH can never work.


- David


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


On Dec. 27, 2015, 2:40 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126505/
> ---
> 
> (Updated Dec. 27, 2015, 2:40 p.m.)
> 
> 
> Review request for KDE Frameworks, KDE Usability, Christoph Feck, and David 
> Faure.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> As discussed in RR 125619 and 126426, the password verification field (in a 
> KNewPasswordWidget) should not be marked as "wrong" before the user even 
> started typing the verification password.
> 
> The revised approach is the following:
> 
> * The user starts typing something as password, e.g. 1234
> * The user types something else as verification password
> * As soon as the verification is not anymore a prefix of the password (e.g. 
> verification = 122), the warning color is shown.
> * As soon as the verification is a prefix again (e.g. the user deletes the 
> second 2, i.e. verification = 12) the warning color is not shown anymore.
> 
> 
> Diffs
> -
> 
>   autotests/knewpasswordwidgettest.h 43845128adec01aced4353c9f7986b7977829a2a 
>   autotests/knewpasswordwidgettest.cpp 
> 297b88d5f18b9cd37f0d26d94e56f38870756f20 
>   src/knewpasswordwidget.cpp a1b59454a2c2d7c09ac32acec52d3fffa73f77fc 
> 
> Diff: https://git.reviewboard.kde.org/r/126505/diff/
> 
> 
> Testing
> ---
> 
> Autotests assert what described above. Gif pictures would explain the patch 
> better than 1000 words, but I suck at creating them :(
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

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