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

[Kai Uwe Broulik]

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

(Updated Nov. 26, 2015, 8:15 nachm.)


Review request for KDE Frameworks and Plasma.


Changes
---

Adding Frameworks

Does anyone of you have an idea how to prevent it from registering the copies 
as applications (see last comments)?


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: Review Request 126161: OS X housekeeping

[René J . V . Bertin]

Alex Merry wrote:

> If I recall rightly, it's a speed thing. kdeinit pre-loads some
> libraries common across most KDE applications (eg: Qt5Core and Qt5Gui,

Ah, right. Dive in too deeply and you can forget about underlying reasons.
The real underlying mechanism is of course (IIUC) the fact that there's only 1 
kdeinit5 instance running, meaning that each dlopen is only done once.


Makes you wonder what happens when it gets to dlopen a new version of a shared 
library, or one that's compatible with a library that was already loaded. But 
that's a different topic, one that clearly cannot arise on OS X.

R.
___
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)


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 126161: OS X housekeeping

[René J . V . Bertin]

On Thursday November 26 2015 08:54:25 David Faure 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.

You've got a point, but I don't see how `ps -axxlw | fgrep ` can miss 
something unless it has an unexpected name. Which is possible (I don't think I 
search for kwrapper instances) but would still mean that `killall` won't work.

I should check the exact parameters given to the execve() call.

>> 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 ?

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.
I presume we'd have to dig deep into some very lowlevel OS X code to know 
what's going on.
BTW, I may be wrong, but from what I understand the crash is provoked 
intentionally, i.e. it's a sort of abort.

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

Maybe the above helps a wee bit?

>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?

Ah. Indeed, we have NOT tried the simple fork+dlopen approach, for some reason 
(your patch skips the `l.load()` step).
However, supposing that it's in fact the dlopen in the child process that's 
off-limits, relaying it to an exec'ed helper still seems to produce more 
comparable behaviour.

There's something I don't really understand though: the exact same question you 
asked above.
What's the difference between starting kwrite directly on the commandline (or 
through execve()), and dlopen'ing it? Why does the code go through that on 
Linux? Whatever that difference is, it's probably the reason why the approach 
fails on OS X.

Final thing: did you see the question about the socket file klauncher leaves 
behind? Is it possible to let it be removed when klauncher exits?

R.
___
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

[René J . V . Bertin]

On Thursday November 26 2015 08:54:25 David Faure wrote:

> Not at all, kdeinit on linux does fork+dlopen, no exec.

Actually, that is true only when launch() is called with a library (.so), not 
when it's called with an executable!

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

kdeinit freezes on Wayland in OOM protection

[Martin Graesslin]

Hi all,

we are facing a problem during the startup of Plasma on Wayland. If OOM 
protection is enabled for kdeinit and we already have a running X server, 
kdeinit freezes dead.

I'm sorry for having ignored the issue for too long and had just disabled OOM 
protection on my system, so I never hit it. Now I enabled it again to get the 
problem. On my system I have now two frozen kdeinit processes:

martin1960  1956  0 77832 26448   1 13:05 ?00:00:00 /opt/kf5/bin/
kdeinit5 --oom-pipe 4 --kded +kcminit_startup
martin1961  1960  0 77832  2816   3 13:05 ?00:00:00 /opt/kf5/bin/
kdeinit5 --oom-pipe 4 --kded +kcminit_startup

One has the following stacktrace:
#0  0x7f2bbd812446 in do_sigsuspend (set=0x7ffc9d6f7b80) at ../sysdeps/
unix/sysv/linux/sigsuspend.c:31
#1  __GI___sigsuspend (set=0x7ffc9d6f7b80) at ../sysdeps/unix/sysv/linux/
sigsuspend.c:41
#2  0x0040af7a in reset_oom_protect () at /home/martin/src/kf5/
frameworks/kinit/src/kdeinit/kinit.cpp:461
#3  0x0040b85c in launch (argc=2, _name=0x4133cd "klauncher", 
args=0x7ffc9d6f8210 "--fd=9", cwd=0x0, envc=0, envs=0x0, reset_env=false, 
tty=0x0, avoid_loops=false, startup_id_str=0x4133d7 "0")
at /home/martin/src/kf5/frameworks/kinit/src/kdeinit/kinit.cpp:573
#4  0x0040ce49 in start_klauncher () at /home/martin/src/kf5/
frameworks/kinit/src/kdeinit/kinit.cpp:1033
#5  0x0040f0f3 in main (argc=5, argv=0x7ffc9d6f83f8) at /home/martin/
src/kf5/frameworks/kinit/src/kdeinit/kinit.cpp:1792

It's frozen in this line of code:
sigsuspend();   // wait for the signal to come

The other one has the following stacktrace:
#0  0x7f2bbd8b65e0 in __read_nocancel () at ../sysdeps/unix/syscall-
template.S:81
#1  0x0040c33b in launch (argc=2, _name=0x4133cd "klauncher", 
args=0x7ffc9d6f8210 "--fd=9", cwd=0x0, envc=0, envs=0x0, reset_env=false, 
tty=0x0, avoid_loops=false, startup_id_str=0x4133d7 "0")
at /home/martin/src/kf5/frameworks/kinit/src/kdeinit/kinit.cpp:754
#2  0x0040ce49 in start_klauncher () at /home/martin/src/kf5/
frameworks/kinit/src/kdeinit/kinit.cpp:1033
#3  0x0040f0f3 in main (argc=5, argv=0x7ffc9d6f83f8) at /home/martin/
src/kf5/frameworks/kinit/src/kdeinit/kinit.cpp:1792

which is:
d.n = read(d.fd[0], , 1);

Given that it looks to me like these two processes dead-lock. I do not 
understand why, why it only happens on Wayland, why the fact that an X server 
must already be running is relevant and what the OOM protection has to do with 
it.

Any advise is welcome.

Cheers
Martin

signature.asc
Description: This is a digitally signed message part.
___
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

[René J . V . Bertin]

On Thursday November 26 2015 08:54:25 David Faure wrote:

> 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?

Indeed, it's fork+dlopen that is the problem:

`kwrapper5 /path/to/kwrite` succeeds without my helper, whereas
`kwrapper5 /path/to/libkdeinit4_kwrite.dylib` (sic!) crashes during `l.load`.

Evidently there is no choice but to use my helper if kdeinit5 and/or kwrapper5 
are supposed to be able to launch() shared libraries containing kdeinitmain or 
kdemain.
The question is, are they, or is that guaranteed *never* to happen?

Note that I could not reproduce the absence of the standalone kwrite process in 
`ps` output, with your fix. It does however show up without name in the 
Activity Monitor, which is kind of an annoyance. Using [[NSProcessInfo 
processInfo] setProcessName:@"foo"] might be resolve that (requiring an 
additional ObjC++ module, OR a dedicated kinit_mac.mm). I'd have to check 
though whether this is not a KDE4/Qt4 issue because I've been annoyed at the 
fact that those apps tend to show up as mystery entries in certain listings.

R
___
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

[René J . V . Bertin]

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

(Updated Nov. 26, 2015, 5:20 p.m.)


Review request for KDE Software on Mac OS X and KDE Frameworks.


Changes
---

This revision moves introduces `kinit_mac.mm` in order to cut down on the 
number of #ifdefs, and merges in David's suggestion:

After looking more closely at the underlying issue, it turns out that the 
off-limits behaviour that results in a crash (possibly an abort) is the fact of 
calling non-POSIX APIs (which are non-reentrant) after a `fork()`. In `kdeinit` 
this happens during the `dlopen` call (`QLibrary::load`), when KDE and/or Qt 
ctors are called. Calling `dlopen` becomes safe again after starting a new 
process through `exec`.
Thus, my new proposition does:
- fork + exec when `launch()` is told to start executable. This is in fact the 
same code-path also used on other Unix variants
- fork + exec(helper) when `launch()` is told to start a shared library 
containing `kdeinitmain` or `kdemain`; the helper then calls `dlopen` and 
finally the resolved `kde*main` function.

The code is now prepared to be slimmed down if the helper tool turns out to be 
redundant (`kdeinit` or `klauncher` are never used to "launch" a shared 
library). I'd really prefer to let this become apparent in time, rather than 
having to figure out at an unknown time in the future why some features fail to 
work (or rather, slap a big fat warning into the user's face).


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 (updated)
-

  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, 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 126161: OS X housekeeping

[René J . V . Bertin]

> On Nov. 25, 2015, 8:39 a.m., David Faure wrote:
> > src/kdeinit/kinit.cpp, line 1621
> > 
> >
> > 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.
> 
> David Faure wrote:
> 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.
> 
> René J.V. Bertin wrote:
> Erm we have a communications problem here.
> 
> 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.
> 
> 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? 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).
> 
> Looking at this more closely I do think that my fix could borrow from 
> yours, and skip the whole `QLibrary l(libpath)` bit (including creating `l`) 
> because that is being done for nothing. For the rest, using a helper does 
> seem to give a better "emulation" of what `kdeinit5` does on Linux, no?
> 
> René J.V. Bertin wrote:
> With "my" fix:
> 
> ```
> 17906 1 400c   0  33  0  2527592   6744 -  Ss 
>  0 ?? 0:00.02 /opt/local/bin/kdeinit5 --suicide --nofork
> 17907 17906 4004   0  33  0  2641356  14844 -  S  
>  0 ?? 0:00.10 

Re: Review Request 126161: OS X housekeeping

[Alex Merry]

On 2015-11-26 09:27, René J.V. Bertin wrote:

There's something I don't really understand though: the exact same
question you asked above.
What's the difference between starting kwrite directly on the
commandline (or through execve()), and dlopen'ing it? Why does the
code go through that on Linux? Whatever that difference is, it's
probably the reason why the approach fails on OS X.


If I recall rightly, it's a speed thing. kdeinit pre-loads some 
libraries common across most KDE applications (eg: Qt5Core and Qt5Gui, 
which are reasonably large). fork()+dlopen() can then make direct use of 
these already-loaded libraries, whereas an exec() would have to 
reload/remap them all.


I think there's also possibly some memory-usage benefits from the fact 
that fork() gives you copy-on-write access to kdeinit's memory pages.


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