from:"René J . V . Bertin"

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

(Updated Sept. 22, 2016, 9:25 p.m.)

Review request for KDE Base Apps and KDE Software on Mac OS X.

Changes
---

Hope I didn't introduce any more "wrong indentation" issues resolving the ones
in ReniceDlg.cpp, or introduced "separate ticket" issues because the file isn't
exactly consistent in its indentation...

Repository: libksysguard

Description
---

This patch implements initial and rather minimal support for OS X, for now
focussing purely on process information. That feature is justified as it is
used by KDevelop in order to obtain the list of processes one can attach a
debugger to.

Mac OS X is tricky because it requires special privileges in order to obtain
certain types of information for any running process. For example, even
obtaining the number of threads spawned by a foreign process requires
privileges that aren't trivial to set up. I've prepared the terrain, but also
implemented a fallback strategy that calls `ps` to be sure that crucial
information like the command name is available for all processes.

Diffs (updated)
-

processcore/CMakeLists.txt e7c9263
processcore/Info.plist PRE-CREATION
processcore/processes_darwin_p.cpp PRE-CREATION
processcore/processes_local_p.cpp 2bc123f
processui/ReniceDlg.cpp a97563f
processui/ksysguardprocesslist.cpp 44603de

Diff: https://git.reviewboard.kde.org/r/128909/diff/

Testing
---

On OS X 10.9.5 with Frameworks 5.24.0 and Qt 5.6.1

File Attachments

Attach to Process widget in KDevelop

https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png

Thanks,

René J.V. Bertin

Re: Review Request 128909: initial, minimal support for OS X



> On Sept. 22, 2016, 6:05 p.m., Aleix Pol Gonzalez wrote:
> > processui/ReniceDlg.cpp, line 79
> > 
> >
> > Wrong indentation.
> > Also isn't it possible to query the backend or something?

Wrong indentation indeed, the file uses tabs for some reason.

Which backend do you want to query, for what? The "process backend" has no 
provision for indicating whether or not the host support changing anything, 
except for the IO priority. And I don't see how you could query the KAuth 
backend to know if these features should be disabled.


- René J.V.


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


On Sept. 22, 2016, 4:50 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128909/
> ---
> 
> (Updated Sept. 22, 2016, 4:50 p.m.)
> 
> 
> Review request for KDE Base Apps and KDE Software on Mac OS X.
> 
> 
> Repository: libksysguard
> 
> 
> Description
> ---
> 
> This patch implements initial and rather minimal support for OS X, for now 
> focussing purely on process information. That feature is justified as it is 
> used by KDevelop in order to obtain the list of processes one can attach a 
> debugger to.
> 
> Mac OS X is tricky because it requires special privileges in order to obtain 
> certain types of information for any running process. For example, even 
> obtaining the number of threads spawned by a foreign process requires 
> privileges that aren't trivial to set up. I've prepared the terrain, but also 
> implemented a fallback strategy that calls `ps` to be sure that crucial 
> information like the command name is available for all processes.
> 
> 
> Diffs
> -
> 
>   processcore/CMakeLists.txt e7c9263 
>   processcore/Info.plist PRE-CREATION 
>   processcore/processes_darwin_p.cpp PRE-CREATION 
>   processcore/processes_local_p.cpp 2bc123f 
>   processui/ReniceDlg.cpp a97563f 
>   processui/ksysguardprocesslist.cpp 44603de 
> 
> Diff: https://git.reviewboard.kde.org/r/128909/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.9.5 with Frameworks 5.24.0 and Qt 5.6.1
> 
> 
> File Attachments
> 
> 
> Attach to Process widget in KDevelop
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

Re: Review Request 128909: initial, minimal support for OS X

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

(Updated Sept. 22, 2016, 4:50 p.m.)

Review request for KDE Base Apps and KDE Software on Mac OS X.

Changes
---

Cleanup

Repository: libksysguard

Description
---

Diffs (updated)
-

Diff: https://git.reviewboard.kde.org/r/128909/diff/

Testing
---

On OS X 10.9.5 with Frameworks 5.24.0 and Qt 5.6.1

File Attachments

Attach to Process widget in KDevelop

https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png

Thanks,

René J.V. Bertin

Re: Review Request 128909: initial, minimal support for OS X



> On Sept. 17, 2016, 9:52 p.m., René J.V. Bertin wrote:
> > processui/ReniceDlg.cpp, lines 78-87
> > 
> >
> > I've actually managed to get ksysguardprocesslist_helper to work with a 
> > patched libdbus and a hack to export the session bus socket name to the 
> > system dbus.
> 
> Aleix Pol Gonzalez wrote:
> Is it really a good idea to rely on a patched libdbus?
> 
> René J.V. Bertin wrote:
> I've been discussing this on the DBus ML, and am slowly getting my head 
> about the hairy details and confusing terminology. I now know that the KAuth 
> helper (DBus service executable, ksysguardprocesslist_helper in this case) 
> has to connect on the system dbus. That removes the need for a hack, 
> basically all I have to do is set `DBUS_LAUNCHD_SESSION_BUS_SOCKET` to the 
> system dbus socket in the root user's environment.
> 
> It also means the libdbus patch can actually be discussed for 
> upstreaming; the reason this particular feature doesn't work on OS X seems to 
> be that helpers (services) like ksysguardprocesslist_helper don't even 
> attempt to connect on the system bus on OS X. They just bail out when they 
> fail to read the session bus socket name.
> 
> If we really wanted to tackle this problem correctly we'd write a 
> specific (native) helper backend for KAuth, probably based on Apple's Service 
> Management framework (which leverages launchd). I doubt that's worth the 
> trouble at this point.

I now have a much better patch for [lib]dbus which I hope to submit in a near 
future, after more extensive testing. In the meantime I'm leaving the 
libksysguard features disabled that require elevated privileges.


- René J.V.


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


On Sept. 17, 2016, 7:32 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128909/
> ---
> 
> (Updated Sept. 17, 2016, 7:32 p.m.)
> 
> 
> Review request for KDE Base Apps and KDE Software on Mac OS X.
> 
> 
> Repository: libksysguard
> 
> 
> Description
> ---
> 
> This patch implements initial and rather minimal support for OS X, for now 
> focussing purely on process information. That feature is justified as it is 
> used by KDevelop in order to obtain the list of processes one can attach a 
> debugger to.
> 
> Mac OS X is tricky because it requires special privileges in order to obtain 
> certain types of information for any running process. For example, even 
> obtaining the number of threads spawned by a foreign process requires 
> privileges that aren't trivial to set up. I've prepared the terrain, but also 
> implemented a fallback strategy that calls `ps` to be sure that crucial 
> information like the command name is available for all processes.
> 
> 
> Diffs
> -
> 
>   processcore/CMakeLists.txt e7c9263 
>   processcore/Info.plist PRE-CREATION 
>   processcore/helper.cpp d54c8e1 
>   processcore/processes_darwin_p.cpp PRE-CREATION 
>   processcore/processes_local_p.cpp 2bc123f 
>   processui/ReniceDlg.cpp a97563f 
>   processui/ksysguardprocesslist.cpp 44603de 
> 
> Diff: https://git.reviewboard.kde.org/r/128909/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.9.5 with Frameworks 5.24.0 and Qt 5.6.1
> 
> 
> File Attachments
> 
> 
> Attach to Process widget in KDevelop
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

Re: Review Request 128909: initial, minimal support for OS X

2016-09-18 Thread René J . V . Bertin

> On Sept. 17, 2016, 9:52 p.m., René J.V. Bertin wrote:
> > processui/ReniceDlg.cpp, lines 78-87
> > 
> >
> > I've actually managed to get ksysguardprocesslist_helper to work with a 
> > patched libdbus and a hack to export the session bus socket name to the 
> > system dbus.
> 
> Aleix Pol Gonzalez wrote:
> Is it really a good idea to rely on a patched libdbus?

I've been discussing this on the DBus ML, and am slowly getting my head about 
the hairy details and confusing terminology. I now know that the KAuth helper 
(DBus service executable, ksysguardprocesslist_helper in this case) has to 
connect on the system dbus. That removes the need for a hack, basically all I 
have to do is set `DBUS_LAUNCHD_SESSION_BUS_SOCKET` to the system dbus socket 
in the root user's environment.

It also means the libdbus patch can actually be discussed for upstreaming; the 
reason this particular feature doesn't work on OS X seems to be that helpers 
(services) like ksysguardprocesslist_helper don't even attempt to connect on 
the system bus on OS X. They just bail out when they fail to read the session 
bus socket name.

If we really wanted to tackle this problem correctly we'd write a specific 
(native) helper backend for KAuth, probably based on Apple's Service Management 
framework (which leverages launchd). I doubt that's worth the trouble at this 
point.

- René J.V.

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

On Sept. 17, 2016, 7:32 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128909/
> ---
> 
> (Updated Sept. 17, 2016, 7:32 p.m.)
> 
> 
> Review request for KDE Base Apps and KDE Software on Mac OS X.
> 
> 
> Repository: libksysguard
> 
> 
> Description
> ---
> 
> This patch implements initial and rather minimal support for OS X, for now 
> focussing purely on process information. That feature is justified as it is 
> used by KDevelop in order to obtain the list of processes one can attach a 
> debugger to.
> 
> Mac OS X is tricky because it requires special privileges in order to obtain 
> certain types of information for any running process. For example, even 
> obtaining the number of threads spawned by a foreign process requires 
> privileges that aren't trivial to set up. I've prepared the terrain, but also 
> implemented a fallback strategy that calls `ps` to be sure that crucial 
> information like the command name is available for all processes.
> 
> 
> Diffs
> -
> 
>   processcore/CMakeLists.txt e7c9263 
>   processcore/Info.plist PRE-CREATION 
>   processcore/helper.cpp d54c8e1 
>   processcore/processes_darwin_p.cpp PRE-CREATION 
>   processcore/processes_local_p.cpp 2bc123f 
>   processui/ReniceDlg.cpp a97563f 
>   processui/ksysguardprocesslist.cpp 44603de 
> 
> Diff: https://git.reviewboard.kde.org/r/128909/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.9.5 with Frameworks 5.24.0 and Qt 5.6.1
> 
> 
> File Attachments
> 
> 
> Attach to Process widget in KDevelop
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

Re: Review Request 128909: initial, minimal support for OS X

2016-09-17 Thread René J . V . Bertin


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




processui/ReniceDlg.cpp (lines 78 - 87)


I've actually managed to get ksysguardprocesslist_helper to work with a 
patched libdbus and a hack to export the session bus socket name to the system 
dbus.


- René J.V. Bertin


On Sept. 17, 2016, 7:32 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128909/
> ---
> 
> (Updated Sept. 17, 2016, 7:32 p.m.)
> 
> 
> Review request for KDE Base Apps and KDE Software on Mac OS X.
> 
> 
> Repository: libksysguard
> 
> 
> Description
> ---
> 
> This patch implements initial and rather minimal support for OS X, for now 
> focussing purely on process information. That feature is justified as it is 
> used by KDevelop in order to obtain the list of processes one can attach a 
> debugger to.
> 
> Mac OS X is tricky because it requires special privileges in order to obtain 
> certain types of information for any running process. For example, even 
> obtaining the number of threads spawned by a foreign process requires 
> privileges that aren't trivial to set up. I've prepared the terrain, but also 
> implemented a fallback strategy that calls `ps` to be sure that crucial 
> information like the command name is available for all processes.
> 
> 
> Diffs
> -
> 
>   processcore/CMakeLists.txt e7c9263 
>   processcore/Info.plist PRE-CREATION 
>   processcore/helper.cpp d54c8e1 
>   processcore/processes_darwin_p.cpp PRE-CREATION 
>   processcore/processes_local_p.cpp 2bc123f 
>   processui/ReniceDlg.cpp a97563f 
>   processui/ksysguardprocesslist.cpp 44603de 
> 
> Diff: https://git.reviewboard.kde.org/r/128909/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.9.5 with Frameworks 5.24.0 and Qt 5.6.1
> 
> 
> File Attachments
> 
> 
> Attach to Process widget in KDevelop
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

Re: Review Request 128909: initial, minimal support for OS X

2016-09-17 Thread René J . V . Bertin

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

(Updated Sept. 17, 2016, 7:32 p.m.)

Review request for KDE Base Apps and KDE Software on Mac OS X.

Changes
---

Fail with a nice error when trying to send a signal to processes belonging to
another user.

Repository: libksysguard

Description
---

Diffs (updated)
-

processcore/CMakeLists.txt e7c9263
processcore/Info.plist PRE-CREATION
processcore/helper.cpp d54c8e1
processcore/processes_darwin_p.cpp PRE-CREATION
processcore/processes_local_p.cpp 2bc123f
processui/ReniceDlg.cpp a97563f
processui/ksysguardprocesslist.cpp 44603de

Diff: https://git.reviewboard.kde.org/r/128909/diff/

Testing
---

On OS X 10.9.5 with Frameworks 5.24.0 and Qt 5.6.1

File Attachments

Attach to Process widget in KDevelop

https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png

Thanks,

René J.V. Bertin

Re: Review Request 128909: initial, minimal support for OS X

2016-09-17 Thread René J . V . Bertin

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

(Updated Sept. 17, 2016, 4:38 p.m.)

Review request for KDE Base Apps and KDE Software on Mac OS X.

Changes
---

New version that disables making changes to the process parameters that cannot
currently be changed. I should still do the same for sending signals to other
users' processes.

Repository: libksysguard

Description
---

Diffs (updated)
-

Diff: https://git.reviewboard.kde.org/r/128909/diff/

Testing
---

On OS X 10.9.5 with Frameworks 5.24.0 and Qt 5.6.1

File Attachments

Attach to Process widget in KDevelop

https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png

Thanks,

René J.V. Bertin

Re: Review Request 128909: initial, minimal support for OS X



> On Sept. 15, 2016, 1:29 a.m., Aleix Pol Gonzalez wrote:
> > processcore/helper.cpp, line 133
> > 
> >
> > `KAUTH_HELPER_MAIN` doesn't work on OS X?
> 
> René J.V. Bertin wrote:
> Thanks for picking up on this, I'd forgotten to make a comment about it.
> 
> I think the macro does work, it's KAuth which currently doesn't seem to 
> work for me. It also doesn't in my Linux build, btw, but there at least it 
> gives some error output suggesting tries to use polkit and possibly even the 
> processlist helper from my KDE4 desktop. 
> 
> The explicit implementation on OS X wasn't because of the macro, it was 
> to allow running setuid root. Initial testing didn't show any benefit to 
> that, but it could well be that's because KAuth still fails somewhere and 
> then simply aborts the requested operation.
> 
> Suggestions very welcome, but if we can't get this to work I'll probably 
> want to disable changing process priorities.
> 
> René J.V. Bertin wrote:
> As to testing with setuid root: it looks it won't be trivial to hack 
> KAuth out of helper.cpp . Is there a way to take KAuth out of the loop one 
> way or another - for instance instruct it to do nothing when already running 
> with the required privileges, or to ignore failures?
> 
> I'm fully aware why Qt disables running setuid root, but I'd hope the 
> risk is minimal in a helper app like this that is designed to work as root 
> (and doesn't "linger").
> 
> Also, the helper is started via DBus, right? Doesn't DBus provide a 
> mechanism to launch a service with elevated privileges?
> 
> René J.V. Bertin wrote:
> FWIW, after I uninstalled the KDE4 helper app from my Linux system 
> (`apt-get remove ksysguard`) I started getting the same error as I see on OS 
> X when I try to increase a process's priority:
> 
> ```
> kf5.kauth: Tried to start an invalid action
> kf5.kauth: Tried to start an invalid action
> ```
> 
> After reinstalling the KDE4 ksysguard package all errors went away and 
> the feature works. Go figure ...
> (I don't have KDE4 ksysguard installed on OS X)
> 
> Aleix Pol Gonzalez wrote:
> I suggest dropping the workaround for now and investigate how to get 
> KAuth to work altogether then.
> 
> René J.V. Bertin wrote:
> Not really related, but yeah I guess you're right.
> 
> FWIW, on OS X I see this:
> 
> ```
> > KAuthHelperTest -vb -v2
> [...]
> INFO   : HelperTest::testExecuteJobSignals() 
> QVERIFY(job->data().isEmpty())
>Loc: 
> [/opt/local/var/macports/build/_Volumes_Debian_MP9_site-ports_kf5_KF5-Frameworks/kf5-kauth/work/kauth-5.24.0/autotests/HelperTest.cpp(184)]
> PASS   : HelperTest::testExecuteJobSignals()
> INFO   : HelperTest::testActionData() entering
> INFO   : HelperTest::testActionData() QVERIFY(action.isValid())
>Loc: 
> [/opt/local/var/macports/build/_Volumes_Debian_MP9_site-ports_kf5_KF5-Frameworks/kf5-kauth/work/kauth-5.24.0/autotests/HelperTest.cpp(199)]
> INFO   : HelperTest::testActionData() QCOMPARE(action.status(), 
> KAuth::Action::AuthRequiredStatus)
>Loc: 
> [/opt/local/var/macports/build/_Volumes_Debian_MP9_site-ports_kf5_KF5-Frameworks/kf5-kauth/work/kauth-5.24.0/autotests/HelperTest.cpp(201)]
> QWARN  : HelperTest::testActionData() QSocketNotifier: Socket notifiers 
> cannot be enabled or disabled from another thread
> ^C
> QFATAL : HelperTest::testActionData() Received signal 2
>  Function time: 5229ms Total time: 7337ms
> FAIL!  : HelperTest::testActionData() Received a fatal error.
>Loc: [Unknown file(0)]
> Totals: 3 passed, 1 failed, 0 skipped, 0 blacklisted
> * Finished testing of HelperTest *
> Exit 1
> ```
> 
> That's at least 1 thing that doesn't work as it should; not yet sure what 
> this has to do with the failures I'm seeing (the socket error is 
> platform-agnostic so the KAuth OS X code does cross-thread stuff the Linux 
> code apparently doesn't ... unless it's libdbus that uses threading)
> ).
> 
> René J.V. Bertin wrote:
> Well ... it turns out I had never "back"ported my modifications to KDE4's 
> Auth Services to KAuth, nor had anybody else in the 2 years since they were 
> committed.
> 
> https://git.reviewboard.kde.org/r/126086/
> 
> However, that doesn't fix everything. I now have a ksysguard app that 
> obtains authorisation to do whatever it is it wants to do, and that then 
> raises a DBus connection error because it is designed to use a helper for 
> that purpose, which apparently fails to connect (I think it isn't even 
> launched).
> 
> It seems ksysguard was never designed with platforms in mind where 
> authorisation isn't obtained through a helper that's controlled via DBus. 
> Technically that *should* work on OS X, but in

Re: Review Request 128909: initial, minimal support for OS X



> On Sept. 15, 2016, 1:29 a.m., Aleix Pol Gonzalez wrote:
> > processcore/helper.cpp, line 133
> > 
> >
> > `KAUTH_HELPER_MAIN` doesn't work on OS X?
> 
> René J.V. Bertin wrote:
> Thanks for picking up on this, I'd forgotten to make a comment about it.
> 
> I think the macro does work, it's KAuth which currently doesn't seem to 
> work for me. It also doesn't in my Linux build, btw, but there at least it 
> gives some error output suggesting tries to use polkit and possibly even the 
> processlist helper from my KDE4 desktop. 
> 
> The explicit implementation on OS X wasn't because of the macro, it was 
> to allow running setuid root. Initial testing didn't show any benefit to 
> that, but it could well be that's because KAuth still fails somewhere and 
> then simply aborts the requested operation.
> 
> Suggestions very welcome, but if we can't get this to work I'll probably 
> want to disable changing process priorities.
> 
> René J.V. Bertin wrote:
> As to testing with setuid root: it looks it won't be trivial to hack 
> KAuth out of helper.cpp . Is there a way to take KAuth out of the loop one 
> way or another - for instance instruct it to do nothing when already running 
> with the required privileges, or to ignore failures?
> 
> I'm fully aware why Qt disables running setuid root, but I'd hope the 
> risk is minimal in a helper app like this that is designed to work as root 
> (and doesn't "linger").
> 
> Also, the helper is started via DBus, right? Doesn't DBus provide a 
> mechanism to launch a service with elevated privileges?
> 
> René J.V. Bertin wrote:
> FWIW, after I uninstalled the KDE4 helper app from my Linux system 
> (`apt-get remove ksysguard`) I started getting the same error as I see on OS 
> X when I try to increase a process's priority:
> 
> ```
> kf5.kauth: Tried to start an invalid action
> kf5.kauth: Tried to start an invalid action
> ```
> 
> After reinstalling the KDE4 ksysguard package all errors went away and 
> the feature works. Go figure ...
> (I don't have KDE4 ksysguard installed on OS X)
> 
> Aleix Pol Gonzalez wrote:
> I suggest dropping the workaround for now and investigate how to get 
> KAuth to work altogether then.
> 
> René J.V. Bertin wrote:
> Not really related, but yeah I guess you're right.
> 
> FWIW, on OS X I see this:
> 
> ```
> > KAuthHelperTest -vb -v2
> [...]
> INFO   : HelperTest::testExecuteJobSignals() 
> QVERIFY(job->data().isEmpty())
>Loc: 
> [/opt/local/var/macports/build/_Volumes_Debian_MP9_site-ports_kf5_KF5-Frameworks/kf5-kauth/work/kauth-5.24.0/autotests/HelperTest.cpp(184)]
> PASS   : HelperTest::testExecuteJobSignals()
> INFO   : HelperTest::testActionData() entering
> INFO   : HelperTest::testActionData() QVERIFY(action.isValid())
>Loc: 
> [/opt/local/var/macports/build/_Volumes_Debian_MP9_site-ports_kf5_KF5-Frameworks/kf5-kauth/work/kauth-5.24.0/autotests/HelperTest.cpp(199)]
> INFO   : HelperTest::testActionData() QCOMPARE(action.status(), 
> KAuth::Action::AuthRequiredStatus)
>Loc: 
> [/opt/local/var/macports/build/_Volumes_Debian_MP9_site-ports_kf5_KF5-Frameworks/kf5-kauth/work/kauth-5.24.0/autotests/HelperTest.cpp(201)]
> QWARN  : HelperTest::testActionData() QSocketNotifier: Socket notifiers 
> cannot be enabled or disabled from another thread
> ^C
> QFATAL : HelperTest::testActionData() Received signal 2
>  Function time: 5229ms Total time: 7337ms
> FAIL!  : HelperTest::testActionData() Received a fatal error.
>Loc: [Unknown file(0)]
> Totals: 3 passed, 1 failed, 0 skipped, 0 blacklisted
> * Finished testing of HelperTest *
> Exit 1
> ```
> 
> That's at least 1 thing that doesn't work as it should; not yet sure what 
> this has to do with the failures I'm seeing (the socket error is 
> platform-agnostic so the KAuth OS X code does cross-thread stuff the Linux 
> code apparently doesn't ... unless it's libdbus that uses threading)
> ).

Well ... it turns out I had never "back"ported my modifications to KDE4's Auth 
Services to KAuth, nor had anybody else in the 2 years since they were 
committed.

https://git.reviewboard.kde.org/r/126086/

However, that doesn't fix everything. I now have a ksysguard app that obtains 
authorisation to do whatever it is it wants to do, and that then raises a DBus 
connection error because it is designed to use a helper for that purpose, which 
apparently fails to connect (I think it isn't even launched).

It seems ksysguard was never designed with platforms in mind where 
authorisation isn't obtained through a helper that's controlled via DBus. 
Technically that *should* work on OS X, but in practice KAuth would probably 
better do without.

What's the better approach here? Modify

Re: Review Request 128909: initial, minimal support for OS X

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

(Updated Sept. 15, 2016, 2:47 p.m.)

Review request for KDE Base Apps and KDE Software on Mac OS X.

Changes
---

drops the support for running setuid root from helper.cpp

Repository: libksysguard

Description
---

Diffs (updated)
-

processcore/CMakeLists.txt e7c9263
processcore/Info.plist PRE-CREATION
processcore/processes_darwin_p.cpp PRE-CREATION
processcore/processes_local_p.cpp 2bc123f

Diff: https://git.reviewboard.kde.org/r/128909/diff/

Testing
---

On OS X 10.9.5 with Frameworks 5.24.0 and Qt 5.6.1

File Attachments

Attach to Process widget in KDevelop

https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png

Thanks,

René J.V. Bertin

Re: Review Request 128909: initial, minimal support for OS X



> On Sept. 15, 2016, 1:29 a.m., Aleix Pol Gonzalez wrote:
> > processcore/helper.cpp, line 133
> > 
> >
> > `KAUTH_HELPER_MAIN` doesn't work on OS X?
> 
> René J.V. Bertin wrote:
> Thanks for picking up on this, I'd forgotten to make a comment about it.
> 
> I think the macro does work, it's KAuth which currently doesn't seem to 
> work for me. It also doesn't in my Linux build, btw, but there at least it 
> gives some error output suggesting tries to use polkit and possibly even the 
> processlist helper from my KDE4 desktop. 
> 
> The explicit implementation on OS X wasn't because of the macro, it was 
> to allow running setuid root. Initial testing didn't show any benefit to 
> that, but it could well be that's because KAuth still fails somewhere and 
> then simply aborts the requested operation.
> 
> Suggestions very welcome, but if we can't get this to work I'll probably 
> want to disable changing process priorities.
> 
> René J.V. Bertin wrote:
> As to testing with setuid root: it looks it won't be trivial to hack 
> KAuth out of helper.cpp . Is there a way to take KAuth out of the loop one 
> way or another - for instance instruct it to do nothing when already running 
> with the required privileges, or to ignore failures?
> 
> I'm fully aware why Qt disables running setuid root, but I'd hope the 
> risk is minimal in a helper app like this that is designed to work as root 
> (and doesn't "linger").
> 
> Also, the helper is started via DBus, right? Doesn't DBus provide a 
> mechanism to launch a service with elevated privileges?
> 
> René J.V. Bertin wrote:
> FWIW, after I uninstalled the KDE4 helper app from my Linux system 
> (`apt-get remove ksysguard`) I started getting the same error as I see on OS 
> X when I try to increase a process's priority:
> 
> ```
> kf5.kauth: Tried to start an invalid action
> kf5.kauth: Tried to start an invalid action
> ```
> 
> After reinstalling the KDE4 ksysguard package all errors went away and 
> the feature works. Go figure ...
> (I don't have KDE4 ksysguard installed on OS X)
> 
> Aleix Pol Gonzalez wrote:
> I suggest dropping the workaround for now and investigate how to get 
> KAuth to work altogether then.

Not really related, but yeah I guess you're right.

FWIW, on OS X I see this:

```
> KAuthHelperTest -vb -v2
[...]
INFO   : HelperTest::testExecuteJobSignals() QVERIFY(job->data().isEmpty())
   Loc: 
[/opt/local/var/macports/build/_Volumes_Debian_MP9_site-ports_kf5_KF5-Frameworks/kf5-kauth/work/kauth-5.24.0/autotests/HelperTest.cpp(184)]
PASS   : HelperTest::testExecuteJobSignals()
INFO   : HelperTest::testActionData() entering
INFO   : HelperTest::testActionData() QVERIFY(action.isValid())
   Loc: 
[/opt/local/var/macports/build/_Volumes_Debian_MP9_site-ports_kf5_KF5-Frameworks/kf5-kauth/work/kauth-5.24.0/autotests/HelperTest.cpp(199)]
INFO   : HelperTest::testActionData() QCOMPARE(action.status(), 
KAuth::Action::AuthRequiredStatus)
   Loc: 
[/opt/local/var/macports/build/_Volumes_Debian_MP9_site-ports_kf5_KF5-Frameworks/kf5-kauth/work/kauth-5.24.0/autotests/HelperTest.cpp(201)]
QWARN  : HelperTest::testActionData() QSocketNotifier: Socket notifiers cannot 
be enabled or disabled from another thread
^C
QFATAL : HelperTest::testActionData() Received signal 2
 Function time: 5229ms Total time: 7337ms
FAIL!  : HelperTest::testActionData() Received a fatal error.
   Loc: [Unknown file(0)]
Totals: 3 passed, 1 failed, 0 skipped, 0 blacklisted
* Finished testing of HelperTest *
Exit 1
```

That's at least 1 thing that doesn't work as it should; not yet sure what this 
has to do with the failures I'm seeing (the socket error is platform-agnostic 
so the KAuth OS X code does cross-thread stuff the Linux code apparently 
doesn't ... unless it's libdbus that uses threading)
).


- René J.V.


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


On Sept. 14, 2016, 9:37 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128909/
> ---
> 
> (Updated Sept. 14, 2016, 9:37 p.m.)
> 
> 
> Review request for KDE Base Apps and KDE Software on Mac OS X.
> 
> 
> Repository: libksysguard
> 
> 
> Description
> ---
> 
> This patch implements initial and rather minimal support for OS X, for now 
> focussing purely on process information. That feature is justified as it is 
> used by KDevelop in order to obtain the list of processes one can attach a 
> debugger to.
> 
> Mac OS X is tricky because it

Re: Review Request 128909: initial, minimal support for OS X



> On Sept. 15, 2016, 1:29 a.m., Aleix Pol Gonzalez wrote:
> > processcore/helper.cpp, line 133
> > 
> >
> > `KAUTH_HELPER_MAIN` doesn't work on OS X?
> 
> René J.V. Bertin wrote:
> Thanks for picking up on this, I'd forgotten to make a comment about it.
> 
> I think the macro does work, it's KAuth which currently doesn't seem to 
> work for me. It also doesn't in my Linux build, btw, but there at least it 
> gives some error output suggesting tries to use polkit and possibly even the 
> processlist helper from my KDE4 desktop. 
> 
> The explicit implementation on OS X wasn't because of the macro, it was 
> to allow running setuid root. Initial testing didn't show any benefit to 
> that, but it could well be that's because KAuth still fails somewhere and 
> then simply aborts the requested operation.
> 
> Suggestions very welcome, but if we can't get this to work I'll probably 
> want to disable changing process priorities.
> 
> René J.V. Bertin wrote:
> As to testing with setuid root: it looks it won't be trivial to hack 
> KAuth out of helper.cpp . Is there a way to take KAuth out of the loop one 
> way or another - for instance instruct it to do nothing when already running 
> with the required privileges, or to ignore failures?
> 
> I'm fully aware why Qt disables running setuid root, but I'd hope the 
> risk is minimal in a helper app like this that is designed to work as root 
> (and doesn't "linger").
> 
> Also, the helper is started via DBus, right? Doesn't DBus provide a 
> mechanism to launch a service with elevated privileges?

FWIW, after I uninstalled the KDE4 helper app from my Linux system (`apt-get 
remove ksysguard`) I started getting the same error as I see on OS X when I try 
to increase a process's priority:

```
kf5.kauth: Tried to start an invalid action
kf5.kauth: Tried to start an invalid action
```

After reinstalling the KDE4 ksysguard package all errors went away and the 
feature works. Go figure ...
(I don't have KDE4 ksysguard installed on OS X)


- René J.V.


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


On Sept. 14, 2016, 9:37 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128909/
> ---
> 
> (Updated Sept. 14, 2016, 9:37 p.m.)
> 
> 
> Review request for KDE Base Apps and KDE Software on Mac OS X.
> 
> 
> Repository: libksysguard
> 
> 
> Description
> ---
> 
> This patch implements initial and rather minimal support for OS X, for now 
> focussing purely on process information. That feature is justified as it is 
> used by KDevelop in order to obtain the list of processes one can attach a 
> debugger to.
> 
> Mac OS X is tricky because it requires special privileges in order to obtain 
> certain types of information for any running process. For example, even 
> obtaining the number of threads spawned by a foreign process requires 
> privileges that aren't trivial to set up. I've prepared the terrain, but also 
> implemented a fallback strategy that calls `ps` to be sure that crucial 
> information like the command name is available for all processes.
> 
> 
> Diffs
> -
> 
>   processcore/CMakeLists.txt e7c9263 
>   processcore/Info.plist PRE-CREATION 
>   processcore/helper.cpp d54c8e1 
>   processcore/processes_darwin_p.cpp PRE-CREATION 
>   processcore/processes_local_p.cpp 2bc123f 
> 
> Diff: https://git.reviewboard.kde.org/r/128909/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.9.5 with Frameworks 5.24.0 and Qt 5.6.1
> 
> 
> File Attachments
> 
> 
> Attach to Process widget in KDevelop
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

Re: Review Request 128909: initial, minimal support for OS X

> On Sept. 15, 2016, 1:29 a.m., Aleix Pol Gonzalez wrote:
> > Other than that, the patch LGTM.

Aleix, you coined the idea of implementing a simpler widget that provides just 
a list of PIDs with their command names and possible command lines, based on 
the `ps` command (and comparable to the one in Qt Creator). Do you still think 
it'd be a good idea to add such a feature to libksysguard?
(Here's as good a place to raise the idea as any)

> On Sept. 15, 2016, 1:29 a.m., Aleix Pol Gonzalez wrote:
> > processcore/helper.cpp, line 133
> > 
> >
> > `KAUTH_HELPER_MAIN` doesn't work on OS X?

Thanks for picking up on this, I'd forgotten to make a comment about it.

I think the macro does work, it's KAuth which currently doesn't seem to work 
for me. It also doesn't in my Linux build, btw, but there at least it gives 
some error output suggesting tries to use polkit and possibly even the 
processlist helper from my KDE4 desktop. 

The explicit implementation on OS X wasn't because of the macro, it was to 
allow running setuid root. Initial testing didn't show any benefit to that, but 
it could well be that's because KAuth still fails somewhere and then simply 
aborts the requested operation.

Suggestions very welcome, but if we can't get this to work I'll probably want 
to disable changing process priorities.

- René J.V.

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

On Sept. 14, 2016, 9:37 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128909/
> ---
> 
> (Updated Sept. 14, 2016, 9:37 p.m.)
> 
> 
> Review request for KDE Base Apps and KDE Software on Mac OS X.
> 
> 
> Repository: libksysguard
> 
> 
> Description
> ---
> 
> This patch implements initial and rather minimal support for OS X, for now 
> focussing purely on process information. That feature is justified as it is 
> used by KDevelop in order to obtain the list of processes one can attach a 
> debugger to.
> 
> Mac OS X is tricky because it requires special privileges in order to obtain 
> certain types of information for any running process. For example, even 
> obtaining the number of threads spawned by a foreign process requires 
> privileges that aren't trivial to set up. I've prepared the terrain, but also 
> implemented a fallback strategy that calls `ps` to be sure that crucial 
> information like the command name is available for all processes.
> 
> 
> Diffs
> -
> 
>   processcore/CMakeLists.txt e7c9263 
>   processcore/Info.plist PRE-CREATION 
>   processcore/helper.cpp d54c8e1 
>   processcore/processes_darwin_p.cpp PRE-CREATION 
>   processcore/processes_local_p.cpp 2bc123f 
> 
> Diff: https://git.reviewboard.kde.org/r/128909/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.9.5 with Frameworks 5.24.0 and Qt 5.6.1
> 
> 
> File Attachments
> 
> 
> Attach to Process widget in KDevelop
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

Re: Review Request 128909: initial, minimal support for OS X

2016-09-14 Thread René J . V . Bertin

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

(Updated Sept. 14, 2016, 9:37 p.m.)

Review request for KDE Base Apps and KDE Software on Mac OS X.

Changes
---

inline issue hopefully fixed

Repository: libksysguard

Description
---

Diffs (updated)
-

processcore/CMakeLists.txt e7c9263
processcore/Info.plist PRE-CREATION
processcore/helper.cpp d54c8e1
processcore/processes_darwin_p.cpp PRE-CREATION
processcore/processes_local_p.cpp 2bc123f

Diff: https://git.reviewboard.kde.org/r/128909/diff/

Testing
---

On OS X 10.9.5 with Frameworks 5.24.0 and Qt 5.6.1

File Attachments

Attach to Process widget in KDevelop

https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png

Thanks,

René J.V. Bertin

Re: Review Request 128909: initial, minimal support for OS X

2016-09-14 Thread René J . V . Bertin



> On Sept. 14, 2016, 7:33 p.m., Anthony Fieroni wrote:
> > processcore/processes_darwin_p.cpp, line 104
> > 
> >
> > *inline* must be used in function definition not in declaration.

I didn't see the point of the way the keyword is being used here either, but I 
aligned myself to the current practice.

So I take it I shouldn't only correct `argMax()`?


- René J.V.


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


On Sept. 14, 2016, 5:28 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128909/
> ---
> 
> (Updated Sept. 14, 2016, 5:28 p.m.)
> 
> 
> Review request for KDE Base Apps and KDE Software on Mac OS X.
> 
> 
> Repository: libksysguard
> 
> 
> Description
> ---
> 
> This patch implements initial and rather minimal support for OS X, for now 
> focussing purely on process information. That feature is justified as it is 
> used by KDevelop in order to obtain the list of processes one can attach a 
> debugger to.
> 
> Mac OS X is tricky because it requires special privileges in order to obtain 
> certain types of information for any running process. For example, even 
> obtaining the number of threads spawned by a foreign process requires 
> privileges that aren't trivial to set up. I've prepared the terrain, but also 
> implemented a fallback strategy that calls `ps` to be sure that crucial 
> information like the command name is available for all processes.
> 
> 
> Diffs
> -
> 
>   processcore/CMakeLists.txt e7c9263 
>   processcore/Info.plist PRE-CREATION 
>   processcore/helper.cpp d54c8e1 
>   processcore/processes_darwin_p.cpp PRE-CREATION 
>   processcore/processes_local_p.cpp 2bc123f 
> 
> Diff: https://git.reviewboard.kde.org/r/128909/diff/
> 
> 
> Testing
> ---
> 
> On OS X 10.9.5 with Frameworks 5.24.0 and Qt 5.6.1
> 
> 
> File Attachments
> 
> 
> Attach to Process widget in KDevelop
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/09/14/47468811-58cb-40b8-a735-4dd86dce98e1__Screen_Shot_2016-09-14_at_17.38.22.png
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

Review Request 128909: initial, minimal support for OS X

2016-09-14 Thread René J . V . Bertin


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

Review request for KDE Base Apps and KDE Software on Mac OS X.


Repository: libksysguard


Description
---

This patch implements initial and rather minimal support for OS X, for now 
focussing purely on process information. That feature is justified as it is 
used by KDevelop in order to obtain the list of processes one can attach a 
debugger to.

Mac OS X is tricky because it requires special privileges in order to obtain 
certain types of information for any running process. For example, even 
obtaining the number of threads spawned by a foreign process requires 
privileges that aren't trivial to set up. I've prepared the terrain, but also 
implemented a fallback strategy that calls `ps` to be sure that crucial 
information like the command name is available for all processes.


Diffs
-

  processcore/CMakeLists.txt e7c9263 
  processcore/Info.plist PRE-CREATION 
  processcore/helper.cpp d54c8e1 
  processcore/processes_darwin_p.cpp PRE-CREATION 
  processcore/processes_local_p.cpp 2bc123f 

Diff: https://git.reviewboard.kde.org/r/128909/diff/


Testing
---

On OS X 10.9.5 with Frameworks 5.24.0 and Qt 5.6.1


Thanks,

René J.V. Bertin

Re: Review Request 120319: Make Kate mousewheel zoom feature respect the corresponding global setting (KGlobalSettings::wheelMouseZooms())

2016-08-28 Thread René J . V . Bertin

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

(Updated Aug. 28, 2016, 12:35 p.m.)

Status
--

This change has been discarded.

Review request for Kate, KDE Software on Mac OS X and kdelibs.

Repository: kate

Description
---

KDE has a global text editor option that can be used to let Ctrl-MouseWheel
events zoom the text font being used. Kate does not respect this setting, which
is an omission that can lead to unexpected behaviour.

On OS X, the feature works slightly differently in the sense that
`Qt::ControlModifier` does not designate the control key, but the command (?,
Apple) key. In addition, Qt's event handling does not appear to be able to
distinguish between scrolling under direct control, and residual scroll
movement that's due to simulated inertia. As a result, any attempt to use a
keyboard shortcut while a text view has not stopped moving completely will lead
to text zooming.
See https://bugreports.qt-project.org/browse/QTBUG-41475 .

At first I thought to replace `Qt::ControlModifier` with `Qt::MetaModifier` on
OS X but that would probably require changes in many locations, and thus best
be preceded by a design decision if the standard shortcut modifier key ought
not be referenced via a symbolic platform constant not named after a specific
key, instead of being hardcoded (and using a key name).

Therefore, I present a small patch that checks
`KGlobalSettings::wheelMouseZooms()` when the platform's `ControlModifier` is
held and a wheel event received.

An alternative solution could introduce a Kate-specific setting (just like
Konsole has one), but that would require far more changes.

Diffs
-

part/view/kateviewinternal.cpp a2906f3

Diff: https://git.reviewboard.kde.org/r/120319/diff/

Testing
---

On OS X against kdelibs 4.14.1 (git/kde4). The change consists of an additional
call to a standard kdelibs function which I do not expect to introduce
regressions on any platform.

I looked at kate's git/master code, which lacks a `wheelEvent` handler
suggesting the feature works differently there. However, Qt 5.3.1 applications
(like Digia's own Qt Creator) still suffer from the phenomenon described above,
so a fix would be beneficial for KF5 too)

File Attachments

Qt4 patch to prevent unwanted/unexpected text zoom while scrolling

https://git.reviewboard.kde.org/media/uploaded/files/2015/09/03/ae8f63f3-4289-40bb-8b97-61a934bb33ec__patch-avoid-unwanted-fontzooming.diff

Thanks,

René J.V. Bertin

Re: Review Request 126304: (re)enable building with -DQTC_QT5_ENABLE_KDE

2016-01-04 Thread René J . V . Bertin


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

(Updated Jan. 4, 2016, 4:04 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, Qt KDE and Yichao Yu.


Changes
---

Submitted with commit 38d973cced1b2ca8a8c799ebb1bbb9408f191114 by René J.V. 
Bertin to branch master.


Repository: qtcurve


Description
---

As the title says, this restores the definition of QTC_QT5_ENABLE_KDE in the 
CMake file when a KF5 build is done, and introduces the changes required for 
that build to succeed.

Except for removing references to the m_componentData member (already removed 
from the class; deprecated type) I have not made any other changes to the code.


Diffs
-

  qt5/CMakeLists.txt 837d9c2 
  qt5/style/CMakeLists.txt 7f65f8c 
  qt5/style/qtcurve.cpp febcfcf 
  qt5/style/qtcurve_api.cpp 87a927f 
  qt5/style/qtcurve_p.h bfc7502 
  qt5/style/qtcurve_primitive.cpp b5a3204 

Diff: https://git.reviewboard.kde.org/r/126304/diff/


Testing
---

On KUbuntu 14.04 and Mac OS X 10.9.5, both with Qt 5.5.1 and KF5 Frameworks 
5.16.0 installed under /opt/local


Thanks,

René J.V. Bertin

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

2015-12-23 Thread René J . V . Bertin



> On Dec. 19, 2015, 11:23 a.m., David Faure wrote:
> > src/kdeui/kpushbutton.cpp, line 256
> > 
> >
> > 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?
> 
> René J.V. Bertin wrote:
> David, I'm dropping this whole RR, leaving it open only in case Thomas 
> wants to pursue his view on fixing what there is to fix.
> 
> You are right though, it's a workaround for KPushButton which doesn't 
> depend on fixing QPushButton. And whether or not QPushButton requires fixing 
> is apparently the big question. What is your idea on the scope of 
> `ShowIconsOnButtons`?
> 
> Thomas Lübking wrote:
> The "problem" is that this is really scattered around everywhere :(
> 
> One major catch should be (frameworksintegration)
> 
> ```
> diff --git a/src/platformtheme/kdeplatformfiledialoghelper.cpp 
> b/src/platformtheme/kdeplatformfiledialoghelper.cpp
> index 11e7efb..9cd374c 100644
> --- a/src/platformtheme/kdeplatformfiledialoghelper.cpp
> +++ b/src/platformtheme/kdeplatformfiledialoghelper.cpp
> @@ -161,6 +161,11 @@ void 
> KDEPlatformFileDialog::setFileMode(QFileDialogOptions::FileMode mode)
>  m_fileWidget->setMode(KFile::File);
>  break;
>  }
> +// ::setOperationMode happily adds icons to our buttonbox, so we 
> clear them everytime the mode is set
> +if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, 
> nullptr, this)) {
> +foreach (QAbstractButton *button, m_buttons->buttons())
> +button->setIcon(QIcon());
> +}
>  }
>  
>  void 
> KDEPlatformFileDialog::setCustomLabel(QFileDialogOptions::DialogLabel label, 
> const QString )
> diff --git a/src/platformtheme/kdirselectdialog.cpp 
> b/src/platformtheme/kdirselectdialog.cpp
> index 0a65cd3..13d7dc7 100644
> --- a/src/platformtheme/kdirselectdialog.cpp
> +++ b/src/platformtheme/kdirselectdialog.cpp
> @@ -299,6 +299,10 @@ KDirSelectDialog::KDirSelectDialog(const QUrl 
> , bool localOnly, QWidget
>  m_buttons->setStandardButtons(QDialogButtonBox::Ok | 
> QDialogButtonBox::Cancel);
>  connect(m_buttons, SIGNAL(accepted()), this, SLOT(accept()));
>  connect(m_buttons, SIGNAL(rejected()), this, SLOT(reject()));
> +if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, 
> nullptr, this)) {
> +foreach (QAbstractButton *button, m_buttons->buttons())
> +button->setIcon(QIcon());
> +}
>  topLayout->addWidget(m_buttons);
>  
>  QHBoxLayout *hlay = new QHBoxLayout(page);
> ```
> 
> But that somehow doesn't scale.
> 
> KGuiItem::apply would have to catch that, but doesn't.
> QDialogButtonBox::addButton *could* catch things, but doesn't (also it's 
> not clear whether or when m_buttons->button(SomeRole)->setIcon() is invoked, 
> so QPushButton::setIcon() would have to catch it and QPushButton would have 
> to catch it on reparenting.
> 
> 
> => For a complete solution, QPushButton actually needs painting code to 
> check the parent & style hint when setting up the style option - or we simply 
> rely on the style testing (widget && 
> qobject_cast(widget->parentWidget())) when calculating 
> sizes and painting the button.
> The only alternative is to walk a long way and tell everyone to please 
> check the hint and fix their buttonboxes eewww.
> 
> 
> 
> ==> QPushButton will require a "fix" to handle the StyleHint, but we 
> cannot expect *Q*PushButton to honor some global KDE setting, that's really a 
> job for the platform integration and in this case ultimately the style.
> 
> 
> 
> 
> => the platform integration plugin (KDE part in all Qt applications) 
> needs to read that setting and expose it to the styles.
> Alternatively the styles could read the setting from kdeglobals directly, 
> but that requires them to link kconfig (to do it correctly, just grabbing it 
> from the users kdeglobals ini isn't that much of a problem ;-), ie. rules out 
> Qt-only styles (and somehow contrasts w/ the KF5 approach to

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

2015-12-21 Thread René J . V . Bertin



> On Dec. 19, 2015, 11:23 a.m., David Faure wrote:
> > src/kdeui/kpushbutton.cpp, line 256
> > 
> >
> > 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?
> 
> René J.V. Bertin wrote:
> David, I'm dropping this whole RR, leaving it open only in case Thomas 
> wants to pursue his view on fixing what there is to fix.
> 
> You are right though, it's a workaround for KPushButton which doesn't 
> depend on fixing QPushButton. And whether or not QPushButton requires fixing 
> is apparently the big question. What is your idea on the scope of 
> `ShowIconsOnButtons`?
> 
> Thomas Lübking wrote:
> The "problem" is that this is really scattered around everywhere :(
> 
> One major catch should be (frameworksintegration)
> 
> ```
> diff --git a/src/platformtheme/kdeplatformfiledialoghelper.cpp 
> b/src/platformtheme/kdeplatformfiledialoghelper.cpp
> index 11e7efb..9cd374c 100644
> --- a/src/platformtheme/kdeplatformfiledialoghelper.cpp
> +++ b/src/platformtheme/kdeplatformfiledialoghelper.cpp
> @@ -161,6 +161,11 @@ void 
> KDEPlatformFileDialog::setFileMode(QFileDialogOptions::FileMode mode)
>  m_fileWidget->setMode(KFile::File);
>  break;
>  }
> +// ::setOperationMode happily adds icons to our buttonbox, so we 
> clear them everytime the mode is set
> +if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, 
> nullptr, this)) {
> +foreach (QAbstractButton *button, m_buttons->buttons())
> +button->setIcon(QIcon());
> +}
>  }
>  
>  void 
> KDEPlatformFileDialog::setCustomLabel(QFileDialogOptions::DialogLabel label, 
> const QString )
> diff --git a/src/platformtheme/kdirselectdialog.cpp 
> b/src/platformtheme/kdirselectdialog.cpp
> index 0a65cd3..13d7dc7 100644
> --- a/src/platformtheme/kdirselectdialog.cpp
> +++ b/src/platformtheme/kdirselectdialog.cpp
> @@ -299,6 +299,10 @@ KDirSelectDialog::KDirSelectDialog(const QUrl 
> , bool localOnly, QWidget
>  m_buttons->setStandardButtons(QDialogButtonBox::Ok | 
> QDialogButtonBox::Cancel);
>  connect(m_buttons, SIGNAL(accepted()), this, SLOT(accept()));
>  connect(m_buttons, SIGNAL(rejected()), this, SLOT(reject()));
> +if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, 
> nullptr, this)) {
> +foreach (QAbstractButton *button, m_buttons->buttons())
> +button->setIcon(QIcon());
> +}
>  topLayout->addWidget(m_buttons);
>  
>  QHBoxLayout *hlay = new QHBoxLayout(page);
> ```
> 
> But that somehow doesn't scale.
> 
> KGuiItem::apply would have to catch that, but doesn't.
> QDialogButtonBox::addButton *could* catch things, but doesn't (also it's 
> not clear whether or when m_buttons->button(SomeRole)->setIcon() is invoked, 
> so QPushButton::setIcon() would have to catch it and QPushButton would have 
> to catch it on reparenting.
> 
> 
> => For a complete solution, QPushButton actually needs painting code to 
> check the parent & style hint when setting up the style option - or we simply 
> rely on the style testing (widget && 
> qobject_cast(widget->parentWidget())) when calculating 
> sizes and painting the button.
> The only alternative is to walk a long way and tell everyone to please 
> check the hint and fix their buttonboxes eewww.
> 
> 
> 
> ==> QPushButton will require a "fix" to handle the StyleHint, but we 
> cannot expect *Q*PushButton to honor some global KDE setting, that's really a 
> job for the platform integration and in this case ultimately the style.
> 
> 
> 
> 
> => the platform integration plugin (KDE part in all Qt applications) 
> needs to read that setting and expose it to the styles.
> Alternatively the styles could read the setting from kdeglobals directly, 
> but that requires them to link kconfig (to do it correctly, just grabbing it 
> from the users kdeglobals ini isn't that much of a problem ;-), ie. rules out 
> Qt-only styles (and somehow contrasts w/ the KF5 approach to

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

2015-12-21 Thread René J . V . Bertin



> On Dec. 19, 2015, 11:23 a.m., David Faure wrote:
> > src/kdeui/kpushbutton.cpp, line 256
> > 
> >
> > 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?
> 
> René J.V. Bertin wrote:
> David, I'm dropping this whole RR, leaving it open only in case Thomas 
> wants to pursue his view on fixing what there is to fix.
> 
> You are right though, it's a workaround for KPushButton which doesn't 
> depend on fixing QPushButton. And whether or not QPushButton requires fixing 
> is apparently the big question. What is your idea on the scope of 
> `ShowIconsOnButtons`?
> 
> Thomas Lübking wrote:
> The "problem" is that this is really scattered around everywhere :(
> 
> One major catch should be (frameworksintegration)
> 
> ```
> diff --git a/src/platformtheme/kdeplatformfiledialoghelper.cpp 
> b/src/platformtheme/kdeplatformfiledialoghelper.cpp
> index 11e7efb..9cd374c 100644
> --- a/src/platformtheme/kdeplatformfiledialoghelper.cpp
> +++ b/src/platformtheme/kdeplatformfiledialoghelper.cpp
> @@ -161,6 +161,11 @@ void 
> KDEPlatformFileDialog::setFileMode(QFileDialogOptions::FileMode mode)
>  m_fileWidget->setMode(KFile::File);
>  break;
>  }
> +// ::setOperationMode happily adds icons to our buttonbox, so we 
> clear them everytime the mode is set
> +if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, 
> nullptr, this)) {
> +foreach (QAbstractButton *button, m_buttons->buttons())
> +button->setIcon(QIcon());
> +}
>  }
>  
>  void 
> KDEPlatformFileDialog::setCustomLabel(QFileDialogOptions::DialogLabel label, 
> const QString )
> diff --git a/src/platformtheme/kdirselectdialog.cpp 
> b/src/platformtheme/kdirselectdialog.cpp
> index 0a65cd3..13d7dc7 100644
> --- a/src/platformtheme/kdirselectdialog.cpp
> +++ b/src/platformtheme/kdirselectdialog.cpp
> @@ -299,6 +299,10 @@ KDirSelectDialog::KDirSelectDialog(const QUrl 
> , bool localOnly, QWidget
>  m_buttons->setStandardButtons(QDialogButtonBox::Ok | 
> QDialogButtonBox::Cancel);
>  connect(m_buttons, SIGNAL(accepted()), this, SLOT(accept()));
>  connect(m_buttons, SIGNAL(rejected()), this, SLOT(reject()));
> +if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, 
> nullptr, this)) {
> +foreach (QAbstractButton *button, m_buttons->buttons())
> +button->setIcon(QIcon());
> +}
>  topLayout->addWidget(m_buttons);
>  
>  QHBoxLayout *hlay = new QHBoxLayout(page);
> ```
> 
> But that somehow doesn't scale.
> 
> KGuiItem::apply would have to catch that, but doesn't.
> QDialogButtonBox::addButton *could* catch things, but doesn't (also it's 
> not clear whether or when m_buttons->button(SomeRole)->setIcon() is invoked, 
> so QPushButton::setIcon() would have to catch it and QPushButton would have 
> to catch it on reparenting.
> 
> 
> => For a complete solution, QPushButton actually needs painting code to 
> check the parent & style hint when setting up the style option - or we simply 
> rely on the style testing (widget && 
> qobject_cast(widget->parentWidget())) when calculating 
> sizes and painting the button.
> The only alternative is to walk a long way and tell everyone to please 
> check the hint and fix their buttonboxes eewww.
> 
> 
> 
> ==> QPushButton will require a "fix" to handle the StyleHint, but we 
> cannot expect *Q*PushButton to honor some global KDE setting, that's really a 
> job for the platform integration and in this case ultimately the style.
> 
> 
> 
> 
> => the platform integration plugin (KDE part in all Qt applications) 
> needs to read that setting and expose it to the styles.
> Alternatively the styles could read the setting from kdeglobals directly, 
> but that requires them to link kconfig (to do it correctly, just grabbing it 
> from the users kdeglobals ini isn't that much of a problem ;-), ie. rules out 
> Qt-only styles (and somehow contrasts w/ the KF5 approach to

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

2015-12-17 Thread René J . V . Bertin



> On Dec. 10, 2015, 11:11 p.m., Thomas Lübking wrote:
> > 1. What tells you that this is a dialog buttonbox pushbutton?
> > 2. What happens if the button has no text?
> > 
> > 
> > The bug is in QDialogButtonBox (or rather the K variant, 
> > QDialogButtonBoxPrivate::createButton() seems to incorporate the hint 
> > correctly)
> > 
> > 
> > [KDE] has "ShowIconsOnPushButtons=false" in kdeglobals which was 
> > interpreted by KPushButton _and_ kstyle for the hint, but the hint only 
> > covers Q'DialogButtonBox'es - there's simply no global rule for this like 
> > AA_DontShowIconsInMenus
> > 
> > => KDialogButtonBox shall respect the hint for its buttons (there're two 
> > special creation routines).
> > 
> > For the rest, the platform plugin ideally picks the kdeglobals setting and 
> > exports it to the application object (dyn property?) where the style can 
> > pick it and incorporate it into its calculations (ie. if no icons are 
> > wanted and there's text or arrow, omit the icon in size calculation and 
> > painting)
> > 
> > "Fixing" that in deprecated KPushButton doesn't really fix anything. We'll 
> > face the mix we had, just that users of QPushButton were far less prone to 
> > pass them an icon in pre-KF5 times.
> > 
> > Please also attach Hugo Pereira Da Costa.
> 
> René J.V. Bertin wrote:
> 1: why should one care? It is said nowhere that the setting defined as 
> "show icons on buttons" in `systemsettings(5)` applies only to dialogs. 
> Rather, the tooltip says "when this is enabled, KDE applications will show 
> icons alongside [sic!] some important buttons".
> In other words, when "this" is *not* enabled, there should be no icons, 
> period.
> I have found no sign in the code that the ShowIconsOnPushButtons hint is 
> to be used only for dialogs; `SH_DialogButtonBox_ButtonsHaveIcons` indeed 
> carries the suggestion in its name but I would not be surprised if Qt really 
> thinks of it in a more general sense. Probably also because the notion of 
> what a dialog is has become a lot vaguer.
> 
> And that also happens to be what Qt does; buttons show their icons on 
> Linux (and other Unix variants?) but on OS X or MS Windows displaying of 
> those icons is deactivated unless you use a style that enables it. In fact, 
> the default setting for `SH_DialogButtonBox_ButtonsHaveIcons` is false except 
> in the generic Unix theme (= it does work globally like 
> `AA_DontShowIconsInMenus` everywhere else).
> 
> 2: a user who indicates s/he doesn't want to see icons will get an empty 
> button. That's also what can happen with QPushButton, and app developers have 
> to take this into consideration. Cf. toolbars (and Qt's position on the use 
> of "texted separators").
> I don't think I've ever come across a standard button showing only an 
> icon, except possibly the arrow button next to the progress indicators in 
> KMail and KDevelop.
> 
> As to fixing it here: as it turns out, "here" is the main source for 
> annoying icons rearing their silly heads on buttons on my screen ;) and it 
> was also something of a challenge to understand why they kept appearing 
> despite the fact that all code appeared to return the value of 
> `ShowIconsOnPushButtons`. Deprecated or not, it doesn't look like all 
> applications are going to stop using it anytime soon.
> 
> I looked into fixing the issue in KDialogButtonBox but saw that it does 
> nothing to override the `ShowIconsOnPushButtons` setting. The only way to 
> respect the setting through that class (or a modern equivalent) would be to 
> set an empty icon if `ShowIconsOnPushButtons=false`. That introduces another 
> regression: changes in this setting are supposed to be reflected by running 
> applications without requiring a restart (or a recreation of dialogs). If it 
> were just me I'd decree that buttons can have either text or an icon, but 
> right now we have to make do with this mixed situation.
> 
> I don't mind making this an OS X (and MS Windows) specific modification, 
> of course, but on those platforms
> 
> Thomas Lübking wrote:
> > 1: why should one care?
> 
> Because, as explained, that is what the hint says. Nothing else.
> 
> > I have found no sign in the code that the ShowIconsOnPushButtons hint 
> is to be used only for dialogs
> 
> No, but it's been used to feed SH_DialogButtonBox_ButtonsHaveIcons *AND* 
> KPushButton.
> ShowIconsOnPushButtons implied SH_DialogButtonBox_ButtonsHaveIcons but 
> NOT vv.
> 
> The approach is wrong, since you're abusing the hint for generalisation.
> 
> > but on OS X or MS Windows
> 
> ... Qt uses native elements which might simply globally downforce the 
> pushbutton icon nonsense (as could any style - I was more than once close to 
> doing that in virtuality)
> Eg. Breeze might do that on favor of the HIG, but that's not relevant 
> here.
> 
> Downforcing in KPushButton means to

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

2015-12-17 Thread René J . V . Bertin



> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> >
> 
> René J.V. Bertin wrote:
> For the record: I've raised a few interrogations that are preventing me 
> from following up and addressing the open issues raised by Thomas.

I'm stepping out, patching icons out of buttons elsewhere but in the style no 
longer has any urgency for me. 

A final word of advice: try to get the Qt devs to indicate what they really 
intend with `SH_DialogButtonBox_ButtonsHaveIcons` : the end result on the 
screen (i.e. ButtonsShowIcons) or the result at the code/data level (i.e. 
ButtonsHaveIconMemberVariablesThatAreNotNull). If the former, there's no bug to 
patch (except in the style).


> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 39
> > 
> >
> > QDialogButtonBox::addButton should do correctly anyway, so please don't 
> > workaround things that are not broken.
> 
> René J.V. Bertin wrote:
> No, I've looked at Qt 5.5.1 . The only QDialogButtonBox::addButton that 
> "does correctly" is the one that takes a StandardButton. I haven't had time 
> to test this (will need to rebuild QtBase first) but I'm pretty sure that 
> that method creates a button with an icon with its sequence
> 
> ```
> QPushButton *button = new QPushButton(text, this);
> d->addButton(button, role);
> ```
> 
> My approach here is to avoid adding an icon if ButtonsHaveIcons is false, 
> or remove the icon if one was already added. That's what QDB does with its 
> ::addButton(StandardButton btn) method (calling a private createButton() 
> method). Any other approach is useless without a style supporting and 
> enforcing ButtonsHaveIcons, but which such a style KDialogButtonBox doesn't 
> need to be fixed in the first place...
> 
> Thomas Lübking wrote:
> ::addButton(text, role) creates "new QPushButton(text, this)" - those 
> should seriously not have any icons.
> 
> > whith such a style KDialogButtonBox doesn't need to be fixed in the 
> first place
> 
> If it's broken, it needs to be fixed - you cannot bail out with "the 
> style is correcting that for us" (I've been fixing far too many kdelibs/qtgui 
> bugs in the style ;-)
> 
> René J.V. Bertin wrote:
> > ::addButton(text, role) creates "new QPushButton(text, this)" - those 
> should seriously not have any icons.
> 
> Agreed, they shouldn't *show* any if the user doesn't want to see them. 
> AFAIC they can have a whole bunch of icons as long as they're not displayed. 
> 
> This argument is a bit useless as long as we don't know if an interface 
> should stop showing icons the moment the user unticks the corresponding 
> setting in systemsettings (and start showing them again when the setting is 
> ticked). If it's ok to tell the user that "the new setting will only be 
> respected after an application restart", then fine, let's simply not add 
> icons when they're not wanted. In all those countless places where the 
> setting will have to be applied.
> 
> But didn't you point out yourself that the style is in the best position 
> to avoid drawing any unwanted icons?
> 
> Thomas Lübking wrote:
> If you create a PushButton with this constructor, the button has at this 
> point no icon assigned. This has absolutely nothing to do with some setting 
> or the display of anything - where should the button have obtained an icon? 
> None is set here in the first place. QPushButton::icon().isNull()

Yes, apparently my assumption was wrong. It beats me where that icon gets set 
then, and how it'll be possible ultimately to prevent one from getting set. I'm 
going to hand this one back to you - I've solved my personal icon gripe in the 
style I use so as far as I'm concerned there is no longer a bug here O:-)


> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 70
> > 
> >
> > Setting the icon is sufficient, please do not mess around with other 
> > attributes.
> 
> René J.V. Bertin wrote:
> Are you sure? setIcon() doesn't call setIconSize() nor does it reset any 
> size information already present. Is it a good idea to replace an icon and 
> leaving the size information from the previous icon)? NB: should the icon 
> from the KGuiItem override the role's standard icon or should it be the other 
> way round (provided icon as a default when the role doesn't provide an icon, 
> for instance)?
> 
> Thomas Lübking wrote:
> setIcon *shall* not setIconSize, the two values are completely 
> orhtogonal. Dropping the size information will just get you into trouble when 
> you should require it again.
> If eg. a style would reserve icon space of iconSize despite there 
> actually is no icon to paint, the style is simply broken.
> 
> The icon size refers

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

2015-12-13 Thread René J . V . Bertin



> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> >

For the record: I've raised a few interrogations that are preventing me from 
following up and addressing the open issues raised by Thomas.


- René J.V.


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


On Dec. 11, 2015, 5: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, 5: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
> 
>

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

2015-12-13 Thread René J . V . Bertin



> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 39
> > 
> >
> > QDialogButtonBox::addButton should do correctly anyway, so please don't 
> > workaround things that are not broken.
> 
> René J.V. Bertin wrote:
> No, I've looked at Qt 5.5.1 . The only QDialogButtonBox::addButton that 
> "does correctly" is the one that takes a StandardButton. I haven't had time 
> to test this (will need to rebuild QtBase first) but I'm pretty sure that 
> that method creates a button with an icon with its sequence
> 
> ```
> QPushButton *button = new QPushButton(text, this);
> d->addButton(button, role);
> ```
> 
> My approach here is to avoid adding an icon if ButtonsHaveIcons is false, 
> or remove the icon if one was already added. That's what QDB does with its 
> ::addButton(StandardButton btn) method (calling a private createButton() 
> method). Any other approach is useless without a style supporting and 
> enforcing ButtonsHaveIcons, but which such a style KDialogButtonBox doesn't 
> need to be fixed in the first place...
> 
> Thomas Lübking wrote:
> ::addButton(text, role) creates "new QPushButton(text, this)" - those 
> should seriously not have any icons.
> 
> > whith such a style KDialogButtonBox doesn't need to be fixed in the 
> first place
> 
> If it's broken, it needs to be fixed - you cannot bail out with "the 
> style is correcting that for us" (I've been fixing far too many kdelibs/qtgui 
> bugs in the style ;-)

> ::addButton(text, role) creates "new QPushButton(text, this)" - those should 
> seriously not have any icons.

Agreed, they shouldn't *show* any if the user doesn't want to see them. AFAIC 
they can have a whole bunch of icons as long as they're not displayed. 

This argument is a bit useless as long as we don't know if an interface should 
stop showing icons the moment the user unticks the corresponding setting in 
systemsettings (and start showing them again when the setting is ticked). If 
it's ok to tell the user that "the new setting will only be respected after an 
application restart", then fine, let's simply not add icons when they're not 
wanted. In all those countless places where the setting will have to be applied.

But didn't you point out yourself that the style is in the best position to 
avoid drawing any unwanted icons?


> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 57
> > 
> >
> > you can completely spare this, there's no reason to manipulate a copy 
> > of the GuiItem, just burns CPU
> 
> René J.V. Bertin wrote:
> In that case I'll have to remove the `const` from guiitem, meaning a 
> change to the API.
> 
> Thomas Lübking wrote:
> No, you do not touch the KGuiItem that comes (it's not yours!) and it's 
> pointless to create a copy, strip the icon from that, assign it to the button 
> (and maybe even strip the icon from the button) - you just apply the GuiItem 
> that enters and strip the icon from the button. The interim (deep) GuiItem 
> copy is just detour in the path to remove the icon from the button.

I must be getting on with other real-life stuff now; I agree with not touching 
the incoming item of course. I'll see if there isn't a way to avoid adding the 
unwanted icon at all, to avoid the icon deep copy as well (probably the most 
expensive part of a GuiItem deep copy, no?)


> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 61
> > 
> >
> > unrelated and it won't leak, since the cleanup is done by the 
> > parent/child relation ("this" passed to KPushButton)
> 
> René J.V. Bertin wrote:
> Maybe it won't leak, but the question remains why what buttons with an 
> invalid role are good for.
> 
> Thomas Lübking wrote:
> Did you see such role being used? It would be a client code bug (the 
> symbol is juts for completeness sake, so that you can eg. assign it to a 
> role, pipe that to some transformation functions, check whether it's still 
> invalid and then react to that)

No, I haven't seen such a role (and client code bug, agreed on that). I did see 
however that there is no trivial way to check for it; you apparently have to 
look at the role before adding the button, or else query the KDialogButtonBox 
if it owns the button (if that's even possible).


> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 70
> > 
> >
> > Setting the icon is sufficient, please do not mess around with other 
> > attributes.
> 
> René J.V. Bertin wrote:
> Are you sure? setIcon() doesn't call

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



> On Dec. 10, 2015, 11:11 p.m., Thomas Lübking wrote:
> > 1. What tells you that this is a dialog buttonbox pushbutton?
> > 2. What happens if the button has no text?
> > 
> > 
> > The bug is in QDialogButtonBox (or rather the K variant, 
> > QDialogButtonBoxPrivate::createButton() seems to incorporate the hint 
> > correctly)
> > 
> > 
> > [KDE] has "ShowIconsOnPushButtons=false" in kdeglobals which was 
> > interpreted by KPushButton _and_ kstyle for the hint, but the hint only 
> > covers Q'DialogButtonBox'es - there's simply no global rule for this like 
> > AA_DontShowIconsInMenus
> > 
> > => KDialogButtonBox shall respect the hint for its buttons (there're two 
> > special creation routines).
> > 
> > For the rest, the platform plugin ideally picks the kdeglobals setting and 
> > exports it to the application object (dyn property?) where the style can 
> > pick it and incorporate it into its calculations (ie. if no icons are 
> > wanted and there's text or arrow, omit the icon in size calculation and 
> > painting)
> > 
> > "Fixing" that in deprecated KPushButton doesn't really fix anything. We'll 
> > face the mix we had, just that users of QPushButton were far less prone to 
> > pass them an icon in pre-KF5 times.
> > 
> > Please also attach Hugo Pereira Da Costa.
> 
> René J.V. Bertin wrote:
> 1: why should one care? It is said nowhere that the setting defined as 
> "show icons on buttons" in `systemsettings(5)` applies only to dialogs. 
> Rather, the tooltip says "when this is enabled, KDE applications will show 
> icons alongside [sic!] some important buttons".
> In other words, when "this" is *not* enabled, there should be no icons, 
> period.
> I have found no sign in the code that the ShowIconsOnPushButtons hint is 
> to be used only for dialogs; `SH_DialogButtonBox_ButtonsHaveIcons` indeed 
> carries the suggestion in its name but I would not be surprised if Qt really 
> thinks of it in a more general sense. Probably also because the notion of 
> what a dialog is has become a lot vaguer.
> 
> And that also happens to be what Qt does; buttons show their icons on 
> Linux (and other Unix variants?) but on OS X or MS Windows displaying of 
> those icons is deactivated unless you use a style that enables it. In fact, 
> the default setting for `SH_DialogButtonBox_ButtonsHaveIcons` is false except 
> in the generic Unix theme (= it does work globally like 
> `AA_DontShowIconsInMenus` everywhere else).
> 
> 2: a user who indicates s/he doesn't want to see icons will get an empty 
> button. That's also what can happen with QPushButton, and app developers have 
> to take this into consideration. Cf. toolbars (and Qt's position on the use 
> of "texted separators").
> I don't think I've ever come across a standard button showing only an 
> icon, except possibly the arrow button next to the progress indicators in 
> KMail and KDevelop.
> 
> As to fixing it here: as it turns out, "here" is the main source for 
> annoying icons rearing their silly heads on buttons on my screen ;) and it 
> was also something of a challenge to understand why they kept appearing 
> despite the fact that all code appeared to return the value of 
> `ShowIconsOnPushButtons`. Deprecated or not, it doesn't look like all 
> applications are going to stop using it anytime soon.
> 
> I looked into fixing the issue in KDialogButtonBox but saw that it does 
> nothing to override the `ShowIconsOnPushButtons` setting. The only way to 
> respect the setting through that class (or a modern equivalent) would be to 
> set an empty icon if `ShowIconsOnPushButtons=false`. That introduces another 
> regression: changes in this setting are supposed to be reflected by running 
> applications without requiring a restart (or a recreation of dialogs). If it 
> were just me I'd decree that buttons can have either text or an icon, but 
> right now we have to make do with this mixed situation.
> 
> I don't mind making this an OS X (and MS Windows) specific modification, 
> of course, but on those platforms
> 
> Thomas Lübking wrote:
> > 1: why should one care?
> 
> Because, as explained, that is what the hint says. Nothing else.
> 
> > I have found no sign in the code that the ShowIconsOnPushButtons hint 
> is to be used only for dialogs
> 
> No, but it's been used to feed SH_DialogButtonBox_ButtonsHaveIcons *AND* 
> KPushButton.
> ShowIconsOnPushButtons implied SH_DialogButtonBox_ButtonsHaveIcons but 
> NOT vv.
> 
> The approach is wrong, since you're abusing the hint for generalisation.
> 
> > but on OS X or MS Windows
> 
> ... Qt uses native elements which might simply globally downforce the 
> pushbutton icon nonsense (as could any style - I was more than once close to 
> doing that in virtuality)
> Eg. Breeze might do that on favor of the HIG, but that's not relevant 
> here.
> 
> Downforcing in KPushButton means to

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



> On Dec. 10, 2015, 11:11 p.m., Thomas Lübking wrote:
> > 1. What tells you that this is a dialog buttonbox pushbutton?
> > 2. What happens if the button has no text?
> > 
> > 
> > The bug is in QDialogButtonBox (or rather the K variant, 
> > QDialogButtonBoxPrivate::createButton() seems to incorporate the hint 
> > correctly)
> > 
> > 
> > [KDE] has "ShowIconsOnPushButtons=false" in kdeglobals which was 
> > interpreted by KPushButton _and_ kstyle for the hint, but the hint only 
> > covers Q'DialogButtonBox'es - there's simply no global rule for this like 
> > AA_DontShowIconsInMenus
> > 
> > => KDialogButtonBox shall respect the hint for its buttons (there're two 
> > special creation routines).
> > 
> > For the rest, the platform plugin ideally picks the kdeglobals setting and 
> > exports it to the application object (dyn property?) where the style can 
> > pick it and incorporate it into its calculations (ie. if no icons are 
> > wanted and there's text or arrow, omit the icon in size calculation and 
> > painting)
> > 
> > "Fixing" that in deprecated KPushButton doesn't really fix anything. We'll 
> > face the mix we had, just that users of QPushButton were far less prone to 
> > pass them an icon in pre-KF5 times.
> > 
> > Please also attach Hugo Pereira Da Costa.
> 
> René J.V. Bertin wrote:
> 1: why should one care? It is said nowhere that the setting defined as 
> "show icons on buttons" in `systemsettings(5)` applies only to dialogs. 
> Rather, the tooltip says "when this is enabled, KDE applications will show 
> icons alongside [sic!] some important buttons".
> In other words, when "this" is *not* enabled, there should be no icons, 
> period.
> I have found no sign in the code that the ShowIconsOnPushButtons hint is 
> to be used only for dialogs; `SH_DialogButtonBox_ButtonsHaveIcons` indeed 
> carries the suggestion in its name but I would not be surprised if Qt really 
> thinks of it in a more general sense. Probably also because the notion of 
> what a dialog is has become a lot vaguer.
> 
> And that also happens to be what Qt does; buttons show their icons on 
> Linux (and other Unix variants?) but on OS X or MS Windows displaying of 
> those icons is deactivated unless you use a style that enables it. In fact, 
> the default setting for `SH_DialogButtonBox_ButtonsHaveIcons` is false except 
> in the generic Unix theme (= it does work globally like 
> `AA_DontShowIconsInMenus` everywhere else).
> 
> 2: a user who indicates s/he doesn't want to see icons will get an empty 
> button. That's also what can happen with QPushButton, and app developers have 
> to take this into consideration. Cf. toolbars (and Qt's position on the use 
> of "texted separators").
> I don't think I've ever come across a standard button showing only an 
> icon, except possibly the arrow button next to the progress indicators in 
> KMail and KDevelop.
> 
> As to fixing it here: as it turns out, "here" is the main source for 
> annoying icons rearing their silly heads on buttons on my screen ;) and it 
> was also something of a challenge to understand why they kept appearing 
> despite the fact that all code appeared to return the value of 
> `ShowIconsOnPushButtons`. Deprecated or not, it doesn't look like all 
> applications are going to stop using it anytime soon.
> 
> I looked into fixing the issue in KDialogButtonBox but saw that it does 
> nothing to override the `ShowIconsOnPushButtons` setting. The only way to 
> respect the setting through that class (or a modern equivalent) would be to 
> set an empty icon if `ShowIconsOnPushButtons=false`. That introduces another 
> regression: changes in this setting are supposed to be reflected by running 
> applications without requiring a restart (or a recreation of dialogs). If it 
> were just me I'd decree that buttons can have either text or an icon, but 
> right now we have to make do with this mixed situation.
> 
> I don't mind making this an OS X (and MS Windows) specific modification, 
> of course, but on those platforms
> 
> Thomas Lübking wrote:
> > 1: why should one care?
> 
> Because, as explained, that is what the hint says. Nothing else.
> 
> > I have found no sign in the code that the ShowIconsOnPushButtons hint 
> is to be used only for dialogs
> 
> No, but it's been used to feed SH_DialogButtonBox_ButtonsHaveIcons *AND* 
> KPushButton.
> ShowIconsOnPushButtons implied SH_DialogButtonBox_ButtonsHaveIcons but 
> NOT vv.
> 
> The approach is wrong, since you're abusing the hint for generalisation.
> 
> > but on OS X or MS Windows
> 
> ... Qt uses native elements which might simply globally downforce the 
> pushbutton icon nonsense (as could any style - I was more than once close to 
> doing that in virtuality)
> Eg. Breeze might do that on favor of the HIG, but that's not relevant 
> here.
> 
> Downforcing in KPushButton means to

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


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

(Updated Dec. 11, 2015, 1:42 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks, Qt KDE, and Hugo 
Pereira Da Costa.


Changes
---

This adds the changes to KDialogButtonBox that seem required to respect 
`SH_DialogButtonBox_ButtonsHaveIcons` regardless of `ShowIconsOnPushButtons` (= 
if the former could be independent of the latter e.g. when using a style that 
does not use the latter to determine the value of the former).

What is the point in allowing `KDialogButtonBox::addButton` to create a button 
that is not added because of an invalid role? It seems that button wouldn't 
appear (or in an unexpected place), and be leaked?


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

  src/kdeui/kpushbutton.cpp 98534fa 
  src/kdeui/kdialogbuttonbox.cpp 0f6649b 

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

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


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

(Updated Dec. 11, 2015, 1:47 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks, Qt KDE, and Hugo 
Pereira Da Costa.


Changes
---

oops, apologies, I was a bit too fast there.


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

  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

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


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

(Updated Dec. 11, 2015, 1:59 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks, Qt KDE, and Hugo 
Pereira Da Costa.


Changes
---

This patch for KDialogButtonBox actually builds.


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

  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

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



> On Dec. 10, 2015, 11:11 p.m., Thomas Lübking wrote:
> > 1. What tells you that this is a dialog buttonbox pushbutton?
> > 2. What happens if the button has no text?
> > 
> > 
> > The bug is in QDialogButtonBox (or rather the K variant, 
> > QDialogButtonBoxPrivate::createButton() seems to incorporate the hint 
> > correctly)
> > 
> > 
> > [KDE] has "ShowIconsOnPushButtons=false" in kdeglobals which was 
> > interpreted by KPushButton _and_ kstyle for the hint, but the hint only 
> > covers Q'DialogButtonBox'es - there's simply no global rule for this like 
> > AA_DontShowIconsInMenus
> > 
> > => KDialogButtonBox shall respect the hint for its buttons (there're two 
> > special creation routines).
> > 
> > For the rest, the platform plugin ideally picks the kdeglobals setting and 
> > exports it to the application object (dyn property?) where the style can 
> > pick it and incorporate it into its calculations (ie. if no icons are 
> > wanted and there's text or arrow, omit the icon in size calculation and 
> > painting)
> > 
> > "Fixing" that in deprecated KPushButton doesn't really fix anything. We'll 
> > face the mix we had, just that users of QPushButton were far less prone to 
> > pass them an icon in pre-KF5 times.
> > 
> > Please also attach Hugo Pereira Da Costa.
> 
> René J.V. Bertin wrote:
> 1: why should one care? It is said nowhere that the setting defined as 
> "show icons on buttons" in `systemsettings(5)` applies only to dialogs. 
> Rather, the tooltip says "when this is enabled, KDE applications will show 
> icons alongside [sic!] some important buttons".
> In other words, when "this" is *not* enabled, there should be no icons, 
> period.
> I have found no sign in the code that the ShowIconsOnPushButtons hint is 
> to be used only for dialogs; `SH_DialogButtonBox_ButtonsHaveIcons` indeed 
> carries the suggestion in its name but I would not be surprised if Qt really 
> thinks of it in a more general sense. Probably also because the notion of 
> what a dialog is has become a lot vaguer.
> 
> And that also happens to be what Qt does; buttons show their icons on 
> Linux (and other Unix variants?) but on OS X or MS Windows displaying of 
> those icons is deactivated unless you use a style that enables it. In fact, 
> the default setting for `SH_DialogButtonBox_ButtonsHaveIcons` is false except 
> in the generic Unix theme (= it does work globally like 
> `AA_DontShowIconsInMenus` everywhere else).
> 
> 2: a user who indicates s/he doesn't want to see icons will get an empty 
> button. That's also what can happen with QPushButton, and app developers have 
> to take this into consideration. Cf. toolbars (and Qt's position on the use 
> of "texted separators").
> I don't think I've ever come across a standard button showing only an 
> icon, except possibly the arrow button next to the progress indicators in 
> KMail and KDevelop.
> 
> As to fixing it here: as it turns out, "here" is the main source for 
> annoying icons rearing their silly heads on buttons on my screen ;) and it 
> was also something of a challenge to understand why they kept appearing 
> despite the fact that all code appeared to return the value of 
> `ShowIconsOnPushButtons`. Deprecated or not, it doesn't look like all 
> applications are going to stop using it anytime soon.
> 
> I looked into fixing the issue in KDialogButtonBox but saw that it does 
> nothing to override the `ShowIconsOnPushButtons` setting. The only way to 
> respect the setting through that class (or a modern equivalent) would be to 
> set an empty icon if `ShowIconsOnPushButtons=false`. That introduces another 
> regression: changes in this setting are supposed to be reflected by running 
> applications without requiring a restart (or a recreation of dialogs). If it 
> were just me I'd decree that buttons can have either text or an icon, but 
> right now we have to make do with this mixed situation.
> 
> I don't mind making this an OS X (and MS Windows) specific modification, 
> of course, but on those platforms
> 
> Thomas Lübking wrote:
> > 1: why should one care?
> 
> Because, as explained, that is what the hint says. Nothing else.
> 
> > I have found no sign in the code that the ShowIconsOnPushButtons hint 
> is to be used only for dialogs
> 
> No, but it's been used to feed SH_DialogButtonBox_ButtonsHaveIcons *AND* 
> KPushButton.
> ShowIconsOnPushButtons implied SH_DialogButtonBox_ButtonsHaveIcons but 
> NOT vv.
> 
> The approach is wrong, since you're abusing the hint for generalisation.
> 
> > but on OS X or MS Windows
> 
> ... Qt uses native elements which might simply globally downforce the 
> pushbutton icon nonsense (as could any style - I was more than once close to 
> doing that in virtuality)
> Eg. Breeze might do that on favor of the HIG, but that's not relevant 
> here.
> 
> Downforcing in KPushButton means to

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



> On Dec. 10, 2015, 11:11 p.m., Thomas Lübking wrote:
> > 1. What tells you that this is a dialog buttonbox pushbutton?
> > 2. What happens if the button has no text?
> > 
> > 
> > The bug is in QDialogButtonBox (or rather the K variant, 
> > QDialogButtonBoxPrivate::createButton() seems to incorporate the hint 
> > correctly)
> > 
> > 
> > [KDE] has "ShowIconsOnPushButtons=false" in kdeglobals which was 
> > interpreted by KPushButton _and_ kstyle for the hint, but the hint only 
> > covers Q'DialogButtonBox'es - there's simply no global rule for this like 
> > AA_DontShowIconsInMenus
> > 
> > => KDialogButtonBox shall respect the hint for its buttons (there're two 
> > special creation routines).
> > 
> > For the rest, the platform plugin ideally picks the kdeglobals setting and 
> > exports it to the application object (dyn property?) where the style can 
> > pick it and incorporate it into its calculations (ie. if no icons are 
> > wanted and there's text or arrow, omit the icon in size calculation and 
> > painting)
> > 
> > "Fixing" that in deprecated KPushButton doesn't really fix anything. We'll 
> > face the mix we had, just that users of QPushButton were far less prone to 
> > pass them an icon in pre-KF5 times.
> > 
> > Please also attach Hugo Pereira Da Costa.
> 
> René J.V. Bertin wrote:
> 1: why should one care? It is said nowhere that the setting defined as 
> "show icons on buttons" in `systemsettings(5)` applies only to dialogs. 
> Rather, the tooltip says "when this is enabled, KDE applications will show 
> icons alongside [sic!] some important buttons".
> In other words, when "this" is *not* enabled, there should be no icons, 
> period.
> I have found no sign in the code that the ShowIconsOnPushButtons hint is 
> to be used only for dialogs; `SH_DialogButtonBox_ButtonsHaveIcons` indeed 
> carries the suggestion in its name but I would not be surprised if Qt really 
> thinks of it in a more general sense. Probably also because the notion of 
> what a dialog is has become a lot vaguer.
> 
> And that also happens to be what Qt does; buttons show their icons on 
> Linux (and other Unix variants?) but on OS X or MS Windows displaying of 
> those icons is deactivated unless you use a style that enables it. In fact, 
> the default setting for `SH_DialogButtonBox_ButtonsHaveIcons` is false except 
> in the generic Unix theme (= it does work globally like 
> `AA_DontShowIconsInMenus` everywhere else).
> 
> 2: a user who indicates s/he doesn't want to see icons will get an empty 
> button. That's also what can happen with QPushButton, and app developers have 
> to take this into consideration. Cf. toolbars (and Qt's position on the use 
> of "texted separators").
> I don't think I've ever come across a standard button showing only an 
> icon, except possibly the arrow button next to the progress indicators in 
> KMail and KDevelop.
> 
> As to fixing it here: as it turns out, "here" is the main source for 
> annoying icons rearing their silly heads on buttons on my screen ;) and it 
> was also something of a challenge to understand why they kept appearing 
> despite the fact that all code appeared to return the value of 
> `ShowIconsOnPushButtons`. Deprecated or not, it doesn't look like all 
> applications are going to stop using it anytime soon.
> 
> I looked into fixing the issue in KDialogButtonBox but saw that it does 
> nothing to override the `ShowIconsOnPushButtons` setting. The only way to 
> respect the setting through that class (or a modern equivalent) would be to 
> set an empty icon if `ShowIconsOnPushButtons=false`. That introduces another 
> regression: changes in this setting are supposed to be reflected by running 
> applications without requiring a restart (or a recreation of dialogs). If it 
> were just me I'd decree that buttons can have either text or an icon, but 
> right now we have to make do with this mixed situation.
> 
> I don't mind making this an OS X (and MS Windows) specific modification, 
> of course, but on those platforms
> 
> Thomas Lübking wrote:
> > 1: why should one care?
> 
> Because, as explained, that is what the hint says. Nothing else.
> 
> > I have found no sign in the code that the ShowIconsOnPushButtons hint 
> is to be used only for dialogs
> 
> No, but it's been used to feed SH_DialogButtonBox_ButtonsHaveIcons *AND* 
> KPushButton.
> ShowIconsOnPushButtons implied SH_DialogButtonBox_ButtonsHaveIcons but 
> NOT vv.
> 
> The approach is wrong, since you're abusing the hint for generalisation.
> 
> > but on OS X or MS Windows
> 
> ... Qt uses native elements which might simply globally downforce the 
> pushbutton icon nonsense (as could any style - I was more than once close to 
> doing that in virtuality)
> Eg. Breeze might do that on favor of the HIG, but that's not relevant 
> here.
> 
> Downforcing in KPushButton means to

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

> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 36
> > 
> >
> > unrelated and not required

Not required indeed, but related in the sense that it removes any ambiguity on 
what method is being called ;)

> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 39
> > 
> >
> > QDialogButtonBox::addButton should do correctly anyway, so please don't 
> > workaround things that are not broken.

No, I've looked at Qt 5.5.1 . The only QDialogButtonBox::addButton that "does 
correctly" is the one that takes a StandardButton. I haven't had time to test 
this (will need to rebuild QtBase first) but I'm pretty sure that that method 
creates a button with an icon with its sequence

```
QPushButton *button = new QPushButton(text, this);
d->addButton(button, role);
```

My approach here is to avoid adding an icon if ButtonsHaveIcons is false, or 
remove the icon if one was already added. That's what QDB does with its 
::addButton(StandardButton btn) method (calling a private createButton() 
method). Any other approach is useless without a style supporting and enforcing 
ButtonsHaveIcons, but which such a style KDialogButtonBox doesn't need to be 
fixed in the first place...

> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 57
> > 
> >
> > you can completely spare this, there's no reason to manipulate a copy 
> > of the GuiItem, just burns CPU

In that case I'll have to remove the `const` from guiitem, meaning a change to 
the API.

> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 70
> > 
> >
> > Setting the icon is sufficient, please do not mess around with other 
> > attributes.

Are you sure? setIcon() doesn't call setIconSize() nor does it reset any size 
information already present. Is it a good idea to replace an icon and leaving 
the size information from the previous icon)? NB: should the icon from the 
KGuiItem override the role's standard icon or should it be the other way round 
(provided icon as a default when the role doesn't provide an icon, for 
instance)?

> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 75
> > 
> >
> > this is really the only thing you should need to do here.

Cf. the previous comment about icon priority: this method can provide 2 icons 
that the button will have to chose from.

And I think that it's probably a good idea to set the icon size to 0 when the 
intent is to remove the icon completely.

> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kpushbutton.cpp, line 257
> > 
> >
> > still wrong and again, please don't mess with the icon size - you're 
> > just tempting DIV zero segfaults.

what?? Code that doesn't check integer div 0 should be encouraged to crash. A 
different bug than the one we're addressing here, but not one I have any 
patience with/for.

I could use QSize() instead of QSize(0,0); the former would mean 
iconSize.isValid() will return false while with the latter it'll return true. 
But note that functions like QPushButton::sizeHint() do not check isValid. A 
bit of a conundrum.
Am I right that a button that never had an icon will have `iconSize() == 
QSize()` ?

- René J.V.

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

On Dec. 11, 2015, 3:03 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, 3:03 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

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



> On Dec. 11, 2015, 2:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 61
> > 
> >
> > unrelated and it won't leak, since the cleanup is done by the 
> > parent/child relation ("this" passed to KPushButton)

Maybe it won't leak, but the question remains why what buttons with an invalid 
role are good for.


- René J.V.


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


On Dec. 11, 2015, 3:03 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, 3:03 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
> 
>

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


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

(Updated Dec. 11, 2015, 5:26 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks, Qt KDE, Hugo 
Pereira Da Costa, and Yichao Yu.


Changes
---

Thomas, what exactly did you mean with "QDialogButtonBox::addButton should do 
correctly"? Looking at Qt's code again, I can confirm that 
QDB::addButton(StandardButton) is the only one invoking the private 
createButton method which in turn sets an icon if ButtonsHaveIcons is true. The 
other QDB::addButton methods simply call the private addButton method which 
will do a layout step by default, but I don't see where an icon would be added.

Should I understand that `style->standardIcon()` is invoked during drawing as a 
function of button's role?

I cannot find evidence of that in QtCurve. But if that is the case nonetheless, 
why are we patching KDialogButtonBox again? And how do you explain that 
removing the icon after a button is created has the effect it has (when the 
style will continue to see the button role)?


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

  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

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



> On Dec. 10, 2015, 11:11 p.m., Thomas Lübking wrote:
> > 1. What tells you that this is a dialog buttonbox pushbutton?
> > 2. What happens if the button has no text?
> > 
> > 
> > The bug is in QDialogButtonBox (or rather the K variant, 
> > QDialogButtonBoxPrivate::createButton() seems to incorporate the hint 
> > correctly)
> > 
> > 
> > [KDE] has "ShowIconsOnPushButtons=false" in kdeglobals which was 
> > interpreted by KPushButton _and_ kstyle for the hint, but the hint only 
> > covers Q'DialogButtonBox'es - there's simply no global rule for this like 
> > AA_DontShowIconsInMenus
> > 
> > => KDialogButtonBox shall respect the hint for its buttons (there're two 
> > special creation routines).
> > 
> > For the rest, the platform plugin ideally picks the kdeglobals setting and 
> > exports it to the application object (dyn property?) where the style can 
> > pick it and incorporate it into its calculations (ie. if no icons are 
> > wanted and there's text or arrow, omit the icon in size calculation and 
> > painting)
> > 
> > "Fixing" that in deprecated KPushButton doesn't really fix anything. We'll 
> > face the mix we had, just that users of QPushButton were far less prone to 
> > pass them an icon in pre-KF5 times.
> > 
> > Please also attach Hugo Pereira Da Costa.
> 
> René J.V. Bertin wrote:
> 1: why should one care? It is said nowhere that the setting defined as 
> "show icons on buttons" in `systemsettings(5)` applies only to dialogs. 
> Rather, the tooltip says "when this is enabled, KDE applications will show 
> icons alongside [sic!] some important buttons".
> In other words, when "this" is *not* enabled, there should be no icons, 
> period.
> I have found no sign in the code that the ShowIconsOnPushButtons hint is 
> to be used only for dialogs; `SH_DialogButtonBox_ButtonsHaveIcons` indeed 
> carries the suggestion in its name but I would not be surprised if Qt really 
> thinks of it in a more general sense. Probably also because the notion of 
> what a dialog is has become a lot vaguer.
> 
> And that also happens to be what Qt does; buttons show their icons on 
> Linux (and other Unix variants?) but on OS X or MS Windows displaying of 
> those icons is deactivated unless you use a style that enables it. In fact, 
> the default setting for `SH_DialogButtonBox_ButtonsHaveIcons` is false except 
> in the generic Unix theme (= it does work globally like 
> `AA_DontShowIconsInMenus` everywhere else).
> 
> 2: a user who indicates s/he doesn't want to see icons will get an empty 
> button. That's also what can happen with QPushButton, and app developers have 
> to take this into consideration. Cf. toolbars (and Qt's position on the use 
> of "texted separators").
> I don't think I've ever come across a standard button showing only an 
> icon, except possibly the arrow button next to the progress indicators in 
> KMail and KDevelop.
> 
> As to fixing it here: as it turns out, "here" is the main source for 
> annoying icons rearing their silly heads on buttons on my screen ;) and it 
> was also something of a challenge to understand why they kept appearing 
> despite the fact that all code appeared to return the value of 
> `ShowIconsOnPushButtons`. Deprecated or not, it doesn't look like all 
> applications are going to stop using it anytime soon.
> 
> I looked into fixing the issue in KDialogButtonBox but saw that it does 
> nothing to override the `ShowIconsOnPushButtons` setting. The only way to 
> respect the setting through that class (or a modern equivalent) would be to 
> set an empty icon if `ShowIconsOnPushButtons=false`. That introduces another 
> regression: changes in this setting are supposed to be reflected by running 
> applications without requiring a restart (or a recreation of dialogs). If it 
> were just me I'd decree that buttons can have either text or an icon, but 
> right now we have to make do with this mixed situation.
> 
> I don't mind making this an OS X (and MS Windows) specific modification, 
> of course, but on those platforms
> 
> Thomas Lübking wrote:
> > 1: why should one care?
> 
> Because, as explained, that is what the hint says. Nothing else.
> 
> > I have found no sign in the code that the ShowIconsOnPushButtons hint 
> is to be used only for dialogs
> 
> No, but it's been used to feed SH_DialogButtonBox_ButtonsHaveIcons *AND* 
> KPushButton.
> ShowIconsOnPushButtons implied SH_DialogButtonBox_ButtonsHaveIcons but 
> NOT vv.
> 
> The approach is wrong, since you're abusing the hint for generalisation.
> 
> > but on OS X or MS Windows
> 
> ... Qt uses native elements which might simply globally downforce the 
> pushbutton icon nonsense (as could any style - I was more than once close to 
> doing that in virtuality)
> Eg. Breeze might do that on favor of the HIG, but that's not relevant 
> here.
> 
> Downforcing in KPushButton means to

Review Request 126304: (re)enable building with -DQTC_QT5_ENABLE_KDE


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

Review request for KDE Frameworks, Qt KDE and Yichao Yu.


Repository: qtcurve


Description
---

As the title says, this restores the definition of QTC_QT5_ENABLE_KDE in the 
CMake file when a KF5 build is done, and introduces the changes required for 
that build to succeed.

Except for removing references to the m_componentData member (already removed 
from the class; deprecated type) I have not made any other changes to the code.


Diffs
-

  qt5/CMakeLists.txt 837d9c2 
  qt5/style/CMakeLists.txt 7f65f8c 
  qt5/style/qtcurve.cpp febcfcf 
  qt5/style/qtcurve_api.cpp 87a927f 
  qt5/style/qtcurve_p.h bfc7502 
  qt5/style/qtcurve_primitive.cpp b5a3204 

Diff: https://git.reviewboard.kde.org/r/126304/diff/


Testing
---

On KUbuntu 14.04 and Mac OS X 10.9.5, both with Qt 5.5.1 and KF5 Frameworks 
5.16.0 installed under /opt/local


Thanks,

René J.V. Bertin

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


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

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


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

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

> On Dec. 10, 2015, 11:11 p.m., Thomas Lübking wrote:
> > 1. What tells you that this is a dialog buttonbox pushbutton?
> > 2. What happens if the button has no text?
> > 
> > 
> > The bug is in QDialogButtonBox (or rather the K variant, 
> > QDialogButtonBoxPrivate::createButton() seems to incorporate the hint 
> > correctly)
> > 
> > 
> > [KDE] has "ShowIconsOnPushButtons=false" in kdeglobals which was 
> > interpreted by KPushButton _and_ kstyle for the hint, but the hint only 
> > covers Q'DialogButtonBox'es - there's simply no global rule for this like 
> > AA_DontShowIconsInMenus
> > 
> > => KDialogButtonBox shall respect the hint for its buttons (there're two 
> > special creation routines).
> > 
> > For the rest, the platform plugin ideally picks the kdeglobals setting and 
> > exports it to the application object (dyn property?) where the style can 
> > pick it and incorporate it into its calculations (ie. if no icons are 
> > wanted and there's text or arrow, omit the icon in size calculation and 
> > painting)
> > 
> > "Fixing" that in deprecated KPushButton doesn't really fix anything. We'll 
> > face the mix we had, just that users of QPushButton were far less prone to 
> > pass them an icon in pre-KF5 times.
> > 
> > Please also attach Hugo Pereira Da Costa.

1: why should one care? It is said nowhere that the setting defined as "show 
icons on buttons" in `systemsettings(5)` applies only to dialogs. Rather, the 
tooltip says "when this is enabled, KDE applications will show icons alongside 
[sic!] some important buttons".
In other words, when "this" is *not* enabled, there should be no icons, period.
I have found no sign in the code that the ShowIconsOnPushButtons hint is to be 
used only for dialogs; `SH_DialogButtonBox_ButtonsHaveIcons` indeed carries the 
suggestion in its name but I would not be surprised if Qt really thinks of it 
in a more general sense. Probably also because the notion of what a dialog is 
has become a lot vaguer.

And that also happens to be what Qt does; buttons show their icons on Linux 
(and other Unix variants?) but on OS X or MS Windows displaying of those icons 
is deactivated unless you use a style that enables it. In fact, the default 
setting for `SH_DialogButtonBox_ButtonsHaveIcons` is false except in the 
generic Unix theme (= it does work globally like `AA_DontShowIconsInMenus` 
everywhere else).

2: a user who indicates s/he doesn't want to see icons will get an empty 
button. That's also what can happen with QPushButton, and app developers have 
to take this into consideration. Cf. toolbars (and Qt's position on the use of 
"texted separators").
I don't think I've ever come across a standard button showing only an icon, 
except possibly the arrow button next to the progress indicators in KMail and 
KDevelop.

As to fixing it here: as it turns out, "here" is the main source for annoying 
icons rearing their silly heads on buttons on my screen ;) and it was also 
something of a challenge to understand why they kept appearing despite the fact 
that all code appeared to return the value of `ShowIconsOnPushButtons`. 
Deprecated or not, it doesn't look like all applications are going to stop 
using it anytime soon.

I looked into fixing the issue in KDialogButtonBox but saw that it does nothing 
to override the `ShowIconsOnPushButtons` setting. The only way to respect the 
setting through that class (or a modern equivalent) would be to set an empty 
icon if `ShowIconsOnPushButtons=false`. That introduces another regression: 
changes in this setting are supposed to be reflected by running applications 
without requiring a restart (or a recreation of dialogs). If it were just me 
I'd decree that buttons can have either text or an icon, but right now we have 
to make do with this mixed situation.

I don't mind making this an OS X (and MS Windows) specific modification, of 
course, but on those platforms

- René J.V.

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

On Dec. 10, 2015, 11:12 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. 10, 2015, 11:12 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks, Qt KDE, and Hugo 
> Pereira Da Costa.
> 
> 
> 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 
>

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


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

(Updated Dec. 10, 2015, 11:54 p.m.)


Review request for KDE Software on Mac OS X, KDE Frameworks, Qt KDE, and Hugo 
Pereira Da Costa.


Changes
---

Easy enough to avoid empty buttons!


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

  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

Re: Review Request 120573: [OS X] make the KDE4 trash use the OS X trash

2015-11-20 Thread René J . V . Bertin


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

(Updated Nov. 20, 2015, 10:11 p.m.)


Review request for KDE Software on Mac OS X, KDE Runtime and David Faure.


Changes
---

renamed to avoid confusion with the KF5 RR of the same name


Summary (updated)
-

[OS X] make the KDE4 trash use the OS X trash


Repository: kde-runtime


Description
---

KDE on OS X does not handle the desktop session (no "Plasma") nor can it rely 
on XDG to obtain the proper paths to use for something like the trash. As a 
result, all applications that propose to move things they manage to the 
wastebin (Dolphin, but also digiKam) will store those items in a place that has 
no particular meaning on OS X, and that will thus tend to fill up.

OS X stores trash in one of several locations. Files trashed from the boot 
volume (and/or the volume containing $HOME, I don't actually know that) end up 
in `~/.Trash`. Files deleted from other volumes end up in 
`/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless 
whether it's an external or a remote drive; only mounted NFS shares are handled 
differently) and uid the numerical user id. Permissions on `.Trashes` are the 
same as those expected by KDE.

The kio_trash kioslave appears to support several actual trash directory 
locations, just like OS X. `TrashImpl::init()` creates a standard trash in 
`~/.local/share/Trash` (at least under OS X) but also 
`TrashImpl::trashForMountPoint()` that is used in cases I have not yet 
encountered.

On OS X, my modified `TrashImpl::init()` sets the standard trash directory to 
`~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as 
required, because they will of course be deleted when the user empties the OS X 
trash. `TrashImpl::fileRemoved()` has been modified to call a new function, 
`deleteEmptyTrashInfraStructure` to delete the KDE trash's internal 
infrastructure when the wastebin is empty so that OS X also sees the trash as 
emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature 
actually works, as expected as far as I can tell).

Remains to be done:
- determine in what cases `trashForMountPoint()` is used, and finish the 
modifications for it to use `/.Trashes/uid/KDE.trash`


Diffs
-

  kioslave/trash/trashimpl.cpp fe2e152 
  kioslave/trash/trashimpl.h bc68723 
  kioslave/trash/tests/CMakeLists.txt 9161fdf 
  kioslave/trash/kcmtrash.cpp f4811fd 
  kioslave/trash/CMakeLists.txt 3604089 

Diff: https://git.reviewboard.kde.org/r/120573/diff/


Testing
---

On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested 
actions are
- move items to wastebin from $HOME and a directory on a different volume
- restore items to both places
- empty wastebin through Dolphin
- empty OS X trashcan


Thanks,

René J.V. Bertin

Re: Review Request 120573: [OS X] make the KDE4 trash use the OS X trash

2015-11-20 Thread René J . V . Bertin


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

(Updated Nov. 20, 2015, 10:11 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Software on Mac OS X, KDE Runtime and David Faure.


Repository: kde-runtime


Description
---

KDE on OS X does not handle the desktop session (no "Plasma") nor can it rely 
on XDG to obtain the proper paths to use for something like the trash. As a 
result, all applications that propose to move things they manage to the 
wastebin (Dolphin, but also digiKam) will store those items in a place that has 
no particular meaning on OS X, and that will thus tend to fill up.

OS X stores trash in one of several locations. Files trashed from the boot 
volume (and/or the volume containing $HOME, I don't actually know that) end up 
in `~/.Trash`. Files deleted from other volumes end up in 
`/Volumes/volName/.Trashes/uid`, where volName is the volume name (regardless 
whether it's an external or a remote drive; only mounted NFS shares are handled 
differently) and uid the numerical user id. Permissions on `.Trashes` are the 
same as those expected by KDE.

The kio_trash kioslave appears to support several actual trash directory 
locations, just like OS X. `TrashImpl::init()` creates a standard trash in 
`~/.local/share/Trash` (at least under OS X) but also 
`TrashImpl::trashForMountPoint()` that is used in cases I have not yet 
encountered.

On OS X, my modified `TrashImpl::init()` sets the standard trash directory to 
`~/.Trash/KDE.trash` and will create the `files` and `info` subdirectories as 
required, because they will of course be deleted when the user empties the OS X 
trash. `TrashImpl::fileRemoved()` has been modified to call a new function, 
`deleteEmptyTrashInfraStructure` to delete the KDE trash's internal 
infrastructure when the wastebin is empty so that OS X also sees the trash as 
emptied. (Since implementing `deleteEmptyTrashInfraStructure` this feature 
actually works, as expected as far as I can tell).

Remains to be done:
- determine in what cases `trashForMountPoint()` is used, and finish the 
modifications for it to use `/.Trashes/uid/KDE.trash`


Diffs
-

  kioslave/trash/trashimpl.cpp fe2e152 
  kioslave/trash/trashimpl.h bc68723 
  kioslave/trash/tests/CMakeLists.txt 9161fdf 
  kioslave/trash/kcmtrash.cpp f4811fd 
  kioslave/trash/CMakeLists.txt 3604089 

Diff: https://git.reviewboard.kde.org/r/120573/diff/


Testing
---

On OS X 10.6.8 with kdelibs and kde-runtime git/4.14, using Dolphin. Tested 
actions are
- move items to wastebin from $HOME and a directory on a different volume
- restore items to both places
- empty wastebin through Dolphin
- empty OS X trashcan


Thanks,

René J.V. Bertin

Review Request 125043: expose the WheelMouseZooms global setting through the input ("mouse") KCM


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

Review request for KDE Software on Mac OS X, kde-workspace and kdelibs.


Repository: kde-workspace


Description
---

KDE4 has been providing a setting that (would have) allowed to avoid unwanted 
text zooming during simulated inertial scrolling (scroll coasting). KDE PIM 
applications were immune to that issue because certain KDELibs classes use the 
parameter, which made it all the more annoying that other (e.g. Kate-based) 
applications weren't. Sadly this setting wasn't published via a GUI.

This patch adds a checkbox to the input ("mouse") KCM which seemed like the 
most appropriate place if not only because it also makes sense to provide this 
KCM on non-X11 platforms like OS X and MS Windows (where settings like "double 
or single click" are relevant).

I consider this a fix of an omission bug, but I realise that it could also be 
considered a new feature, so this RR is also intended to give some public 
exposure to my patch rather than keeping it to myself.


Diffs
-

  kcontrol/input/kmousedlg.ui b48a606 
  kcontrol/input/mouse.h d926a99 
  kcontrol/input/mouse.cpp cebb174 

Diff: https://git.reviewboard.kde.org/r/125043/diff/


Testing
---

For now only on OS X with kdelibs 4.14.11 .


Thanks,

René J.V. Bertin

Re: Review Request 125043: expose the WheelMouseZooms global setting through the input ("mouse") KCM


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



kcontrol/input/mouse.cpp (lines 183 - 185)


It is probably not required to use a dedicated change handler, correct?


- René J.V. Bertin


On Sept. 4, 2015, 1:51 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125043/
> ---
> 
> (Updated Sept. 4, 2015, 1:51 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, kde-workspace and kdelibs.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> KDE4 has been providing a setting that (would have) allowed to avoid unwanted 
> text zooming during simulated inertial scrolling (scroll coasting). KDE PIM 
> applications were immune to that issue because certain KDELibs classes use 
> the parameter, which made it all the more annoying that other (e.g. 
> Kate-based) applications weren't. Sadly this setting wasn't published via a 
> GUI.
> 
> This patch adds a checkbox to the input ("mouse") KCM which seemed like the 
> most appropriate place if not only because it also makes sense to provide 
> this KCM on non-X11 platforms like OS X and MS Windows (where settings like 
> "double or single click" are relevant).
> 
> I consider this a fix of an omission bug, but I realise that it could also be 
> considered a new feature, so this RR is also intended to give some public 
> exposure to my patch rather than keeping it to myself.
> 
> 
> Diffs
> -
> 
>   kcontrol/input/kmousedlg.ui b48a606 
>   kcontrol/input/mouse.h d926a99 
>   kcontrol/input/mouse.cpp cebb174 
> 
> Diff: https://git.reviewboard.kde.org/r/125043/diff/
> 
> 
> Testing
> ---
> 
> For now only on OS X with kdelibs 4.14.11 .
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

Re: Review Request 125043: expose the WheelMouseZooms global setting through the input ("mouse") KCM



> On Sept. 4, 2015, 3:21 p.m., Thomas Lübking wrote:
> > kcontrol/input/mouse.cpp, line 184
> > 
> >
> > read this line closely, then fix it ;-P

Oops :) 
But I guess that answers my question whether the change handler is required in 
the 1st place ... :)


> On Sept. 4, 2015, 3:21 p.m., Thomas Lübking wrote:
> > kcontrol/input/kmousedlg.ui, line 82
> > 
> >
> > You *must* ask the i18n team about this (requires an exception) on 
> > kde-...@kde.org
> > 
> > Since this will have no impact on QTextBrowser (Qt4 or 5) and maybe 
> > some other views in either direction, this limited impact needs to be 
> > somehow lined out (since to a user there's hardly a difference between 
> > QTextBrowser and KTextBrowser)
> 
> Luigi Toscano wrote:
> kde-workspace 4.14? It's still tracked on our i18n branches, but given 
> that the last release is out (see 
> https://mail.kde.org/pipermail/release-team/2015-August/008835.html), I think 
> we will stop tracking that branch soon. (PS: the list is 
> kde-i18n-...@kde.org). This is quite dead code at this point...

Well, the limited impact is hinted at by the use of the verb `may` ... Anyway, 
given the other reactions I'll only bother the i18n team once their opinion 
becomes the last obstacle to shipping!


- René J.V.


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


On Sept. 4, 2015, 4:22 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125043/
> ---
> 
> (Updated Sept. 4, 2015, 4:22 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, kde-workspace and kdelibs.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> KDE4 has been providing a setting that (would have) allowed to avoid unwanted 
> text zooming during simulated inertial scrolling (scroll coasting). KDE PIM 
> applications were immune to that issue because certain KDELibs classes use 
> the parameter, which made it all the more annoying that other (e.g. 
> Kate-based) applications weren't. Sadly this setting wasn't published via a 
> GUI.
> 
> This patch adds a checkbox to the input ("mouse") KCM which seemed like the 
> most appropriate place if not only because it also makes sense to provide 
> this KCM on non-X11 platforms like OS X and MS Windows (where settings like 
> "double or single click" are relevant).
> 
> I consider this a fix of an omission bug, but I realise that it could also be 
> considered a new feature, so this RR is also intended to give some public 
> exposure to my patch rather than keeping it to myself.
> 
> 
> Diffs
> -
> 
>   kcontrol/input/kmousedlg.ui b48a606 
>   kcontrol/input/mouse.h d926a99 
>   kcontrol/input/mouse.cpp cebb174 
> 
> Diff: https://git.reviewboard.kde.org/r/125043/diff/
> 
> 
> Testing
> ---
> 
> For now only on OS X with kdelibs 4.14.11 .
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

Re: Review Request 125043: expose the WheelMouseZooms global setting through the input ("mouse") KCM



> On Sept. 4, 2015, 3:38 p.m., Martin Gräßlin wrote:
> > You are aware that this is a dead repo and that this is a new feature for a 
> > repository that has been feature frozen for years?
> > 
> > Given that I think this should not and never be merged. If you want to keep 
> > the repo going for OSX I suggest to create a branch for your patches.

As I wrote in the summary, I don't consider this so much a new feature as a fix 
to an omission because the parameter is used in kdelibs (and possible elsewhere 
I don't know about). Besides, this concerns a KCM that I think should have been 
part of kde-runtime (but that's probably a moot point).

Also, this is not only about OS X. There are several distribution releases that 
ship KDE4 as the default desktop officially supported LTS version, and I'd hope 
they too would be interested in upstream fixes. As such I don't see the point 
in creating another branch, or in maintaining a freeze on a branch that isn't 
going to see any more releases
A separate repository with only fixes, organised by project and possibly target 
platform could make sense though.


- René J.V.


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


On Sept. 4, 2015, 4:22 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125043/
> ---
> 
> (Updated Sept. 4, 2015, 4:22 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, kde-workspace and kdelibs.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> KDE4 has been providing a setting that (would have) allowed to avoid unwanted 
> text zooming during simulated inertial scrolling (scroll coasting). KDE PIM 
> applications were immune to that issue because certain KDELibs classes use 
> the parameter, which made it all the more annoying that other (e.g. 
> Kate-based) applications weren't. Sadly this setting wasn't published via a 
> GUI.
> 
> This patch adds a checkbox to the input ("mouse") KCM which seemed like the 
> most appropriate place if not only because it also makes sense to provide 
> this KCM on non-X11 platforms like OS X and MS Windows (where settings like 
> "double or single click" are relevant).
> 
> I consider this a fix of an omission bug, but I realise that it could also be 
> considered a new feature, so this RR is also intended to give some public 
> exposure to my patch rather than keeping it to myself.
> 
> 
> Diffs
> -
> 
>   kcontrol/input/kmousedlg.ui b48a606 
>   kcontrol/input/mouse.h d926a99 
>   kcontrol/input/mouse.cpp cebb174 
> 
> Diff: https://git.reviewboard.kde.org/r/125043/diff/
> 
> 
> Testing
> ---
> 
> For now only on OS X with kdelibs 4.14.11 .
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

Re: Review Request 125043: expose the WheelMouseZooms global setting through the input ("mouse") KCM


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

(Updated Sept. 4, 2015, 4:22 p.m.)


Review request for KDE Software on Mac OS X, kde-workspace and kdelibs.


Repository: kde-workspace


Description
---

KDE4 has been providing a setting that (would have) allowed to avoid unwanted 
text zooming during simulated inertial scrolling (scroll coasting). KDE PIM 
applications were immune to that issue because certain KDELibs classes use the 
parameter, which made it all the more annoying that other (e.g. Kate-based) 
applications weren't. Sadly this setting wasn't published via a GUI.

This patch adds a checkbox to the input ("mouse") KCM which seemed like the 
most appropriate place if not only because it also makes sense to provide this 
KCM on non-X11 platforms like OS X and MS Windows (where settings like "double 
or single click" are relevant).

I consider this a fix of an omission bug, but I realise that it could also be 
considered a new feature, so this RR is also intended to give some public 
exposure to my patch rather than keeping it to myself.


Diffs (updated)
-

  kcontrol/input/kmousedlg.ui b48a606 
  kcontrol/input/mouse.h d926a99 
  kcontrol/input/mouse.cpp cebb174 

Diff: https://git.reviewboard.kde.org/r/125043/diff/


Testing
---

For now only on OS X with kdelibs 4.14.11 .


Thanks,

René J.V. Bertin

Re: Review Request 125043: expose the WheelMouseZooms global setting through the input ("mouse") KCM



> On Sept. 4, 2015, 3:38 p.m., Martin Gräßlin wrote:
> > You are aware that this is a dead repo and that this is a new feature for a 
> > repository that has been feature frozen for years?
> > 
> > Given that I think this should not and never be merged. If you want to keep 
> > the repo going for OSX I suggest to create a branch for your patches.
> 
> René J.V. Bertin wrote:
> As I wrote in the summary, I don't consider this so much a new feature as 
> a fix to an omission because the parameter is used in kdelibs (and possible 
> elsewhere I don't know about). Besides, this concerns a KCM that I think 
> should have been part of kde-runtime (but that's probably a moot point).
> 
> Also, this is not only about OS X. There are several distribution 
> releases that ship KDE4 as the default desktop officially supported LTS 
> version, and I'd hope they too would be interested in upstream fixes. As such 
> I don't see the point in creating another branch, or in maintaining a freeze 
> on a branch that isn't going to see any more releases
> A separate repository with only fixes, organised by project and possibly 
> target platform could make sense though.
> 
> Luigi Toscano wrote:
> I disagree: a separate branch makes definitely more sense than a separate 
> repository (which would lead more confusion and divide the code).

In case it wasn't clear: I meant a separate repository containing only 
patchfiles. The patch under consideration here is not specific to OS X so 
wouldn't justify the creation of an OS X branch (I just haven't gotten around 
to including it in my Kubuntu PPA yet).


- René J.V.


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


On Sept. 4, 2015, 4:22 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125043/
> ---
> 
> (Updated Sept. 4, 2015, 4:22 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, kde-workspace and kdelibs.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> KDE4 has been providing a setting that (would have) allowed to avoid unwanted 
> text zooming during simulated inertial scrolling (scroll coasting). KDE PIM 
> applications were immune to that issue because certain KDELibs classes use 
> the parameter, which made it all the more annoying that other (e.g. 
> Kate-based) applications weren't. Sadly this setting wasn't published via a 
> GUI.
> 
> This patch adds a checkbox to the input ("mouse") KCM which seemed like the 
> most appropriate place if not only because it also makes sense to provide 
> this KCM on non-X11 platforms like OS X and MS Windows (where settings like 
> "double or single click" are relevant).
> 
> I consider this a fix of an omission bug, but I realise that it could also be 
> considered a new feature, so this RR is also intended to give some public 
> exposure to my patch rather than keeping it to myself.
> 
> 
> Diffs
> -
> 
>   kcontrol/input/kmousedlg.ui b48a606 
>   kcontrol/input/mouse.h d926a99 
>   kcontrol/input/mouse.cpp cebb174 
> 
> Diff: https://git.reviewboard.kde.org/r/125043/diff/
> 
> 
> Testing
> ---
> 
> For now only on OS X with kdelibs 4.14.11 .
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

Re: Review Request 125043: expose the WheelMouseZooms global setting through the input ("mouse") KCM



> On Sept. 4, 2015, 3:38 p.m., Martin Gräßlin wrote:
> > You are aware that this is a dead repo and that this is a new feature for a 
> > repository that has been feature frozen for years?
> > 
> > Given that I think this should not and never be merged. If you want to keep 
> > the repo going for OSX I suggest to create a branch for your patches.
> 
> René J.V. Bertin wrote:
> As I wrote in the summary, I don't consider this so much a new feature as 
> a fix to an omission because the parameter is used in kdelibs (and possible 
> elsewhere I don't know about). Besides, this concerns a KCM that I think 
> should have been part of kde-runtime (but that's probably a moot point).
> 
> Also, this is not only about OS X. There are several distribution 
> releases that ship KDE4 as the default desktop officially supported LTS 
> version, and I'd hope they too would be interested in upstream fixes. As such 
> I don't see the point in creating another branch, or in maintaining a freeze 
> on a branch that isn't going to see any more releases
> A separate repository with only fixes, organised by project and possibly 
> target platform could make sense though.
> 
> Luigi Toscano wrote:
> I disagree: a separate branch makes definitely more sense than a separate 
> repository (which would lead more confusion and divide the code).
> 
> René J.V. Bertin wrote:
> In case it wasn't clear: I meant a separate repository containing only 
> patchfiles. The patch under consideration here is not specific to OS X so 
> wouldn't justify the creation of an OS X branch (I just haven't gotten around 
> to including it in my Kubuntu PPA yet).
> 
> Jeremy Whiting wrote:
> I think what Martin and Luigi are suggesting is a branch maybe called LTS 
> or something for feature improvements since master is frozen and has been for 
> quite a long time.
> 
> Luigi Toscano wrote:
> Exactly, a separate repository with patches does not make sense (git 
> already manages patches).

Git doesn't make it particularly easy to keep patches (or patch sets) separate 
once they're committed (and a couple other things have been committed too), or 
does it?


- René J.V.


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


On Sept. 4, 2015, 4:22 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125043/
> ---
> 
> (Updated Sept. 4, 2015, 4:22 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, kde-workspace and kdelibs.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> KDE4 has been providing a setting that (would have) allowed to avoid unwanted 
> text zooming during simulated inertial scrolling (scroll coasting). KDE PIM 
> applications were immune to that issue because certain KDELibs classes use 
> the parameter, which made it all the more annoying that other (e.g. 
> Kate-based) applications weren't. Sadly this setting wasn't published via a 
> GUI.
> 
> This patch adds a checkbox to the input ("mouse") KCM which seemed like the 
> most appropriate place if not only because it also makes sense to provide 
> this KCM on non-X11 platforms like OS X and MS Windows (where settings like 
> "double or single click" are relevant).
> 
> I consider this a fix of an omission bug, but I realise that it could also be 
> considered a new feature, so this RR is also intended to give some public 
> exposure to my patch rather than keeping it to myself.
> 
> 
> Diffs
> -
> 
>   kcontrol/input/kmousedlg.ui b48a606 
>   kcontrol/input/mouse.h d926a99 
>   kcontrol/input/mouse.cpp cebb174 
> 
> Diff: https://git.reviewboard.kde.org/r/125043/diff/
> 
> 
> Testing
> ---
> 
> For now only on OS X with kdelibs 4.14.11 .
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

Re: Review Request 125043: expose the WheelMouseZooms global setting through the input ("mouse") KCM



> On Sept. 4, 2015, 3:38 p.m., Martin Gräßlin wrote:
> > You are aware that this is a dead repo and that this is a new feature for a 
> > repository that has been feature frozen for years?
> > 
> > Given that I think this should not and never be merged. If you want to keep 
> > the repo going for OSX I suggest to create a branch for your patches.
> 
> René J.V. Bertin wrote:
> As I wrote in the summary, I don't consider this so much a new feature as 
> a fix to an omission because the parameter is used in kdelibs (and possible 
> elsewhere I don't know about). Besides, this concerns a KCM that I think 
> should have been part of kde-runtime (but that's probably a moot point).
> 
> Also, this is not only about OS X. There are several distribution 
> releases that ship KDE4 as the default desktop officially supported LTS 
> version, and I'd hope they too would be interested in upstream fixes. As such 
> I don't see the point in creating another branch, or in maintaining a freeze 
> on a branch that isn't going to see any more releases
> A separate repository with only fixes, organised by project and possibly 
> target platform could make sense though.
> 
> Luigi Toscano wrote:
> I disagree: a separate branch makes definitely more sense than a separate 
> repository (which would lead more confusion and divide the code).
> 
> René J.V. Bertin wrote:
> In case it wasn't clear: I meant a separate repository containing only 
> patchfiles. The patch under consideration here is not specific to OS X so 
> wouldn't justify the creation of an OS X branch (I just haven't gotten around 
> to including it in my Kubuntu PPA yet).
> 
> Jeremy Whiting wrote:
> I think what Martin and Luigi are suggesting is a branch maybe called LTS 
> or something for feature improvements since master is frozen and has been for 
> quite a long time.
> 
> Luigi Toscano wrote:
> Exactly, a separate repository with patches does not make sense (git 
> already manages patches).
> 
> René J.V. Bertin wrote:
> Git doesn't make it particularly easy to keep patches (or patch sets) 
> separate once they're committed (and a couple other things have been 
> committed too), or does it?
> 
> Martin Klapetek wrote:
> Sure it does. Here is a random git commit from plasma-workspace -- 
> https://quickgit.kde.org/?p=plasma-workspace.git=commit=e2df9af6e2df4ba1c07dc40d43ecd591584db498
>  - you can get the diff with "git show 
> e2df9af6e2df4ba1c07dc40d43ecd591584db498", you can get a diff from that 
> commit to HEAD by "git diff e2df9af6e2df4ba1c07dc40d43ecd591584db498" or diff 
> set between two commits - "git diff 
> e2df9af6e2df4ba1c07dc40d43ecd591584db498..6ee0d63af943526e2453631c6c709861061f08ac"

We're getting a bit side-tracked, but:
Suppose this "expose WheelMouseZooms" patch would get commited, followed by the 
patch to build kcm_input and other kcontrol components on OS X, and then some 
more patches that touch neighbouring lines, and then I do some polishing up on 
the WheelMouseZooms patch which also gets commited. Will it be easy for someone 
who just wants that patch to extract the WheelMouseZooms patch from git, or 
would it be easier to maintain that patch as a separate patchfile (like debian 
patches with dquilt)?
I know it isn't always straightforward to maintain patchfiles, but I'm not so 
sure that working with long, meaningless and unreadable commit hashes is easier 
...


- René J.V.


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


On Sept. 4, 2015, 4:22 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125043/
> ---
> 
> (Updated Sept. 4, 2015, 4:22 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, kde-workspace and kdelibs.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> ---
> 
> KDE4 has been providing a setting that (would have) allowed to avoid unwanted 
> text zooming during simulated inertial scrolling (scroll coasting). KDE PIM 
> applications were immune to that issue because certain KDELibs classes use 
> the parameter, which made it all the more annoying that other (e.g. 
> Kate-based) applications weren't. Sadly this setting wasn't published via a 
> GUI.
> 
> This patch adds a checkbox to the input ("mouse") KCM which seemed like the 
> most appropriate place if not only because it also makes sense to provide 
> this KCM on non-X11 platforms like OS X and MS Windows (where settings like 
> "double or single click" are relevant).
> 
> I consider this a fix of an omission bug, but I realise that it could also be 
> considered a new feature, so this RR is also intended to give some

Re: Review Request 120319: Make Kate mousewheel zoom feature respect the corresponding global setting (KGlobalSettings::wheelMouseZooms())



> On Sept. 23, 2014, 5:01 p.m., Thomas Lübking wrote:
> > Qt cannot "distiguish" because there's nothing to distinguish - the driver 
> > generates synthetic wheel event for the inertia.
> > You can btw. turn that  off.
> > 
> > Seems an issue with inertial scrolling on X11 as well 
> > https://bugs.freedesktop.org/show_bug.cgi?id=38909
> > Otherwise i'd have opted for "the driver shall please stop this when you 
> > hit a key".
> > 
> > On a random note, I can't find that setting in systemsettings?
> > If there's a config GUI for this, aligning to it seems reasonable, BUT does 
> > no way fix the actual issue w/ inertia (ie. "you don't have control over 
> > your input device")
> 
> René J.V. Bertin wrote:
> You may not believe it, but I actually prefer the UE with the feature on. 
> Probably because it saves me some movements, which is always good (less RSI).
> 
> Yes, I imagine that the issue can occur on Linux as well if inertial 
> scrolling works the same way there. It just never bit me on Linux - and yet I 
> run that on underpowered machines ...
> Might be a thought to define the modifier key to get wheelMouseZooms in 
> that case, or at least make that possible somewhere in systemsettings?
> 
> You're right, I haven't been able to find the setting in systemsettings. 
> The setting *is* part of the standard settings, though, no idea why it 
> slipped through and remained a hidden setting. But because it's part of the 
> standard settings I went with aligning to it, instead of hacking in a new 
> switch as in Konsole.
> NB, seems likely that Konsole offers its own switch because the authors 
> didn't go the length I did to find out about the one in `kdeglobalrc`?
> 
> Thomas Lübking wrote:
> UX isn't my concern - but doing that in the driver is simply idiotic.
> (Sorry, but hey - Peter H. shares that opinion ;-)
> What eg. happens when you reached the end of the document? The driver 
> keeps scrolling and you don't even notice that.
> 
> Either the view provides inertic scrolling or you purchase a rodant that 
> can alter between swing and precise wheel (w/o trying to advertise here, eg. 
> Logitech M500 is an affordable device with this capability)
> 
> Choosing another modifier won't help, but at best move the problem around 
> (ok, there's no META key on OSX, but it is indeed used for shortcuts on 
> linux/windows - often global ones. So instead of scaling the text, you end up 
> scaling the entire desktop ;-)
> 
> While I personally don't use text scaling in kate (this way), if there's 
> no present UI for the global config, it can hardly be used and we can't (?) 
> introduce config GUI for the kate part in SC4, I assume (kate devs will know 
> better)
> Also simply disabling a feature because it is *one* occasion of a driver 
> issue doesn't sound too reasonable - what if I'd in general like to scale the 
> text this way but am still annoyed by the driver behavior?
> 
> One way to deal with this is to measure the time between the last 
> unmodified wheel event and the now modified wheel event. If that is too low 
> to be reasonably caused by a human being (ie. the scroll "started" unmodified 
> and suddenly a modifier kicks in) one could ignore the modifier.
> That would however have to apply to all items.
> Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in 
> this regard?
> 
> René J.V. Bertin wrote:
> I use Apple's Magic Trackpad, no spamming intended, but I wouldn't want 
> to change for anything else anymore... 
> 
> > ok, there's no META key on OSX
> 
> There is. META is mapped to Ctrl by default on OS X (and CTRL to Command, 
> which is the real Meta key), but there is a setting to switch that back. I 
> presume the default is conceived to reflect the fact that OS X uses the 
> Command key in place of Ctrl for shortcuts. It apparently never occurred to 
> them to define a special code (`ACCEL`) for the standard accelerator "opcode" 
> ;)
> 
> > Also simply disabling a feature because it is one occasion of a driver 
> issue doesn't sound too reasonable 
> 
> How do you think I found the global setting and how to read it out? It's 
> used exactly this way in KTextEdit (see `KTextEdit::wheelEvent`) which again 
> raises the question why the setting isn't exposed in systemsettings.
> 
> > - what if I'd in general like to scale the text this way but am still 
> annoyed by the driver behavior?
> 
> The only solution in that case would be to configure the feature to use 
> another key or key combination, one that works for you and your workflow and 
> doesn't risk to get triggered unexpectedly. 
> 
> > Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in 
> this regard?
> 
> Not so stupid: Qt does exactly the same thing, and it doesn't appear to 
> be optional in any way there. This is probably why

Re: Review Request 120319: Make Kate mousewheel zoom feature respect the corresponding global setting (KGlobalSettings::wheelMouseZooms())



> On Sept. 23, 2014, 5:01 p.m., Thomas Lübking wrote:
> > Qt cannot "distiguish" because there's nothing to distinguish - the driver 
> > generates synthetic wheel event for the inertia.
> > You can btw. turn that  off.
> > 
> > Seems an issue with inertial scrolling on X11 as well 
> > https://bugs.freedesktop.org/show_bug.cgi?id=38909
> > Otherwise i'd have opted for "the driver shall please stop this when you 
> > hit a key".
> > 
> > On a random note, I can't find that setting in systemsettings?
> > If there's a config GUI for this, aligning to it seems reasonable, BUT does 
> > no way fix the actual issue w/ inertia (ie. "you don't have control over 
> > your input device")
> 
> René J.V. Bertin wrote:
> You may not believe it, but I actually prefer the UE with the feature on. 
> Probably because it saves me some movements, which is always good (less RSI).
> 
> Yes, I imagine that the issue can occur on Linux as well if inertial 
> scrolling works the same way there. It just never bit me on Linux - and yet I 
> run that on underpowered machines ...
> Might be a thought to define the modifier key to get wheelMouseZooms in 
> that case, or at least make that possible somewhere in systemsettings?
> 
> You're right, I haven't been able to find the setting in systemsettings. 
> The setting *is* part of the standard settings, though, no idea why it 
> slipped through and remained a hidden setting. But because it's part of the 
> standard settings I went with aligning to it, instead of hacking in a new 
> switch as in Konsole.
> NB, seems likely that Konsole offers its own switch because the authors 
> didn't go the length I did to find out about the one in `kdeglobalrc`?
> 
> Thomas Lübking wrote:
> UX isn't my concern - but doing that in the driver is simply idiotic.
> (Sorry, but hey - Peter H. shares that opinion ;-)
> What eg. happens when you reached the end of the document? The driver 
> keeps scrolling and you don't even notice that.
> 
> Either the view provides inertic scrolling or you purchase a rodant that 
> can alter between swing and precise wheel (w/o trying to advertise here, eg. 
> Logitech M500 is an affordable device with this capability)
> 
> Choosing another modifier won't help, but at best move the problem around 
> (ok, there's no META key on OSX, but it is indeed used for shortcuts on 
> linux/windows - often global ones. So instead of scaling the text, you end up 
> scaling the entire desktop ;-)
> 
> While I personally don't use text scaling in kate (this way), if there's 
> no present UI for the global config, it can hardly be used and we can't (?) 
> introduce config GUI for the kate part in SC4, I assume (kate devs will know 
> better)
> Also simply disabling a feature because it is *one* occasion of a driver 
> issue doesn't sound too reasonable - what if I'd in general like to scale the 
> text this way but am still annoyed by the driver behavior?
> 
> One way to deal with this is to measure the time between the last 
> unmodified wheel event and the now modified wheel event. If that is too low 
> to be reasonably caused by a human being (ie. the scroll "started" unmodified 
> and suddenly a modifier kicks in) one could ignore the modifier.
> That would however have to apply to all items.
> Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in 
> this regard?
> 
> René J.V. Bertin wrote:
> I use Apple's Magic Trackpad, no spamming intended, but I wouldn't want 
> to change for anything else anymore... 
> 
> > ok, there's no META key on OSX
> 
> There is. META is mapped to Ctrl by default on OS X (and CTRL to Command, 
> which is the real Meta key), but there is a setting to switch that back. I 
> presume the default is conceived to reflect the fact that OS X uses the 
> Command key in place of Ctrl for shortcuts. It apparently never occurred to 
> them to define a special code (`ACCEL`) for the standard accelerator "opcode" 
> ;)
> 
> > Also simply disabling a feature because it is one occasion of a driver 
> issue doesn't sound too reasonable 
> 
> How do you think I found the global setting and how to read it out? It's 
> used exactly this way in KTextEdit (see `KTextEdit::wheelEvent`) which again 
> raises the question why the setting isn't exposed in systemsettings.
> 
> > - what if I'd in general like to scale the text this way but am still 
> annoyed by the driver behavior?
> 
> The only solution in that case would be to configure the feature to use 
> another key or key combination, one that works for you and your workflow and 
> doesn't risk to get triggered unexpectedly. 
> 
> > Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in 
> this regard?
> 
> Not so stupid: Qt does exactly the same thing, and it doesn't appear to 
> be optional in any way there. This is probably why

Re: Review Request 120319: Make Kate mousewheel zoom feature respect the corresponding global setting (KGlobalSettings::wheelMouseZooms())

2015-09-03 Thread René J . V . Bertin

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

(Updated Sept. 3, 2015, 8:03 p.m.)

Review request for Kate, KDE Software on Mac OS X and kdelibs.

Changes
---

With a little help from Qt's Development ML I've come up with the attached
patch for Qt4 that should fix the issue on OS X. A simple fix really, but one
that uses lowlevel information to read the key modifier for the wheel event
only at the onset of the inertial movement.

Repository: kate

Description
---

Therefore, I present a small patch that checks
`KGlobalSettings::wheelMouseZooms()` when the platform's `ControlModifier` is
held and a wheel event received.

An alternative solution could introduce a Kate-specific setting (just like
Konsole has one), but that would require far more changes.

Diffs
-

part/view/kateviewinternal.cpp a2906f3

Diff: https://git.reviewboard.kde.org/r/120319/diff/

Testing
---

On OS X against kdelibs 4.14.1 (git/kde4). The change consists of an additional
call to a standard kdelibs function which I do not expect to introduce
regressions on any platform.

File Attachments (updated)

Qt4 patch to prevent unwanted/unexpected text zoom while scrolling

https://git.reviewboard.kde.org/media/uploaded/files/2015/09/03/ae8f63f3-4289-40bb-8b97-61a934bb33ec__patch-avoid-unwanted-fontzooming.diff

Thanks,

René J.V. Bertin

Re: Review Request 120319: Make Kate mousewheel zoom feature respect the corresponding global setting (KGlobalSettings::wheelMouseZooms())

2015-09-03 Thread René J . V . Bertin



> On Sept. 23, 2014, 5:01 p.m., Thomas Lübking wrote:
> > Qt cannot "distiguish" because there's nothing to distinguish - the driver 
> > generates synthetic wheel event for the inertia.
> > You can btw. turn that  off.
> > 
> > Seems an issue with inertial scrolling on X11 as well 
> > https://bugs.freedesktop.org/show_bug.cgi?id=38909
> > Otherwise i'd have opted for "the driver shall please stop this when you 
> > hit a key".
> > 
> > On a random note, I can't find that setting in systemsettings?
> > If there's a config GUI for this, aligning to it seems reasonable, BUT does 
> > no way fix the actual issue w/ inertia (ie. "you don't have control over 
> > your input device")
> 
> René J.V. Bertin wrote:
> You may not believe it, but I actually prefer the UE with the feature on. 
> Probably because it saves me some movements, which is always good (less RSI).
> 
> Yes, I imagine that the issue can occur on Linux as well if inertial 
> scrolling works the same way there. It just never bit me on Linux - and yet I 
> run that on underpowered machines ...
> Might be a thought to define the modifier key to get wheelMouseZooms in 
> that case, or at least make that possible somewhere in systemsettings?
> 
> You're right, I haven't been able to find the setting in systemsettings. 
> The setting *is* part of the standard settings, though, no idea why it 
> slipped through and remained a hidden setting. But because it's part of the 
> standard settings I went with aligning to it, instead of hacking in a new 
> switch as in Konsole.
> NB, seems likely that Konsole offers its own switch because the authors 
> didn't go the length I did to find out about the one in `kdeglobalrc`?
> 
> Thomas Lübking wrote:
> UX isn't my concern - but doing that in the driver is simply idiotic.
> (Sorry, but hey - Peter H. shares that opinion ;-)
> What eg. happens when you reached the end of the document? The driver 
> keeps scrolling and you don't even notice that.
> 
> Either the view provides inertic scrolling or you purchase a rodant that 
> can alter between swing and precise wheel (w/o trying to advertise here, eg. 
> Logitech M500 is an affordable device with this capability)
> 
> Choosing another modifier won't help, but at best move the problem around 
> (ok, there's no META key on OSX, but it is indeed used for shortcuts on 
> linux/windows - often global ones. So instead of scaling the text, you end up 
> scaling the entire desktop ;-)
> 
> While I personally don't use text scaling in kate (this way), if there's 
> no present UI for the global config, it can hardly be used and we can't (?) 
> introduce config GUI for the kate part in SC4, I assume (kate devs will know 
> better)
> Also simply disabling a feature because it is *one* occasion of a driver 
> issue doesn't sound too reasonable - what if I'd in general like to scale the 
> text this way but am still annoyed by the driver behavior?
> 
> One way to deal with this is to measure the time between the last 
> unmodified wheel event and the now modified wheel event. If that is too low 
> to be reasonably caused by a human being (ie. the scroll "started" unmodified 
> and suddenly a modifier kicks in) one could ignore the modifier.
> That would however have to apply to all items.
> Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in 
> this regard?
> 
> René J.V. Bertin wrote:
> I use Apple's Magic Trackpad, no spamming intended, but I wouldn't want 
> to change for anything else anymore... 
> 
> > ok, there's no META key on OSX
> 
> There is. META is mapped to Ctrl by default on OS X (and CTRL to Command, 
> which is the real Meta key), but there is a setting to switch that back. I 
> presume the default is conceived to reflect the fact that OS X uses the 
> Command key in place of Ctrl for shortcuts. It apparently never occurred to 
> them to define a special code (`ACCEL`) for the standard accelerator "opcode" 
> ;)
> 
> > Also simply disabling a feature because it is one occasion of a driver 
> issue doesn't sound too reasonable 
> 
> How do you think I found the global setting and how to read it out? It's 
> used exactly this way in KTextEdit (see `KTextEdit::wheelEvent`) which again 
> raises the question why the setting isn't exposed in systemsettings.
> 
> > - what if I'd in general like to scale the text this way but am still 
> annoyed by the driver behavior?
> 
> The only solution in that case would be to configure the feature to use 
> another key or key combination, one that works for you and your workflow and 
> doesn't risk to get triggered unexpectedly. 
> 
> > Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in 
> this regard?
> 
> Not so stupid: Qt does exactly the same thing, and it doesn't appear to 
> be optional in any way there. This is probably why

Re: Review Request 120319: Make Kate mousewheel zoom feature respect the corresponding global setting (KGlobalSettings::wheelMouseZooms())

2015-09-03 Thread René J . V . Bertin



> On Sept. 23, 2014, 5:01 p.m., Thomas Lübking wrote:
> > Qt cannot "distiguish" because there's nothing to distinguish - the driver 
> > generates synthetic wheel event for the inertia.
> > You can btw. turn that  off.
> > 
> > Seems an issue with inertial scrolling on X11 as well 
> > https://bugs.freedesktop.org/show_bug.cgi?id=38909
> > Otherwise i'd have opted for "the driver shall please stop this when you 
> > hit a key".
> > 
> > On a random note, I can't find that setting in systemsettings?
> > If there's a config GUI for this, aligning to it seems reasonable, BUT does 
> > no way fix the actual issue w/ inertia (ie. "you don't have control over 
> > your input device")
> 
> René J.V. Bertin wrote:
> You may not believe it, but I actually prefer the UE with the feature on. 
> Probably because it saves me some movements, which is always good (less RSI).
> 
> Yes, I imagine that the issue can occur on Linux as well if inertial 
> scrolling works the same way there. It just never bit me on Linux - and yet I 
> run that on underpowered machines ...
> Might be a thought to define the modifier key to get wheelMouseZooms in 
> that case, or at least make that possible somewhere in systemsettings?
> 
> You're right, I haven't been able to find the setting in systemsettings. 
> The setting *is* part of the standard settings, though, no idea why it 
> slipped through and remained a hidden setting. But because it's part of the 
> standard settings I went with aligning to it, instead of hacking in a new 
> switch as in Konsole.
> NB, seems likely that Konsole offers its own switch because the authors 
> didn't go the length I did to find out about the one in `kdeglobalrc`?
> 
> Thomas Lübking wrote:
> UX isn't my concern - but doing that in the driver is simply idiotic.
> (Sorry, but hey - Peter H. shares that opinion ;-)
> What eg. happens when you reached the end of the document? The driver 
> keeps scrolling and you don't even notice that.
> 
> Either the view provides inertic scrolling or you purchase a rodant that 
> can alter between swing and precise wheel (w/o trying to advertise here, eg. 
> Logitech M500 is an affordable device with this capability)
> 
> Choosing another modifier won't help, but at best move the problem around 
> (ok, there's no META key on OSX, but it is indeed used for shortcuts on 
> linux/windows - often global ones. So instead of scaling the text, you end up 
> scaling the entire desktop ;-)
> 
> While I personally don't use text scaling in kate (this way), if there's 
> no present UI for the global config, it can hardly be used and we can't (?) 
> introduce config GUI for the kate part in SC4, I assume (kate devs will know 
> better)
> Also simply disabling a feature because it is *one* occasion of a driver 
> issue doesn't sound too reasonable - what if I'd in general like to scale the 
> text this way but am still annoyed by the driver behavior?
> 
> One way to deal with this is to measure the time between the last 
> unmodified wheel event and the now modified wheel event. If that is too low 
> to be reasonably caused by a human being (ie. the scroll "started" unmodified 
> and suddenly a modifier kicks in) one could ignore the modifier.
> That would however have to apply to all items.
> Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in 
> this regard?
> 
> René J.V. Bertin wrote:
> I use Apple's Magic Trackpad, no spamming intended, but I wouldn't want 
> to change for anything else anymore... 
> 
> > ok, there's no META key on OSX
> 
> There is. META is mapped to Ctrl by default on OS X (and CTRL to Command, 
> which is the real Meta key), but there is a setting to switch that back. I 
> presume the default is conceived to reflect the fact that OS X uses the 
> Command key in place of Ctrl for shortcuts. It apparently never occurred to 
> them to define a special code (`ACCEL`) for the standard accelerator "opcode" 
> ;)
> 
> > Also simply disabling a feature because it is one occasion of a driver 
> issue doesn't sound too reasonable 
> 
> How do you think I found the global setting and how to read it out? It's 
> used exactly this way in KTextEdit (see `KTextEdit::wheelEvent`) which again 
> raises the question why the setting isn't exposed in systemsettings.
> 
> > - what if I'd in general like to scale the text this way but am still 
> annoyed by the driver behavior?
> 
> The only solution in that case would be to configure the feature to use 
> another key or key combination, one that works for you and your workflow and 
> doesn't risk to get triggered unexpectedly. 
> 
> > Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in 
> this regard?
> 
> Not so stupid: Qt does exactly the same thing, and it doesn't appear to 
> be optional in any way there. This is probably why

Re: Review Request 120319: Make Kate mousewheel zoom feature respect the corresponding global setting (KGlobalSettings::wheelMouseZooms())



> On Sept. 23, 2014, 5:01 p.m., Thomas Lübking wrote:
> > Qt cannot "distiguish" because there's nothing to distinguish - the driver 
> > generates synthetic wheel event for the inertia.
> > You can btw. turn that  off.
> > 
> > Seems an issue with inertial scrolling on X11 as well 
> > https://bugs.freedesktop.org/show_bug.cgi?id=38909
> > Otherwise i'd have opted for "the driver shall please stop this when you 
> > hit a key".
> > 
> > On a random note, I can't find that setting in systemsettings?
> > If there's a config GUI for this, aligning to it seems reasonable, BUT does 
> > no way fix the actual issue w/ inertia (ie. "you don't have control over 
> > your input device")
> 
> René J.V. Bertin wrote:
> You may not believe it, but I actually prefer the UE with the feature on. 
> Probably because it saves me some movements, which is always good (less RSI).
> 
> Yes, I imagine that the issue can occur on Linux as well if inertial 
> scrolling works the same way there. It just never bit me on Linux - and yet I 
> run that on underpowered machines ...
> Might be a thought to define the modifier key to get wheelMouseZooms in 
> that case, or at least make that possible somewhere in systemsettings?
> 
> You're right, I haven't been able to find the setting in systemsettings. 
> The setting *is* part of the standard settings, though, no idea why it 
> slipped through and remained a hidden setting. But because it's part of the 
> standard settings I went with aligning to it, instead of hacking in a new 
> switch as in Konsole.
> NB, seems likely that Konsole offers its own switch because the authors 
> didn't go the length I did to find out about the one in `kdeglobalrc`?
> 
> Thomas Lübking wrote:
> UX isn't my concern - but doing that in the driver is simply idiotic.
> (Sorry, but hey - Peter H. shares that opinion ;-)
> What eg. happens when you reached the end of the document? The driver 
> keeps scrolling and you don't even notice that.
> 
> Either the view provides inertic scrolling or you purchase a rodant that 
> can alter between swing and precise wheel (w/o trying to advertise here, eg. 
> Logitech M500 is an affordable device with this capability)
> 
> Choosing another modifier won't help, but at best move the problem around 
> (ok, there's no META key on OSX, but it is indeed used for shortcuts on 
> linux/windows - often global ones. So instead of scaling the text, you end up 
> scaling the entire desktop ;-)
> 
> While I personally don't use text scaling in kate (this way), if there's 
> no present UI for the global config, it can hardly be used and we can't (?) 
> introduce config GUI for the kate part in SC4, I assume (kate devs will know 
> better)
> Also simply disabling a feature because it is *one* occasion of a driver 
> issue doesn't sound too reasonable - what if I'd in general like to scale the 
> text this way but am still annoyed by the driver behavior?
> 
> One way to deal with this is to measure the time between the last 
> unmodified wheel event and the now modified wheel event. If that is too low 
> to be reasonably caused by a human being (ie. the scroll "started" unmodified 
> and suddenly a modifier kicks in) one could ignore the modifier.
> That would however have to apply to all items.
> Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in 
> this regard?
> 
> René J.V. Bertin wrote:
> I use Apple's Magic Trackpad, no spamming intended, but I wouldn't want 
> to change for anything else anymore... 
> 
> > ok, there's no META key on OSX
> 
> There is. META is mapped to Ctrl by default on OS X (and CTRL to Command, 
> which is the real Meta key), but there is a setting to switch that back. I 
> presume the default is conceived to reflect the fact that OS X uses the 
> Command key in place of Ctrl for shortcuts. It apparently never occurred to 
> them to define a special code (`ACCEL`) for the standard accelerator "opcode" 
> ;)
> 
> > Also simply disabling a feature because it is one occasion of a driver 
> issue doesn't sound too reasonable 
> 
> How do you think I found the global setting and how to read it out? It's 
> used exactly this way in KTextEdit (see `KTextEdit::wheelEvent`) which again 
> raises the question why the setting isn't exposed in systemsettings.
> 
> > - what if I'd in general like to scale the text this way but am still 
> annoyed by the driver behavior?
> 
> The only solution in that case would be to configure the feature to use 
> another key or key combination, one that works for you and your workflow and 
> doesn't risk to get triggered unexpectedly. 
> 
> > Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in 
> this regard?
> 
> Not so stupid: Qt does exactly the same thing, and it doesn't appear to 
> be optional in any way there. This is probably why

Re: Review Request 120319: Make Kate mousewheel zoom feature respect the corresponding global setting (KGlobalSettings::wheelMouseZooms())



> On Sept. 23, 2014, 5:01 p.m., Thomas Lübking wrote:
> > Qt cannot "distiguish" because there's nothing to distinguish - the driver 
> > generates synthetic wheel event for the inertia.
> > You can btw. turn that  off.
> > 
> > Seems an issue with inertial scrolling on X11 as well 
> > https://bugs.freedesktop.org/show_bug.cgi?id=38909
> > Otherwise i'd have opted for "the driver shall please stop this when you 
> > hit a key".
> > 
> > On a random note, I can't find that setting in systemsettings?
> > If there's a config GUI for this, aligning to it seems reasonable, BUT does 
> > no way fix the actual issue w/ inertia (ie. "you don't have control over 
> > your input device")
> 
> René J.V. Bertin wrote:
> You may not believe it, but I actually prefer the UE with the feature on. 
> Probably because it saves me some movements, which is always good (less RSI).
> 
> Yes, I imagine that the issue can occur on Linux as well if inertial 
> scrolling works the same way there. It just never bit me on Linux - and yet I 
> run that on underpowered machines ...
> Might be a thought to define the modifier key to get wheelMouseZooms in 
> that case, or at least make that possible somewhere in systemsettings?
> 
> You're right, I haven't been able to find the setting in systemsettings. 
> The setting *is* part of the standard settings, though, no idea why it 
> slipped through and remained a hidden setting. But because it's part of the 
> standard settings I went with aligning to it, instead of hacking in a new 
> switch as in Konsole.
> NB, seems likely that Konsole offers its own switch because the authors 
> didn't go the length I did to find out about the one in `kdeglobalrc`?
> 
> Thomas Lübking wrote:
> UX isn't my concern - but doing that in the driver is simply idiotic.
> (Sorry, but hey - Peter H. shares that opinion ;-)
> What eg. happens when you reached the end of the document? The driver 
> keeps scrolling and you don't even notice that.
> 
> Either the view provides inertic scrolling or you purchase a rodant that 
> can alter between swing and precise wheel (w/o trying to advertise here, eg. 
> Logitech M500 is an affordable device with this capability)
> 
> Choosing another modifier won't help, but at best move the problem around 
> (ok, there's no META key on OSX, but it is indeed used for shortcuts on 
> linux/windows - often global ones. So instead of scaling the text, you end up 
> scaling the entire desktop ;-)
> 
> While I personally don't use text scaling in kate (this way), if there's 
> no present UI for the global config, it can hardly be used and we can't (?) 
> introduce config GUI for the kate part in SC4, I assume (kate devs will know 
> better)
> Also simply disabling a feature because it is *one* occasion of a driver 
> issue doesn't sound too reasonable - what if I'd in general like to scale the 
> text this way but am still annoyed by the driver behavior?
> 
> One way to deal with this is to measure the time between the last 
> unmodified wheel event and the now modified wheel event. If that is too low 
> to be reasonably caused by a human being (ie. the scroll "started" unmodified 
> and suddenly a modifier kicks in) one could ignore the modifier.
> That would however have to apply to all items.
> Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in 
> this regard?
> 
> René J.V. Bertin wrote:
> I use Apple's Magic Trackpad, no spamming intended, but I wouldn't want 
> to change for anything else anymore... 
> 
> > ok, there's no META key on OSX
> 
> There is. META is mapped to Ctrl by default on OS X (and CTRL to Command, 
> which is the real Meta key), but there is a setting to switch that back. I 
> presume the default is conceived to reflect the fact that OS X uses the 
> Command key in place of Ctrl for shortcuts. It apparently never occurred to 
> them to define a special code (`ACCEL`) for the standard accelerator "opcode" 
> ;)
> 
> > Also simply disabling a feature because it is one occasion of a driver 
> issue doesn't sound too reasonable 
> 
> How do you think I found the global setting and how to read it out? It's 
> used exactly this way in KTextEdit (see `KTextEdit::wheelEvent`) which again 
> raises the question why the setting isn't exposed in systemsettings.
> 
> > - what if I'd in general like to scale the text this way but am still 
> annoyed by the driver behavior?
> 
> The only solution in that case would be to configure the feature to use 
> another key or key combination, one that works for you and your workflow and 
> doesn't risk to get triggered unexpectedly. 
> 
> > Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in 
> this regard?
> 
> Not so stupid: Qt does exactly the same thing, and it doesn't appear to 
> be optional in any way there. This is probably why

Re: Review Request 120319: Make Kate mousewheel zoom feature respect the corresponding global setting (KGlobalSettings::wheelMouseZooms())



> On Sept. 23, 2014, 5:01 p.m., Thomas Lübking wrote:
> > Qt cannot "distiguish" because there's nothing to distinguish - the driver 
> > generates synthetic wheel event for the inertia.
> > You can btw. turn that  off.
> > 
> > Seems an issue with inertial scrolling on X11 as well 
> > https://bugs.freedesktop.org/show_bug.cgi?id=38909
> > Otherwise i'd have opted for "the driver shall please stop this when you 
> > hit a key".
> > 
> > On a random note, I can't find that setting in systemsettings?
> > If there's a config GUI for this, aligning to it seems reasonable, BUT does 
> > no way fix the actual issue w/ inertia (ie. "you don't have control over 
> > your input device")
> 
> René J.V. Bertin wrote:
> You may not believe it, but I actually prefer the UE with the feature on. 
> Probably because it saves me some movements, which is always good (less RSI).
> 
> Yes, I imagine that the issue can occur on Linux as well if inertial 
> scrolling works the same way there. It just never bit me on Linux - and yet I 
> run that on underpowered machines ...
> Might be a thought to define the modifier key to get wheelMouseZooms in 
> that case, or at least make that possible somewhere in systemsettings?
> 
> You're right, I haven't been able to find the setting in systemsettings. 
> The setting *is* part of the standard settings, though, no idea why it 
> slipped through and remained a hidden setting. But because it's part of the 
> standard settings I went with aligning to it, instead of hacking in a new 
> switch as in Konsole.
> NB, seems likely that Konsole offers its own switch because the authors 
> didn't go the length I did to find out about the one in `kdeglobalrc`?
> 
> Thomas Lübking wrote:
> UX isn't my concern - but doing that in the driver is simply idiotic.
> (Sorry, but hey - Peter H. shares that opinion ;-)
> What eg. happens when you reached the end of the document? The driver 
> keeps scrolling and you don't even notice that.
> 
> Either the view provides inertic scrolling or you purchase a rodant that 
> can alter between swing and precise wheel (w/o trying to advertise here, eg. 
> Logitech M500 is an affordable device with this capability)
> 
> Choosing another modifier won't help, but at best move the problem around 
> (ok, there's no META key on OSX, but it is indeed used for shortcuts on 
> linux/windows - often global ones. So instead of scaling the text, you end up 
> scaling the entire desktop ;-)
> 
> While I personally don't use text scaling in kate (this way), if there's 
> no present UI for the global config, it can hardly be used and we can't (?) 
> introduce config GUI for the kate part in SC4, I assume (kate devs will know 
> better)
> Also simply disabling a feature because it is *one* occasion of a driver 
> issue doesn't sound too reasonable - what if I'd in general like to scale the 
> text this way but am still annoyed by the driver behavior?
> 
> One way to deal with this is to measure the time between the last 
> unmodified wheel event and the now modified wheel event. If that is too low 
> to be reasonably caused by a human being (ie. the scroll "started" unmodified 
> and suddenly a modifier kicks in) one could ignore the modifier.
> That would however have to apply to all items.
> Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in 
> this regard?
> 
> René J.V. Bertin wrote:
> I use Apple's Magic Trackpad, no spamming intended, but I wouldn't want 
> to change for anything else anymore... 
> 
> > ok, there's no META key on OSX
> 
> There is. META is mapped to Ctrl by default on OS X (and CTRL to Command, 
> which is the real Meta key), but there is a setting to switch that back. I 
> presume the default is conceived to reflect the fact that OS X uses the 
> Command key in place of Ctrl for shortcuts. It apparently never occurred to 
> them to define a special code (`ACCEL`) for the standard accelerator "opcode" 
> ;)
> 
> > Also simply disabling a feature because it is one occasion of a driver 
> issue doesn't sound too reasonable 
> 
> How do you think I found the global setting and how to read it out? It's 
> used exactly this way in KTextEdit (see `KTextEdit::wheelEvent`) which again 
> raises the question why the setting isn't exposed in systemsettings.
> 
> > - what if I'd in general like to scale the text this way but am still 
> annoyed by the driver behavior?
> 
> The only solution in that case would be to configure the feature to use 
> another key or key combination, one that works for you and your workflow and 
> doesn't risk to get triggered unexpectedly. 
> 
> > Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in 
> this regard?
> 
> Not so stupid: Qt does exactly the same thing, and it doesn't appear to 
> be optional in any way there. This is probably why

Re: Review Request 120319: Make Kate mousewheel zoom feature respect the corresponding global setting (KGlobalSettings::wheelMouseZooms())



> On Sept. 2, 2015, 3:06 p.m., Dominik Haumann wrote:
> > Since this is a mac issue, what about this:
> > 
> > if (e->modifiers() == Qt::ControlModifier
> > #ifdef Q_OS_MAC
> > && KGlobalSettings::wheelMouseZooms()
> > #endif
> > ) { // ...
> > 
> > That way we at least do not touch the current 4.x code for Linux (which 
> > works fine, as I understand), and the workaraound is ok for os X...
> > 
> > Would that be ok?

that would be an option, but if you understood that Kate on Linux (X11/xcb) is 
not affected by this issue, you misunderstood something. If I do not run into 
it there that's mostly because of different scrolling settings, trackpad 
characteristics and above all, the fact that I rarely use Kate or KDevelop 
there.


- René J.V.


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


On Sept. 22, 2014, 5:16 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120319/
> ---
> 
> (Updated Sept. 22, 2014, 5:16 p.m.)
> 
> 
> Review request for Kate, KDE Software on Mac OS X and kdelibs.
> 
> 
> Repository: kate
> 
> 
> Description
> ---
> 
> KDE has a global text editor option that can be used to let Ctrl-MouseWheel 
> events zoom the text font being used. Kate does not respect this setting, 
> which is an omission that can lead to unexpected behaviour.
> 
> On OS X, the feature works slightly differently in the sense that 
> `Qt::ControlModifier` does not designate the control key, but the command (?, 
> Apple) key. In addition, Qt's event handling does not appear to be able to 
> distinguish between scrolling under direct control, and residual scroll 
> movement that's due to simulated inertia. As a result, any attempt to use a 
> keyboard shortcut while a text view has not stopped moving completely will 
> lead to text zooming.
> See https://bugreports.qt-project.org/browse/QTBUG-41475 .
> 
> At first I thought to replace `Qt::ControlModifier` with `Qt::MetaModifier` 
> on OS X but that would probably require changes in many locations, and thus 
> best be preceded by a design decision if the standard shortcut modifier key 
> ought not be referenced via a symbolic platform constant not named after a 
> specific key, instead of being hardcoded (and using a key name).
> 
> Therefore, I present a small patch that checks 
> `KGlobalSettings::wheelMouseZooms()` when the platform's `ControlModifier` is 
> held and a wheel event received.
> 
> An alternative solution could introduce a Kate-specific setting (just like 
> Konsole has one), but that would require far more changes.
> 
> 
> Diffs
> -
> 
>   part/view/kateviewinternal.cpp a2906f3 
> 
> Diff: https://git.reviewboard.kde.org/r/120319/diff/
> 
> 
> Testing
> ---
> 
> On OS X against kdelibs 4.14.1 (git/kde4). The change consists of an 
> additional call to a standard kdelibs function which I do not expect to 
> introduce regressions on any platform.
> 
> I looked at kate's git/master code, which lacks a `wheelEvent` handler 
> suggesting the feature works differently there. However, Qt 5.3.1 
> applications (like Digia's own Qt Creator) still suffer from the phenomenon 
> described above, so a fix would be beneficial for KF5 too)
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

Re: Review Request 120319: Make Kate mousewheel zoom feature respect the corresponding global setting (KGlobalSettings::wheelMouseZooms())

2015-09-01 Thread René J . V . Bertin



> On Sept. 23, 2014, 5:01 p.m., Thomas Lübking wrote:
> > Qt cannot "distiguish" because there's nothing to distinguish - the driver 
> > generates synthetic wheel event for the inertia.
> > You can btw. turn that  off.
> > 
> > Seems an issue with inertial scrolling on X11 as well 
> > https://bugs.freedesktop.org/show_bug.cgi?id=38909
> > Otherwise i'd have opted for "the driver shall please stop this when you 
> > hit a key".
> > 
> > On a random note, I can't find that setting in systemsettings?
> > If there's a config GUI for this, aligning to it seems reasonable, BUT does 
> > no way fix the actual issue w/ inertia (ie. "you don't have control over 
> > your input device")
> 
> René J.V. Bertin wrote:
> You may not believe it, but I actually prefer the UE with the feature on. 
> Probably because it saves me some movements, which is always good (less RSI).
> 
> Yes, I imagine that the issue can occur on Linux as well if inertial 
> scrolling works the same way there. It just never bit me on Linux - and yet I 
> run that on underpowered machines ...
> Might be a thought to define the modifier key to get wheelMouseZooms in 
> that case, or at least make that possible somewhere in systemsettings?
> 
> You're right, I haven't been able to find the setting in systemsettings. 
> The setting *is* part of the standard settings, though, no idea why it 
> slipped through and remained a hidden setting. But because it's part of the 
> standard settings I went with aligning to it, instead of hacking in a new 
> switch as in Konsole.
> NB, seems likely that Konsole offers its own switch because the authors 
> didn't go the length I did to find out about the one in `kdeglobalrc`?
> 
> Thomas Lübking wrote:
> UX isn't my concern - but doing that in the driver is simply idiotic.
> (Sorry, but hey - Peter H. shares that opinion ;-)
> What eg. happens when you reached the end of the document? The driver 
> keeps scrolling and you don't even notice that.
> 
> Either the view provides inertic scrolling or you purchase a rodant that 
> can alter between swing and precise wheel (w/o trying to advertise here, eg. 
> Logitech M500 is an affordable device with this capability)
> 
> Choosing another modifier won't help, but at best move the problem around 
> (ok, there's no META key on OSX, but it is indeed used for shortcuts on 
> linux/windows - often global ones. So instead of scaling the text, you end up 
> scaling the entire desktop ;-)
> 
> While I personally don't use text scaling in kate (this way), if there's 
> no present UI for the global config, it can hardly be used and we can't (?) 
> introduce config GUI for the kate part in SC4, I assume (kate devs will know 
> better)
> Also simply disabling a feature because it is *one* occasion of a driver 
> issue doesn't sound too reasonable - what if I'd in general like to scale the 
> text this way but am still annoyed by the driver behavior?
> 
> One way to deal with this is to measure the time between the last 
> unmodified wheel event and the now modified wheel event. If that is too low 
> to be reasonably caused by a human being (ie. the scroll "started" unmodified 
> and suddenly a modifier kicks in) one could ignore the modifier.
> That would however have to apply to all items.
> Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in 
> this regard?
> 
> René J.V. Bertin wrote:
> I use Apple's Magic Trackpad, no spamming intended, but I wouldn't want 
> to change for anything else anymore... 
> 
> > ok, there's no META key on OSX
> 
> There is. META is mapped to Ctrl by default on OS X (and CTRL to Command, 
> which is the real Meta key), but there is a setting to switch that back. I 
> presume the default is conceived to reflect the fact that OS X uses the 
> Command key in place of Ctrl for shortcuts. It apparently never occurred to 
> them to define a special code (`ACCEL`) for the standard accelerator "opcode" 
> ;)
> 
> > Also simply disabling a feature because it is one occasion of a driver 
> issue doesn't sound too reasonable 
> 
> How do you think I found the global setting and how to read it out? It's 
> used exactly this way in KTextEdit (see `KTextEdit::wheelEvent`) which again 
> raises the question why the setting isn't exposed in systemsettings.
> 
> > - what if I'd in general like to scale the text this way but am still 
> annoyed by the driver behavior?
> 
> The only solution in that case would be to configure the feature to use 
> another key or key combination, one that works for you and your workflow and 
> doesn't risk to get triggered unexpectedly. 
> 
> > Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in 
> this regard?
> 
> Not so stupid: Qt does exactly the same thing, and it doesn't appear to 
> be optional in any way there. This is probably why

Re: Review Request 120319: Make Kate mousewheel zoom feature respect the corresponding global setting (KGlobalSettings::wheelMouseZooms())

2015-09-01 Thread René J . V . Bertin



> On Sept. 23, 2014, 5:01 p.m., Thomas Lübking wrote:
> > Qt cannot "distiguish" because there's nothing to distinguish - the driver 
> > generates synthetic wheel event for the inertia.
> > You can btw. turn that  off.
> > 
> > Seems an issue with inertial scrolling on X11 as well 
> > https://bugs.freedesktop.org/show_bug.cgi?id=38909
> > Otherwise i'd have opted for "the driver shall please stop this when you 
> > hit a key".
> > 
> > On a random note, I can't find that setting in systemsettings?
> > If there's a config GUI for this, aligning to it seems reasonable, BUT does 
> > no way fix the actual issue w/ inertia (ie. "you don't have control over 
> > your input device")
> 
> René J.V. Bertin wrote:
> You may not believe it, but I actually prefer the UE with the feature on. 
> Probably because it saves me some movements, which is always good (less RSI).
> 
> Yes, I imagine that the issue can occur on Linux as well if inertial 
> scrolling works the same way there. It just never bit me on Linux - and yet I 
> run that on underpowered machines ...
> Might be a thought to define the modifier key to get wheelMouseZooms in 
> that case, or at least make that possible somewhere in systemsettings?
> 
> You're right, I haven't been able to find the setting in systemsettings. 
> The setting *is* part of the standard settings, though, no idea why it 
> slipped through and remained a hidden setting. But because it's part of the 
> standard settings I went with aligning to it, instead of hacking in a new 
> switch as in Konsole.
> NB, seems likely that Konsole offers its own switch because the authors 
> didn't go the length I did to find out about the one in `kdeglobalrc`?
> 
> Thomas Lübking wrote:
> UX isn't my concern - but doing that in the driver is simply idiotic.
> (Sorry, but hey - Peter H. shares that opinion ;-)
> What eg. happens when you reached the end of the document? The driver 
> keeps scrolling and you don't even notice that.
> 
> Either the view provides inertic scrolling or you purchase a rodant that 
> can alter between swing and precise wheel (w/o trying to advertise here, eg. 
> Logitech M500 is an affordable device with this capability)
> 
> Choosing another modifier won't help, but at best move the problem around 
> (ok, there's no META key on OSX, but it is indeed used for shortcuts on 
> linux/windows - often global ones. So instead of scaling the text, you end up 
> scaling the entire desktop ;-)
> 
> While I personally don't use text scaling in kate (this way), if there's 
> no present UI for the global config, it can hardly be used and we can't (?) 
> introduce config GUI for the kate part in SC4, I assume (kate devs will know 
> better)
> Also simply disabling a feature because it is *one* occasion of a driver 
> issue doesn't sound too reasonable - what if I'd in general like to scale the 
> text this way but am still annoyed by the driver behavior?
> 
> One way to deal with this is to measure the time between the last 
> unmodified wheel event and the now modified wheel event. If that is too low 
> to be reasonably caused by a human being (ie. the scroll "started" unmodified 
> and suddenly a modifier kicks in) one could ignore the modifier.
> That would however have to apply to all items.
> Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in 
> this regard?
> 
> René J.V. Bertin wrote:
> I use Apple's Magic Trackpad, no spamming intended, but I wouldn't want 
> to change for anything else anymore... 
> 
> > ok, there's no META key on OSX
> 
> There is. META is mapped to Ctrl by default on OS X (and CTRL to Command, 
> which is the real Meta key), but there is a setting to switch that back. I 
> presume the default is conceived to reflect the fact that OS X uses the 
> Command key in place of Ctrl for shortcuts. It apparently never occurred to 
> them to define a special code (`ACCEL`) for the standard accelerator "opcode" 
> ;)
> 
> > Also simply disabling a feature because it is one occasion of a driver 
> issue doesn't sound too reasonable 
> 
> How do you think I found the global setting and how to read it out? It's 
> used exactly this way in KTextEdit (see `KTextEdit::wheelEvent`) which again 
> raises the question why the setting isn't exposed in systemsettings.
> 
> > - what if I'd in general like to scale the text this way but am still 
> annoyed by the driver behavior?
> 
> The only solution in that case would be to configure the feature to use 
> another key or key combination, one that works for you and your workflow and 
> doesn't risk to get triggered unexpectedly. 
> 
> > Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in 
> this regard?
> 
> Not so stupid: Qt does exactly the same thing, and it doesn't appear to 
> be optional in any way there. This is probably why

Re: Review Request 120319: Make Kate mousewheel zoom feature respect the corresponding global setting (KGlobalSettings::wheelMouseZooms())

2015-09-01 Thread René J . V . Bertin



> On Sept. 23, 2014, 5:01 p.m., Thomas Lübking wrote:
> > Qt cannot "distiguish" because there's nothing to distinguish - the driver 
> > generates synthetic wheel event for the inertia.
> > You can btw. turn that  off.
> > 
> > Seems an issue with inertial scrolling on X11 as well 
> > https://bugs.freedesktop.org/show_bug.cgi?id=38909
> > Otherwise i'd have opted for "the driver shall please stop this when you 
> > hit a key".
> > 
> > On a random note, I can't find that setting in systemsettings?
> > If there's a config GUI for this, aligning to it seems reasonable, BUT does 
> > no way fix the actual issue w/ inertia (ie. "you don't have control over 
> > your input device")
> 
> René J.V. Bertin wrote:
> You may not believe it, but I actually prefer the UE with the feature on. 
> Probably because it saves me some movements, which is always good (less RSI).
> 
> Yes, I imagine that the issue can occur on Linux as well if inertial 
> scrolling works the same way there. It just never bit me on Linux - and yet I 
> run that on underpowered machines ...
> Might be a thought to define the modifier key to get wheelMouseZooms in 
> that case, or at least make that possible somewhere in systemsettings?
> 
> You're right, I haven't been able to find the setting in systemsettings. 
> The setting *is* part of the standard settings, though, no idea why it 
> slipped through and remained a hidden setting. But because it's part of the 
> standard settings I went with aligning to it, instead of hacking in a new 
> switch as in Konsole.
> NB, seems likely that Konsole offers its own switch because the authors 
> didn't go the length I did to find out about the one in `kdeglobalrc`?
> 
> Thomas Lübking wrote:
> UX isn't my concern - but doing that in the driver is simply idiotic.
> (Sorry, but hey - Peter H. shares that opinion ;-)
> What eg. happens when you reached the end of the document? The driver 
> keeps scrolling and you don't even notice that.
> 
> Either the view provides inertic scrolling or you purchase a rodant that 
> can alter between swing and precise wheel (w/o trying to advertise here, eg. 
> Logitech M500 is an affordable device with this capability)
> 
> Choosing another modifier won't help, but at best move the problem around 
> (ok, there's no META key on OSX, but it is indeed used for shortcuts on 
> linux/windows - often global ones. So instead of scaling the text, you end up 
> scaling the entire desktop ;-)
> 
> While I personally don't use text scaling in kate (this way), if there's 
> no present UI for the global config, it can hardly be used and we can't (?) 
> introduce config GUI for the kate part in SC4, I assume (kate devs will know 
> better)
> Also simply disabling a feature because it is *one* occasion of a driver 
> issue doesn't sound too reasonable - what if I'd in general like to scale the 
> text this way but am still annoyed by the driver behavior?
> 
> One way to deal with this is to measure the time between the last 
> unmodified wheel event and the now modified wheel event. If that is too low 
> to be reasonably caused by a human being (ie. the scroll "started" unmodified 
> and suddenly a modifier kicks in) one could ignore the modifier.
> That would however have to apply to all items.
> Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in 
> this regard?
> 
> René J.V. Bertin wrote:
> I use Apple's Magic Trackpad, no spamming intended, but I wouldn't want 
> to change for anything else anymore... 
> 
> > ok, there's no META key on OSX
> 
> There is. META is mapped to Ctrl by default on OS X (and CTRL to Command, 
> which is the real Meta key), but there is a setting to switch that back. I 
> presume the default is conceived to reflect the fact that OS X uses the 
> Command key in place of Ctrl for shortcuts. It apparently never occurred to 
> them to define a special code (`ACCEL`) for the standard accelerator "opcode" 
> ;)
> 
> > Also simply disabling a feature because it is one occasion of a driver 
> issue doesn't sound too reasonable 
> 
> How do you think I found the global setting and how to read it out? It's 
> used exactly this way in KTextEdit (see `KTextEdit::wheelEvent`) which again 
> raises the question why the setting isn't exposed in systemsettings.
> 
> > - what if I'd in general like to scale the text this way but am still 
> annoyed by the driver behavior?
> 
> The only solution in that case would be to configure the feature to use 
> another key or key combination, one that works for you and your workflow and 
> doesn't risk to get triggered unexpectedly. 
> 
> > Stupid question: how does (Qt)WebKit behave (eg. in qt-assistant?) in 
> this regard?
> 
> Not so stupid: Qt does exactly the same thing, and it doesn't appear to 
> be optional in any way there. This is probably why

Re: Review Request 124824: [OS X] FindKDE4Internal.cmake : reintroduce a cmake_minimum_required statement

2015-08-21 Thread René J . V . Bertin



 On Aug. 19, 2015, 8:06 p.m., Stephen Kelly wrote:
  This patch is not correct. 
  
  What repo were you trying to build? Add cmake_minimum_required(VERSION 
  2.8.9) there.
 
 Stephen Kelly wrote:
 Why did this review go quiet? Did you pull and realize that line was 
 already there in whatever repo you're looking at?

What, my 2 replies below aren't sufficient? :) If you missed them, that'd 
probably support of my idea that most KDE devs are focussed away from flogging 
dead horses (i.e., KDE4 code, to quote one of those devs, a rather visible 
one I won't name).

No, I didn't pull whatever repo after pulling the 4.14 branch head of one of 
the KDE PIM components and bumping into this error.


- René J.V.


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


On Aug. 19, 2015, 7:41 p.m., René J.V. Bertin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124824/
 ---
 
 (Updated Aug. 19, 2015, 7:41 p.m.)
 
 
 Review request for KDE Software on Mac OS X and kdelibs.
 
 
 Repository: kdelibs
 
 
 Description
 ---
 
 The recent removal of the code block from FindKDE4Internal.cmake starting 
 with the `cmake_minimum_required` statement breaks configuring on OS X:
 
 ```
 -- The C compiler identification is AppleClang 6.0.0.657
 -- The CXX compiler identification is AppleClang 6.0.0.657
 -- Check for working C compiler: 
 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang
 -- Check for working C compiler: 
 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang
  -- works
 -- Detecting C compiler ABI info
 -- Detecting C compiler ABI info - done
 -- Detecting C compile features
 -- Detecting C compile features - done
 -- Check for working CXX compiler: 
 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++
 -- Check for working CXX compiler: 
 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++
  -- works
 -- Detecting CXX compiler ABI info
 -- Detecting CXX compiler ABI info - done
 -- Detecting CXX compile features
 -- Detecting CXX compile features - done
 -- Looking for Q_WS_X11
 -- Looking for Q_WS_X11 - not found
 -- Looking for Q_WS_WIN
 -- Looking for Q_WS_WIN - not found
 -- Looking for Q_WS_QWS
 -- Looking for Q_WS_QWS - not found
 -- Looking for Q_WS_MAC
 -- Looking for Q_WS_MAC - found
 -- Looking for QT_MAC_USE_COCOA
 -- Looking for QT_MAC_USE_COCOA - found
 -- Found Qt-Version 4.8.7 (using /opt/local/libexec/qt4/bin/qmake)
 -- Looking for include file pthread.h
 -- Looking for include file pthread.h - found
 -- Looking for pthread_create
 -- Looking for pthread_create - found
 -- Found Threads: TRUE  
 CMake Error at /opt/local/share/cmake-3.2/Modules/FindPkgConfig.cmake:112 
 (elseif):
   given arguments:
 
 VERSION_LESS 3.1
 
   Unknown arguments specified
 Call Stack (most recent call first):
   /opt/local/share/cmake-3.2/Modules/FindPkgConfig.cmake:501 
 (_pkgconfig_parse_options)
   /opt/local/share/cmake-3.2/Modules/FindOpenSSL.cmake:43 (pkg_check_modules)
   /opt/local/share/apps/cmake/modules/Qt4ConfigDependentSettings.cmake:224 
 (FIND_PACKAGE)
   /opt/local/share/apps/cmake/modules/FindQt4.cmake:1207 (INCLUDE)
   /opt/local/share/apps/cmake/modules/FindKDE4Internal.cmake:425 
 (find_package)
   /opt/local/share/cmake-3.2/Modules/FindKDE4.cmake:108 (find_package)
   CMakeLists.txt:4 (find_package)
 
 
 -- Configuring incomplete, errors occurred!
 ```
 
 The attached patch is the minimum reintroduction of the removed code that 
 allows the cmake procedure to conclude successfully.
 
 CMake experts might be aware of other ways to address this issue more in line 
 with the reason the block was removed.
 
 
 Diffs
 -
 
   cmake/modules/FindKDE4Internal.cmake 7d54b9b 
 
 Diff: https://git.reviewboard.kde.org/r/124824/diff/
 
 
 Testing
 ---
 
 On OS X 10.9.5 with KDELibs 4.14.11 (v4.14.10-20-g150d983) and CMake 3.2.2 .
 
 The unmodified code from v4.14.10-20-g150d983 is fine on Linux with cmake 
 3.0.1 .
 
 
 Thanks,
 
 René J.V. Bertin

Re: Review Request 124824: [OS X] FindKDE4Internal.cmake : reintroduce a cmake_minimum_required statement

2015-08-21 Thread René J . V . Bertin

On Aug. 19, 2015, 8:06 p.m., Stephen Kelly wrote:
This patch is not correct.

What repo were you trying to build? Add cmake_minimum_required(VERSION
2.8.9) there.

Stephen Kelly wrote:
Why did this review go quiet? Did you pull and realize that line was
already there in whatever repo you're looking at?

René J.V. Bertin wrote:
What, my 2 replies below aren't sufficient? :) If you missed them, that'd
probably support of my idea that most KDE devs are focussed away from
flogging dead horses (i.e., KDE4 code, to quote one of those devs, a rather
visible one I won't name).

No, I didn't pull whatever repo after pulling the 4.14 branch head of one
of the KDE PIM components and bumping into this error.

Luigi Toscano wrote:
David Faure, durink Akademy, was rebuilding the last kdelibs4 branch for
most applications, I think to ensure that the changes in the default cmake
policy did not break them.
He did various fixes, so I think that fixing this in the proper way
(which I can't help to, sorry) would be good.

And he did a good job *on Linux* to the extent that I haven't been able to
trigger the issue on my Linux rig (Kubuntu 14.04.3LTS with cmake 3.0.1). Which
is why my patch is Mac-specific.

But frankly, I don't see the point in obliging each and every dependent project
to declare the minimum required cmake version. Not if that's not a requirement
of cmake itself, nor of the project, but of one of the libraries it uses. I
really don't see how this is different from, say, declaring CMake macros that
are specific to KDE and not to a given dependent project.

On the contrary, it seems clear that the version requirement stems from
something done in (or included by) a cmake file that's installed through
kdelibs. In my book that makes it kdelibs' responsibility to declare the
minimum required version. If it's not yet set, and/or as long as subsequent
calls to `cmake_minimum_required` do not override (= downgrade) minimum
versions set by preceding calls.

That said, I'm going to have to get back to my Mac to verify why I apparently
missed kdepim commit 8a187bc316271caed9c2591d91fb8706abe5c665 (Fix the build
after yet another cmake policy change) or what else is going on.

- René J.V.

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

On Aug. 19, 2015, 7:41 p.m., René J.V. Bertin wrote:

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

(Updated Aug. 19, 2015, 7:41 p.m.)

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

Repository: kdelibs

Description
---

The recent removal of the code block from FindKDE4Internal.cmake starting
with the `cmake_minimum_required` statement breaks configuring on OS X:

```
-- The C compiler identification is AppleClang 6.0.0.657
-- The CXX compiler identification is AppleClang 6.0.0.657
-- Check for working C compiler:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang
-- Check for working C compiler:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang
-- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++
-- Check for working CXX compiler:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++
-- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Looking for Q_WS_X11
-- Looking for Q_WS_X11 - not found
-- Looking for Q_WS_WIN
-- Looking for Q_WS_WIN - not found
-- Looking for Q_WS_QWS
-- Looking for Q_WS_QWS - not found
-- Looking for Q_WS_MAC
-- Looking for Q_WS_MAC - found
-- Looking for QT_MAC_USE_COCOA
-- Looking for QT_MAC_USE_COCOA - found
-- Found Qt-Version 4.8.7 (using /opt/local/libexec/qt4/bin/qmake)
-- Looking for include file pthread.h
-- Looking for include file pthread.h - found
-- Looking for pthread_create
-- Looking for pthread_create - found
-- Found Threads: TRUE
CMake Error at /opt/local/share/cmake-3.2/Modules/FindPkgConfig.cmake:112
(elseif):
given arguments:

VERSION_LESS 3.1

Unknown arguments specified
Call Stack (most recent call first):
/opt/local/share/cmake-3.2/Modules/FindPkgConfig.cmake:501
(_pkgconfig_parse_options)

Review Request 124824: [OS X] FindKDE4Internal.cmake : reintroduce a cmake_minimum_required statement

2015-08-19 Thread René J . V . Bertin


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

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


Repository: kdelibs


Description
---

The recent removal of the code block from FindKDE4Internal.cmake starting with 
the `cmake_minimum_required` statement breaks configuring on OS X:

```
-- The C compiler identification is AppleClang 6.0.0.657
-- The CXX compiler identification is AppleClang 6.0.0.657
-- Check for working C compiler: 
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang
-- Check for working C compiler: 
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang
 -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: 
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++
-- Check for working CXX compiler: 
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++
 -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Looking for Q_WS_X11
-- Looking for Q_WS_X11 - not found
-- Looking for Q_WS_WIN
-- Looking for Q_WS_WIN - not found
-- Looking for Q_WS_QWS
-- Looking for Q_WS_QWS - not found
-- Looking for Q_WS_MAC
-- Looking for Q_WS_MAC - found
-- Looking for QT_MAC_USE_COCOA
-- Looking for QT_MAC_USE_COCOA - found
-- Found Qt-Version 4.8.7 (using /opt/local/libexec/qt4/bin/qmake)
-- Looking for include file pthread.h
-- Looking for include file pthread.h - found
-- Looking for pthread_create
-- Looking for pthread_create - found
-- Found Threads: TRUE  
CMake Error at /opt/local/share/cmake-3.2/Modules/FindPkgConfig.cmake:112 
(elseif):
  given arguments:

VERSION_LESS 3.1

  Unknown arguments specified
Call Stack (most recent call first):
  /opt/local/share/cmake-3.2/Modules/FindPkgConfig.cmake:501 
(_pkgconfig_parse_options)
  /opt/local/share/cmake-3.2/Modules/FindOpenSSL.cmake:43 (pkg_check_modules)
  /opt/local/share/apps/cmake/modules/Qt4ConfigDependentSettings.cmake:224 
(FIND_PACKAGE)
  /opt/local/share/apps/cmake/modules/FindQt4.cmake:1207 (INCLUDE)
  /opt/local/share/apps/cmake/modules/FindKDE4Internal.cmake:425 (find_package)
  /opt/local/share/cmake-3.2/Modules/FindKDE4.cmake:108 (find_package)
  CMakeLists.txt:4 (find_package)


-- Configuring incomplete, errors occurred!
```

The attached patch is the minimum reintroduction of the removed code that 
allows the cmake procedure to conclude successfully.

CMake experts might be aware of other ways to address this issue more in line 
with the reason the block was removed.


Diffs
-

  cmake/modules/FindKDE4Internal.cmake 7d54b9b 

Diff: https://git.reviewboard.kde.org/r/124824/diff/


Testing
---

On OS X 10.9.5 with KDELibs 4.14.11 (v4.14.10-20-g150d983) and CMake 3.2.2 .

The unmodified code from v4.14.10-20-g150d983 is fine on Linux with cmake 3.0.1 
.


Thanks,

René J.V. Bertin

Re: Review Request 124824: [OS X] FindKDE4Internal.cmake : reintroduce a cmake_minimum_required statement

2015-08-19 Thread René J . V . Bertin



 On Aug. 19, 2015, 11:14 p.m., Heiko Becker wrote:
  I stumbled upon the same, it's actually a bug in cmake fixed by this commit:
  
  
  http://www.cmake.org/gitweb?p=cmake.git;a=commit;h=b9ec9392da21a3421e48c6961976060d872faffb
 
 René J.V. Bertin wrote:
 Well, IMVHO it's fine to impose upgrading to the latest cmake and/or 
 adding the `cmake_minimum_required` statement in each and every dependency's 
 toplevel CMake file, but not for the legacy KDE/4.14 branch (esp. not the 
 latter option)...

And to reply to Eike (without subscribing to kde-core-devel): the shortest fix 
is not touching what always worked. With most project developers now focussing 
on KF5, changing the toplevel CMake file would fall to users and distro 
maintainers, who shouldn't have to be bothered by this kind of patching.


- René J.V.


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


On Aug. 19, 2015, 7:41 p.m., René J.V. Bertin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124824/
 ---
 
 (Updated Aug. 19, 2015, 7:41 p.m.)
 
 
 Review request for KDE Software on Mac OS X and kdelibs.
 
 
 Repository: kdelibs
 
 
 Description
 ---
 
 The recent removal of the code block from FindKDE4Internal.cmake starting 
 with the `cmake_minimum_required` statement breaks configuring on OS X:
 
 ```
 -- The C compiler identification is AppleClang 6.0.0.657
 -- The CXX compiler identification is AppleClang 6.0.0.657
 -- Check for working C compiler: 
 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang
 -- Check for working C compiler: 
 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang
  -- works
 -- Detecting C compiler ABI info
 -- Detecting C compiler ABI info - done
 -- Detecting C compile features
 -- Detecting C compile features - done
 -- Check for working CXX compiler: 
 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++
 -- Check for working CXX compiler: 
 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++
  -- works
 -- Detecting CXX compiler ABI info
 -- Detecting CXX compiler ABI info - done
 -- Detecting CXX compile features
 -- Detecting CXX compile features - done
 -- Looking for Q_WS_X11
 -- Looking for Q_WS_X11 - not found
 -- Looking for Q_WS_WIN
 -- Looking for Q_WS_WIN - not found
 -- Looking for Q_WS_QWS
 -- Looking for Q_WS_QWS - not found
 -- Looking for Q_WS_MAC
 -- Looking for Q_WS_MAC - found
 -- Looking for QT_MAC_USE_COCOA
 -- Looking for QT_MAC_USE_COCOA - found
 -- Found Qt-Version 4.8.7 (using /opt/local/libexec/qt4/bin/qmake)
 -- Looking for include file pthread.h
 -- Looking for include file pthread.h - found
 -- Looking for pthread_create
 -- Looking for pthread_create - found
 -- Found Threads: TRUE  
 CMake Error at /opt/local/share/cmake-3.2/Modules/FindPkgConfig.cmake:112 
 (elseif):
   given arguments:
 
 VERSION_LESS 3.1
 
   Unknown arguments specified
 Call Stack (most recent call first):
   /opt/local/share/cmake-3.2/Modules/FindPkgConfig.cmake:501 
 (_pkgconfig_parse_options)
   /opt/local/share/cmake-3.2/Modules/FindOpenSSL.cmake:43 (pkg_check_modules)
   /opt/local/share/apps/cmake/modules/Qt4ConfigDependentSettings.cmake:224 
 (FIND_PACKAGE)
   /opt/local/share/apps/cmake/modules/FindQt4.cmake:1207 (INCLUDE)
   /opt/local/share/apps/cmake/modules/FindKDE4Internal.cmake:425 
 (find_package)
   /opt/local/share/cmake-3.2/Modules/FindKDE4.cmake:108 (find_package)
   CMakeLists.txt:4 (find_package)
 
 
 -- Configuring incomplete, errors occurred!
 ```
 
 The attached patch is the minimum reintroduction of the removed code that 
 allows the cmake procedure to conclude successfully.
 
 CMake experts might be aware of other ways to address this issue more in line 
 with the reason the block was removed.
 
 
 Diffs
 -
 
   cmake/modules/FindKDE4Internal.cmake 7d54b9b 
 
 Diff: https://git.reviewboard.kde.org/r/124824/diff/
 
 
 Testing
 ---
 
 On OS X 10.9.5 with KDELibs 4.14.11 (v4.14.10-20-g150d983) and CMake 3.2.2 .
 
 The unmodified code from v4.14.10-20-g150d983 is fine on Linux with cmake 
 3.0.1 .
 
 
 Thanks,
 
 René J.V. Bertin

Re: Review Request 124824: [OS X] FindKDE4Internal.cmake : reintroduce a cmake_minimum_required statement

2015-08-19 Thread René J . V . Bertin



 On Aug. 19, 2015, 11:14 p.m., Heiko Becker wrote:
  I stumbled upon the same, it's actually a bug in cmake fixed by this commit:
  
  
  http://www.cmake.org/gitweb?p=cmake.git;a=commit;h=b9ec9392da21a3421e48c6961976060d872faffb

Well, IMVHO it's fine to impose upgrading to the latest cmake and/or adding the 
`cmake_minimum_required` statement in each and every dependency's toplevel 
CMake file, but not for the legacy KDE/4.14 branch (esp. not the latter 
option)...


- René J.V.


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


On Aug. 19, 2015, 7:41 p.m., René J.V. Bertin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/124824/
 ---
 
 (Updated Aug. 19, 2015, 7:41 p.m.)
 
 
 Review request for KDE Software on Mac OS X and kdelibs.
 
 
 Repository: kdelibs
 
 
 Description
 ---
 
 The recent removal of the code block from FindKDE4Internal.cmake starting 
 with the `cmake_minimum_required` statement breaks configuring on OS X:
 
 ```
 -- The C compiler identification is AppleClang 6.0.0.657
 -- The CXX compiler identification is AppleClang 6.0.0.657
 -- Check for working C compiler: 
 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang
 -- Check for working C compiler: 
 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang
  -- works
 -- Detecting C compiler ABI info
 -- Detecting C compiler ABI info - done
 -- Detecting C compile features
 -- Detecting C compile features - done
 -- Check for working CXX compiler: 
 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++
 -- Check for working CXX compiler: 
 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++
  -- works
 -- Detecting CXX compiler ABI info
 -- Detecting CXX compiler ABI info - done
 -- Detecting CXX compile features
 -- Detecting CXX compile features - done
 -- Looking for Q_WS_X11
 -- Looking for Q_WS_X11 - not found
 -- Looking for Q_WS_WIN
 -- Looking for Q_WS_WIN - not found
 -- Looking for Q_WS_QWS
 -- Looking for Q_WS_QWS - not found
 -- Looking for Q_WS_MAC
 -- Looking for Q_WS_MAC - found
 -- Looking for QT_MAC_USE_COCOA
 -- Looking for QT_MAC_USE_COCOA - found
 -- Found Qt-Version 4.8.7 (using /opt/local/libexec/qt4/bin/qmake)
 -- Looking for include file pthread.h
 -- Looking for include file pthread.h - found
 -- Looking for pthread_create
 -- Looking for pthread_create - found
 -- Found Threads: TRUE  
 CMake Error at /opt/local/share/cmake-3.2/Modules/FindPkgConfig.cmake:112 
 (elseif):
   given arguments:
 
 VERSION_LESS 3.1
 
   Unknown arguments specified
 Call Stack (most recent call first):
   /opt/local/share/cmake-3.2/Modules/FindPkgConfig.cmake:501 
 (_pkgconfig_parse_options)
   /opt/local/share/cmake-3.2/Modules/FindOpenSSL.cmake:43 (pkg_check_modules)
   /opt/local/share/apps/cmake/modules/Qt4ConfigDependentSettings.cmake:224 
 (FIND_PACKAGE)
   /opt/local/share/apps/cmake/modules/FindQt4.cmake:1207 (INCLUDE)
   /opt/local/share/apps/cmake/modules/FindKDE4Internal.cmake:425 
 (find_package)
   /opt/local/share/cmake-3.2/Modules/FindKDE4.cmake:108 (find_package)
   CMakeLists.txt:4 (find_package)
 
 
 -- Configuring incomplete, errors occurred!
 ```
 
 The attached patch is the minimum reintroduction of the removed code that 
 allows the cmake procedure to conclude successfully.
 
 CMake experts might be aware of other ways to address this issue more in line 
 with the reason the block was removed.
 
 
 Diffs
 -
 
   cmake/modules/FindKDE4Internal.cmake 7d54b9b 
 
 Diff: https://git.reviewboard.kde.org/r/124824/diff/
 
 
 Testing
 ---
 
 On OS X 10.9.5 with KDELibs 4.14.11 (v4.14.10-20-g150d983) and CMake 3.2.2 .
 
 The unmodified code from v4.14.10-20-g150d983 is fine on Linux with cmake 
 3.0.1 .
 
 
 Thanks,
 
 René J.V. Bertin

Re: Review Request 120202: [OS X] improvements to the kwallet/OSX keychain integration

2015-08-17 Thread René J . V . Bertin


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

(Updated Aug. 17, 2015, 5 p.m.)


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


Changes
---

Improved handling of the QSharedMemory create/attach functionality for the 
wallet access timer feature. Attempting an attach in order to create the shared 
segment only when the attach failed can lead to race conditions where a 
different process manages to create the segment while we're still trying to 
attach. This is actually documented in the QSharedMemory notes for QNX.


Repository: kdelibs


Description
---

I'm still working on (the KDE4-based version of) my OS X keychain backend for 
kwallet. I'm at a point where I think I can present a work-in-progress in an RR 
because at least one feature has been improved enough to be of interest for 
everyone, and also because I could use feedback on how to proceed.
I'm currently focussing on 2 settings that are configured in the kwallet KCM 
(SystemSettings), and for which I'm working on an implementation not requiring 
kwalletd and/or DBus.

- idle time closing of wallets. This feature was not supported in the commited 
version presented in https://git.reviewboard.kde.org/r/119838/ The present 
patch adds an idleTimer and a shared lastAccessTime member. The idleTimer is 
reset each time a client performs one of a series of actions that I count as 
wallet accesses, and before resetting I update the idle timeout value from 
KConfig. When the timer fires, the elapsed time is compared to the shared last 
access time, and if it is = the timeout, the wallet is closed. This applies 
only to KDE keychains, so keychains used by OS X applications should not be 
affected.

- close when last application exits. This requires maintaining a user list 
which keeps track of what application has what wallet open. I've implemented an 
internal version of such a registry, mapping wallet name to application names 
and the list of wallets they have open (a list of wallet reference, pid per 
application name). The registry is functional, but I have not yet decided 
(read: figured out) how to make a distributed representation of it.

So the work-in-progress concerns the distributed user registry. The idea would 
be to maintain the registry in shared memory, meaning it'd be reset (= 
disappear) when the last application exits, contrary to a file which can go 
stale. This would be simple if QSharedMemory objects could be resized, but 
apparently they cannot, so I'll have to look at other solutions possibly 
involving OS X frameworks (NSData and it's non-objectiveC version CFDataRef or 
CFMutableDataRef might be candidates). Suggestions welcome.

Other work in progress concerns a less wheel-reinventing approach that builds 
on kwalletd and DBus. I don't see why the code used in `kwallet.cpp` wouldn't 
work, but I must still misunderstand its finer details. The present patch 
contains outcommented code that does indeed cause kwalletd to be launched and 
slots and signals to become visible e.g. in `qdbusviewer`. But they don't work, 
which in turn makes the whole kwallet layer dysfunctional. Here too feedback is 
welcome on how what I'm missing and/or how to get this to work.
Once kwalletd works, wallet idle timeout closing and closing when the last 
client exits should work out-of-the-box, or at least I suppose.


Diffs (updated)
-

  kdeui/util/kwallet_mac.h PRE-CREATION 
  kdeui/util/kwallet_mac.cpp 8344ebb 
  kdeui/util/qosxkeychain.h d0934e6 
  kdeui/util/qosxkeychain.cpp 7cb9a22 

Diff: https://git.reviewboard.kde.org/r/120202/diff/


Testing
---

OS X 10.6.8 and OS X 10.9.5, kdelibs 4.14.n git/master, KDE/MacPorts 4.12.5  
4.13.3.
Once finalised, all changes should port easily to KF5's kwallet_mac.cpp .


Thanks,

René J.V. Bertin

Re: Review Request 123458: Improvements to the handling of font weights and styles


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

(Updated April 22, 2015, 12:01 a.m.)


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


Repository: qt


Description
---

Handling of and support for less common font weight/style combinations is far 
from ideal on OS X but not perfect on Linux either. It is not difficult to run 
into typefaces that will not be restored properly from settings files for 
instance, because QFont(family,weight,style) and other methods to obtain a 
QFont from a font description do not return the appropriate font.
This is especially the case on OS X where the code makes the assumption in at 
least two locations that anything that isn't Normal is Bold. In other 
places, including generic code, parsers apply overly course numberic weight 
classifications or fail to consider weights like Medium, Semibold, 
Regular, Roman etc. (and return a fall-back weight: Normal).
Among the font families that are affected there are als common fonts like Segoe 
UI and Helvetica Neue (UI fonts on MS Windows and OS X 10.10+). NB: 
medium/semi-/demi-bold weights are perfect in UIs on high-resolution screens.

The proposed patch improves the code by adding additional checks against style 
names and weights. The changes are not only to Mac-specific files so Linux 
benefits from this too (and other platforms ought to, as well).

I'm putting it up for review on here mainly for lack of time to figure out why 
I failed to get in onto Qt's own code review site. It may appear there too, but 
if not:

I herewith put the attached code changes in the public domain, for possible 
inclusion into the Qt 4.x codebase under the license that governs that software.


Diffs (updated)
-

  src/gui/dialogs/qfontdialog.cpp d791462 
  src/gui/dialogs/qfontdialog_mac.mm d557a7a 
  src/gui/kernel/qt_mac.cpp fb241ce 
  src/gui/text/qfontdatabase.cpp 4c2ace4 
  src/gui/text/qfontdatabase_mac.cpp 816a7bd 
  src/gui/text/qfontengine_coretext.mm 204d685 
  src/plugins/platforms/fontdatabases/coretext/qcoretextfontdatabase.mm 312015f 
  tools/qtconfig/mainwindow.cpp 1bb6e4e 

Diff: https://git.reviewboard.kde.org/r/123458/diff/


Testing
---

On OS X 10.9.5 and Kubuntu 14.04.2 against Qt 4.8.7 with the qtconfig utility 
and the attached test application


File Attachments


fontweight issue test application
  
https://git.reviewboard.kde.org/media/uploaded/files/2015/04/21/bbebc0af-e457-4fdf-965c-3918d8e871d0__fontweightissue.pro
fontweight issue test application
  
https://git.reviewboard.kde.org/media/uploaded/files/2015/04/21/cfdcbf23-406a-4abf-b0cb-5fdc1957dfe9__main.cpp
fontweight issue test application
  
https://git.reviewboard.kde.org/media/uploaded/files/2015/04/21/9898afae-82ca-4c92-b7d9-5607d0dd8e24__dialog.h
fontweight issue test application
  
https://git.reviewboard.kde.org/media/uploaded/files/2015/04/21/8ad258bf-7f82-4db4-a344-ce81d75e8c50__dialog.cpp
fontweight issue test application
  
https://git.reviewboard.kde.org/media/uploaded/files/2015/04/21/2f0a7c6e-4601-406e-b703-603a36bff62b__fontweightissue.desktop


Thanks,

René J.V. Bertin

Re: Review Request 123458: Improvements to the handling of font weights and styles


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

(Updated April 22, 2015, 12:06 a.m.)


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


Repository: qt


Description
---

Handling of and support for less common font weight/style combinations is far 
from ideal on OS X but not perfect on Linux either. It is not difficult to run 
into typefaces that will not be restored properly from settings files for 
instance, because QFont(family,weight,style) and other methods to obtain a 
QFont from a font description do not return the appropriate font.
This is especially the case on OS X where the code makes the assumption in at 
least two locations that anything that isn't Normal is Bold. In other 
places, including generic code, parsers apply overly course numberic weight 
classifications or fail to consider weights like Medium, Semibold, 
Regular, Roman etc. (and return a fall-back weight: Normal).
Among the font families that are affected there are als common fonts like Segoe 
UI and Helvetica Neue (UI fonts on MS Windows and OS X 10.10+). NB: 
medium/semi-/demi-bold weights are perfect in UIs on high-resolution screens.

The proposed patch improves the code by adding additional checks against style 
names and weights. The changes are not only to Mac-specific files so Linux 
benefits from this too (and other platforms ought to, as well).

I'm putting it up for review on here mainly for lack of time to figure out why 
I failed to get in onto Qt's own code review site. It may appear there too, but 
if not:

I herewith put the attached code changes in the public domain, for possible 
inclusion into the Qt 4.x codebase under the license that governs that software.


Diffs (updated)
-

  src/gui/dialogs/qfontdialog.cpp d791462 
  src/gui/dialogs/qfontdialog_mac.mm d557a7a 
  src/gui/kernel/qt_mac.cpp fb241ce 
  src/gui/text/qfontdatabase.cpp 4c2ace4 
  src/gui/text/qfontdatabase_mac.cpp 816a7bd 
  src/gui/text/qfontengine_coretext.mm 204d685 
  src/plugins/platforms/fontdatabases/coretext/qcoretextfontdatabase.mm 312015f 
  tools/qtconfig/mainwindow.cpp 1bb6e4e 

Diff: https://git.reviewboard.kde.org/r/123458/diff/


Testing
---

On OS X 10.9.5 and Kubuntu 14.04.2 against Qt 4.8.7 with the qtconfig utility 
and the attached test application


File Attachments


fontweight issue test application
  
https://git.reviewboard.kde.org/media/uploaded/files/2015/04/21/bbebc0af-e457-4fdf-965c-3918d8e871d0__fontweightissue.pro
fontweight issue test application
  
https://git.reviewboard.kde.org/media/uploaded/files/2015/04/21/cfdcbf23-406a-4abf-b0cb-5fdc1957dfe9__main.cpp
fontweight issue test application
  
https://git.reviewboard.kde.org/media/uploaded/files/2015/04/21/9898afae-82ca-4c92-b7d9-5607d0dd8e24__dialog.h
fontweight issue test application
  
https://git.reviewboard.kde.org/media/uploaded/files/2015/04/21/8ad258bf-7f82-4db4-a344-ce81d75e8c50__dialog.cpp
fontweight issue test application
  
https://git.reviewboard.kde.org/media/uploaded/files/2015/04/21/2f0a7c6e-4601-406e-b703-603a36bff62b__fontweightissue.desktop


Thanks,

René J.V. Bertin

Re: Review Request 123458: Improvements to the handling of font weights and styles

On April 22, 2015, 12:40 a.m., Luigi Toscano wrote:
I think this should go to Qt (I think it's quite difficult they will accept
it, as Qt4 is in hard freeze mode), and they will probably ask to see if it
applies to Qt5 as well.
We just mirror Qt... not sure how to handle this.

Luigi Toscano wrote:
Yes, I know you wrote that it should go to Qt, just stating that I'm not
sure how to handle this patch in our Qt mirrors.

Aleix Pol Gonzalez wrote:
+1, we shouldn't go about maintaining a patched Qt4 fork.

Ben Cooksley wrote:
Our Qt mirrors are configured to be exact copies of the upstream
code.qt.io mirrors, therefore any patches have to go through Qt Project to
form part of our mirrors of it.

It didn't even cross my mind to commit this patch to the Qt4 fork, just to put
it up for review (and make it available publicly).

Oh, and if someone were to beat me to putting this up on Qt's code review site,
I'd actually appreciate that, a lot even O:-)

- René J.V.

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

On April 22, 2015, 12:06 a.m., René J.V. Bertin wrote:

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

(Updated April 22, 2015, 12:06 a.m.)

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

Repository: qt

Description
---

Handling of and support for less common font weight/style combinations is far
from ideal on OS X but not perfect on Linux either. It is not difficult to
run into typefaces that will not be restored properly from settings files for
instance, because QFont(family,weight,style) and other methods to obtain a
QFont from a font description do not return the appropriate font.
This is especially the case on OS X where the code makes the assumption in at
least two locations that anything that isn't Normal is Bold. In other
places, including generic code, parsers apply overly course numberic weight
classifications or fail to consider weights like Medium, Semibold,
Regular, Roman etc. (and return a fall-back weight: Normal).
Among the font families that are affected there are als common fonts like
Segoe UI and Helvetica Neue (UI fonts on MS Windows and OS X 10.10+). NB:
medium/semi-/demi-bold weights are perfect in UIs on high-resolution screens.

The proposed patch improves the code by adding additional checks against
style names and weights. The changes are not only to Mac-specific files so
Linux benefits from this too (and other platforms ought to, as well).

I'm putting it up for review on here mainly for lack of time to figure out
why I failed to get in onto Qt's own code review site. It may appear there
too, but if not:

I herewith put the attached code changes in the public domain, for possible
inclusion into the Qt 4.x codebase under the license that governs that
software.

Diffs
-

src/gui/dialogs/qfontdialog.cpp d791462
src/gui/dialogs/qfontdialog_mac.mm d557a7a
src/gui/kernel/qt_mac.cpp fb241ce
src/gui/text/qfontdatabase.cpp 4c2ace4
src/gui/text/qfontdatabase_mac.cpp 816a7bd
src/gui/text/qfontengine_coretext.mm 204d685
src/plugins/platforms/fontdatabases/coretext/qcoretextfontdatabase.mm
312015f
tools/qtconfig/mainwindow.cpp 1bb6e4e

Diff: https://git.reviewboard.kde.org/r/123458/diff/

Testing
---

On OS X 10.9.5 and Kubuntu 14.04.2 against Qt 4.8.7 with the qtconfig utility
and the attached test application

File Attachments

fontweight issue test application

https://git.reviewboard.kde.org/media/uploaded/files/2015/04/21/bbebc0af-e457-4fdf-965c-3918d8e871d0__fontweightissue.pro
fontweight issue test application

https://git.reviewboard.kde.org/media/uploaded/files/2015/04/21/cfdcbf23-406a-4abf-b0cb-5fdc1957dfe9__main.cpp
fontweight issue test application

https://git.reviewboard.kde.org/media/uploaded/files/2015/04/21/9898afae-82ca-4c92-b7d9-5607d0dd8e24__dialog.h
fontweight issue test application

https://git.reviewboard.kde.org/media/uploaded/files/2015/04/21/8ad258bf-7f82-4db4-a344-ce81d75e8c50__dialog.cpp
fontweight issue test application

https://git.reviewboard.kde.org/media/uploaded/files/2015/04/21/2f0a7c6e-4601-406e-b703-603a36bff62b__fontweightissue.desktop

Thanks,

René J.V. Bertin

Review Request 123458: Improvements to the handling of font weights and styles


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

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


Repository: qt


Description
---

Handling of and support for less common font weight/style combinations is far 
from ideal on OS X but not perfect on Linux either. It is not difficult to run 
into typefaces that will not be restored properly from settings files for 
instance, because QFont(family,weight,style) and other methods to obtain a 
QFont from a font description do not return the appropriate font.
This is especially the case on OS X where the code makes the assumption in at 
least two locations that anything that isn't Normal is Bold. In other 
places, including generic code, parsers apply overly course numberic weight 
classifications or fail to consider weights like Medium, Semibold, 
Regular, Roman etc. (and return a fall-back weight: Normal).
Among the font families that are affected there are als common fonts like Segoe 
UI and Helvetica Neue (UI fonts on MS Windows and OS X 10.10+). NB: 
medium/semi-/demi-bold weights are perfect in UIs on high-resolution screens.

The proposed patch improves the code by adding additional checks against style 
names and weights. The changes are not only to Mac-specific files so Linux 
benefits from this too (and other platforms ought to, as well).

I'm putting it up for review on here mainly for lack of time to figure out why 
I failed to get in onto Qt's own code review site. It may appear there too, but 
if not:

I herewith put the attached code changes in the public domain, for possible 
inclusion into the Qt 4.x codebase under the license that governs that software.


Diffs
-

  src/gui/dialogs/qfontdialog.cpp d791462 
  src/gui/dialogs/qfontdialog_mac.mm d557a7a 
  src/gui/kernel/qt_mac.cpp fb241ce 
  src/gui/text/qfontdatabase.cpp 4c2ace4 
  src/gui/text/qfontdatabase_mac.cpp 816a7bd 
  src/gui/text/qfontengine_coretext.mm 204d685 
  src/plugins/platforms/fontdatabases/coretext/qcoretextfontdatabase.mm 312015f 
  tools/qtconfig/mainwindow.cpp 1bb6e4e 

Diff: https://git.reviewboard.kde.org/r/123458/diff/


Testing
---

On OS X 10.9.5 and Kubuntu 14.04.2 against Qt 4.8.7 with the qtconfig utility 
and the attached test application


File Attachments


fontweight issue test application
  
https://git.reviewboard.kde.org/media/uploaded/files/2015/04/21/bbebc0af-e457-4fdf-965c-3918d8e871d0__fontweightissue.pro
fontweight issue test application
  
https://git.reviewboard.kde.org/media/uploaded/files/2015/04/21/cfdcbf23-406a-4abf-b0cb-5fdc1957dfe9__main.cpp
fontweight issue test application
  
https://git.reviewboard.kde.org/media/uploaded/files/2015/04/21/9898afae-82ca-4c92-b7d9-5607d0dd8e24__dialog.h
fontweight issue test application
  
https://git.reviewboard.kde.org/media/uploaded/files/2015/04/21/8ad258bf-7f82-4db4-a344-ce81d75e8c50__dialog.cpp
fontweight issue test application
  
https://git.reviewboard.kde.org/media/uploaded/files/2015/04/21/2f0a7c6e-4601-406e-b703-603a36bff62b__fontweightissue.desktop


Thanks,

René J.V. Bertin

Re: [KDE/Mac] Multiplatform frameworks

2015-03-02 Thread René J . V . Bertin

On Sunday March 01 2015 17:37:35 Ian Wadham wrote:

Hi Ian

Let me kick off by saying that I am not necessarily in favour of doing what
the Romans do… ;-) 

Heh,  I got that! :)

And I do not like directory names with a space in them
either, but that is just an annoying detail...

I *hope* it's just a detail...

Modifying Q(Core)Application's constructor might not work in all cases (e.g. 
QSP)
because there may be processes that do not use Q(Core)Application, even if they
use Qt.  I am thinking of klauncher5, etc.  OTOH, you could just do extra 
patches
for those processes...

Yeah, I know I wrote Q(Core)Application, and later on I realised I should have 
referred to the QSP ctor. But it seems it doesn't have one, it's just a class 
with static methods, right? Which would still allow us to add an additional 
argument with a configurable default value where required...

I've started to look at how the ApplicationAttributes (AA_*) feature works. 
That's just a static member value of a private QApplication subclass, which is 
initialised outside of a function. That would only work to define a default at 
Qt's compile time, and not at the client compile time like I am after. Still, 
one might think of a solution in which that additional argument to the QSP 
functions is set to use whatever the AA_QSP_FLAVOUR attribute dictates in 
stock Qt, and can take other values to override that attribute. That's 
similar to the approach Qt follows for QAction menuRoles (cf. 
TextHeuristicsRole).

Another possibility (an Apple way… ;-) ), is to use Custom Keys in .plist 
files [2].

Yes, but that too would require reading out the values, and of course there's 
the issue that 1) not all applications have plist files and 2) even those (app 
bundles) that do, most have a standard plist. 

On a per app basis, you can insert values in Info.plist, using an 
Info.plist.template
file in your CMake build (as you well know, René, but I am telling others).

Yes, or you could modify the standard plist template that I'm guessing must 
exist somewhere. Still that's a lot more modification that I'd hope for, even 
if it has the advantage of shifting it from the source code to the build system.

Some examples;
/Library/Preferences/org.cups.printers.plistHost's list of known CUPS 
 printers
/Library/Preferences/com.apple.TimeMachine.plistHost's backup 
 preferences
~/Library/Preferences/org.cups.PrintingPrefs.plistMy CUPS printing 
 preferences

Those would be the settings of server applications, not of a library your app 
links to...

A shared .plist could contain whatever you liked (e.g. a prefix and a base for
alternate standard paths).  It could be read, using Cocoa methods, the first 
time
QSP is called.

Without QSP ctor I think it doesn't make too much sense to follow a first time 
only approach, and besides most code I've seen that does similar things seems 
to read the dictionary at each invocation. Presumable with the idea that the 
settings could change (which wouldn't make too much sense here).

could have a file at /Library/Preferences/org.kde.qt_tweaks.plist, but we would
still need a way to tell Qt libraries to look at it (or not)… :-(

As I already noted above. It sounds nifty, but I fear that in the end it just 
adds complexity by adding a platform-specific alternative to functionality that 
already exists (QSettings). (Who knows, maybe there's even a Qt interface to 
handling plist files, because the C code for reading them gets very 
elaborate/chatty very quickly.)

R.

Re: [KDE/Mac] Multiplatform frameworks



On February 28, 2015 1:20:27 PM CET, Jeremy Whiting jpwhit...@kde.org wrote:

Then data will all be under ~/Library/Application Support/Qt5/appname ?

I think Ian's idea was not to use the appname bit (for stuff shared with other 
apps).


that's a bit cleaner, but why would Qt/KDE applications need to use a
namespace like that when apple's own applications don't? Here I see

They don't. They don't even *have* to use /Library/Application Support...

directly, they all use a subfolder of it, otherwise we would see
application data files in /usr/share on linux which isn't recommended
either.

Which just means that patching to use ${prefix} should be straightforward O:^)


Which of those are using Qt or QStandardPaths though?

Few, and none from the Gnome/GTk universe I presume. Which is exactly my point. 
If they were all using Qt we could do what we want, but as it is we would 
either have to patch all those other apps. Or align ourselves to tried and true 
solutions ...





 R.
 ___
 kde-...@kde.org
 List Information: https://mail.kde.org/mailman/listinfo/kde-mac
 KDE/Mac Information: http://community.kde.org/Mac





___
kde-...@kde.org
List Information: https://mail.kde.org/mailman/listinfo/kde-mac
KDE/Mac Information: http://community.kde.org/Mac

Re: [KDE/Mac] Multiplatform frameworks

On Saturday February 28 2015 18:12:40 Ian Wadham wrote:

One problem is that these are NOT exactly like ~/.local/share, 
/usr/local/share,
~/.local/share/APPNAME and /usr/local/share/APPNAME on Linux.
...
A bundle identifier is something like org.kde.appname.  Actually, on my 
system I find:

Tara:~ls /Library/Application\ Support/
Adobe/ Macromedia/ProApps/   iLife/
Apple/ Microsoft/ Script Editor/ iLifeMediaBrowser/
CrashReporter/ Mozilla/   avbdeviced/iLifeSlideshow/
GarageBand/Oracle/iDVD/  iPhoto/

All are either names of organisations or names of apps from Apple.
...
Another problem is that the /Library/Application Support directory is not 
really geared towards
apps that share data-files with other apps.  Subdirs of GenericDataDir such as 
doc/, kservices5/,
icons/, kxmlgui5/ , mime/ and dbus-1/ would look out-of-place, since they are 
not apps.  Also the
sheer number of KDE apps would tend to crowd out names of other organisation 
and apps.

One way to solve both problems is somehow to inject an organisation-id, such 
as KDE/ or
org.kde/ into QStandardPaths, so that the generic data paths would become 
something like:

I think that's about the ONLY solution. How else are applications going to find 
shared data?
Crowding out isn't the biggest issue, BTW  IMHO, it's the risk that we end up 
overwriting or otherwise interfering with stuff that's not ours but happens to 
be in the way.

But I do not know how or when this could be done.  Clearly, we cannot 
hard-wire that into
the QSP code, because there are other apps that use Qt but do not come from 
the KDE
Community.  It would be easy enough to put it into ECM modules when building 
KF5 or
Frameworks software, but then how would that info be given to QSP within 
*every* KF5
executable, GUI or not-GUI, before the first call to QSP?

Exactly: it'd put us in the same place we're now, whipping up a patch that does 
basically the same thing even if the exact strings (paths) are closer to the 
MacOS Roman thing (hehe, thanks Ian :))

Ideas welcome… :-)

My idea is still that we need a patch that adds a runtime selection between 
Maccy and Linuxy QSPs, controlled through an AA_* flag (cf. Icons in 
menus) or an argument to Q(Core)Application's ctor. The initial setting of 
that switch would be controlled through a preprocessor macro, which we could 
set somewhere in the CMake scripts but that could of course be overridden if 
needed. Multiple levels of control each with a default, I think that gives the 
flexibility we need and also the compliance with Digia's requirements (App 
Store rules...)

Example: KDE4's GUIEnabled argument to the KApplication ctor. It's not used the 
same way on all platforms, and it may not have the same default on all 
platforms either. On OS X it has a function, so I patched the build system and 
headers to link it to the CMake NOGUI token used when defining an executable. 
With my mods, setting that token causes a preprocessor macro to be set. The 
default value of GUIEnabled depends on that preproc. macro: when set, the 
GUIEnabled defaults to False, otherwise to True. The result is that 
applications defined with NOGUI will now be built such that they start as 
non-gui applications without changing a line in their code, unless they already 
happened to specify GUIEnabled themselves.

I think that's what we want, regardless of where we decide to store stuff.
But for that I really think we have to prefer function over form. Apple's 
guidelines are just that, guidelines. They're the first to deviate from them if 
it suits them, or else we end up with contraptions like a complete unix-style 
tree behind a prefix like /Applications/Xcode.app/Contents/Developer/... or 
/Library/Developer/CommandLineTools/ and a bunch of symlinks set up to point in 
there.

To complement Ian's quote: when in Rome, do as the Romans do ... but don't try 
to be more catholic than the pope ;)

Unless you want to ship KDE applications through the App Store, but in a way I 
hope that's never going to happen.

R.

Re: [KDE/Mac] Multiplatform frameworks

On Saturday February 28 2015 22:00:07 Ian Wadham wrote:

Hi Ian,

 Esprit d'escalier.  We could change GenericDataDir in QStandardPaths to be:
 
  ~/Library/Application Support/Qt5, /Library/Application Support/Qt5
 
 That would work for ALL applications that use Qt5 and QSP, regardless of 
 origin,
 and would be a more correct usage of Apple OS X by the Qt 5 libraries.

And then you'd have to duplicate or migrate everything by the time Qt 6 comes 
out ... 
I'd hope that Qt version-specific stuff is already installed by Qt itself in an 
appropriate place that's under configure control (${prefix}/share/qt5 with my 
qt5-mac-devel port).

The other issue is: how is the build system going to decide in which of the 2 
locations above to install stuff?
Has  KF5 done away with a ~/.kde/share equivalent that (partly) overrides 
things in ${prefix}/share?

  Crowding out isn't the biggest issue, BTW  IMHO, it's the risk that we end 
  up overwriting or
  otherwise interfering with stuff that's not ours but happens to be in the 
  way.
 
 I was thinking the same.  I come from an environment where everything had to 
 be uniquified:

And as I said earlier, there's also the question of all the non-KDE/KF5 
applications that use freedesktop/xdg-style data paths that are conceived to be 
shared with KDE/KF5 (icons, themes, styles, the shared-mime database, 
.desktop/.service/dbus files, etc). There are loads of those in MacPorts (and 
Fink, and ...) and I think they're more widely used than KDE applications, so 
we have to factor in their existence. As well as MacPorts' policy to install 
everything in its own prefix (as well as the policies of the other major 
packaging efforts). There are just too many dependencies not to use MacPorts or 
similar, and OS X is different enough from Linux to make rolling our own 
standalone, do-as-the-romans-do distribution scheme a daunting task that I 
won't have anything to do with.


R.

Re: Review Request 121390: make Qt5 theme build on Linux again


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

(Updated March 1, 2015, 10:02 p.m.)


Status
--

This change has been discarded.


Review request for KDE Frameworks, Qt KDE and Yichao Yu.


Repository: qtcurve


Description
---

Yesterday's patches for OS X building broke the build of the Qt5 parts on Linux 
(and other Unix/X11 platforms). I had presumed that Q_WS_X11 would be defined 
in that context as it is when building Qt4 code, but apparently it isn't.

This patch restores building on Unix/X11 by replacing

`#ifdef Q_WS_X11`

with

`#if defined(Q_OS_UNIX)  !defined(Q_OS_OSX)`

please verify if that catches all possible contexts where X11 is to be used?! 
(Qt/Cygwin might use X11?)


Diffs
-

  qt5/style/blurhelper.cpp 5dcc95c 
  qt5/style/qtcurve.cpp 7b0d7e6 
  qt5/style/qtcurve_plugin.cpp febc977 
  qt5/style/qtcurve_utils.cpp 728c26a 
  qt5/style/shadowhelper.cpp a239cf1 
  qt5/style/utils.cpp 0680442 
  qt5/style/windowmanager.cpp 3c2bc1c 

Diff: https://git.reviewboard.kde.org/r/121390/diff/


Testing
---

On KUbuntu 14.04 with Qt 5.3.2 and KF5 in the (sadly discontinued) Project 
Neon5 environment.


Thanks,

René J.V. Bertin

Re: [KDE/Mac] Multiplatform frameworks

2015-02-26 Thread René J . V . Bertin

On Wednesday February 25 2015 19:10:08 Jeremy Whiting wrote:

QStandardPaths there that worked pretty well. In discussion with the Qt
developers I began to think that we maybe should be installing our data
files in the places that QStandardPaths expect to find them, rather than
get QStandardPaths to find files in linuxy paths.

Even if that were the easy way out, I don't think it's the proper solution if 
not only because OS X and MS Windows are multi-user machines and are maybe more 
often used like that than Linux desktop installs. Installing stuff in 
$HOME/Library/Application Support is thus not an option (besides, there's that 
obnoxious space in the filename that's bound to cause issues).

If we can't find a best-of-both-worlds solution that we all agree on and can go 
into Qt, we'll just have to roll our own (which might be incorporated after all 
once it's proved its value ;))

Reminder to self: add my views to wherever we decided to continue the stalled 
discussion from gerrit.

R

Re: Review Request 122404: [OS X]: draft support for audio in the IOKit backend



 On Feb. 3, 2015, 1:57 p.m., Harald Sitter wrote:
  I really think this code needs to be in phononserver or better yet Phonon 
  itself (akin to the pulsesupport) or even better in VLC.
  
  The solid audiointerface stuff is gone in frameworks5, so ultimately this 
  is a dead end. Equally phononserver is bound to go away at some point in 
  the future and being replaced by listing exclusively what phonon reports 
  (which in turn is pulseaudio listing || backend listing). So, enabling VLC 
  to expose this information through the libvlc_audio_output_device 
  enumerable would be the best course of action. If the VLC devs don't want 
  that or whatever, then putting it in Phonon seems the most useful approach.

To be honest, that would suit me perfectly. I'm not quite motivated to invest 
in learning IOKit at the moment, and Solid even less.

Pulseaudio is a bit problematic on OS X currently, and I've seen many 
complaints about it (e.g. concerning delays that I can confirm on my Linux 
systems where event notifications tend to sound long after the event).
So the best course of action might yet be to figure out why the vlc backend 
isn't in MacPorts.

Is phonon-backend-vlc still/indeed the preferred backend for KDE/KF5?


- René J.V.


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


On Feb. 3, 2015, 1:44 p.m., René J.V. Bertin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122404/
 ---
 
 (Updated Feb. 3, 2015, 1:44 p.m.)
 
 
 Review request for KDE Software on Mac OS X, kdelibs, Phonon, and Solid.
 
 
 Repository: kdelibs
 
 
 Description
 ---
 
 While trying to bring a functional Multimedia kcm to OS X (see 
 https://git.reviewboard.kde.org/r/122403/), I realised that Solid's IOKit 
 backend doesn't currently support audio devices.
 
 This RR outlines the current state of my draft attempt to provide that 
 support. I'm handicapped by my lack of experience with both IOKit and Solid, 
 and would thus appreciate a helping hand.
 
 I'm currently at a stage where Solid returns a list representing the audio 
 devices present in my system, but apparently not with the correct metadata 
 for phononserver to consider them, despite the fact that I pretend all 
 devices to be pluripotent (i.e. AUDIOCONTROL|AUDIOINPUT|AUDIOOUTPUT) :
 
 ```
 kded(81671)/phonon (kded module) PhononServer::findDevices: Solid offers 5 
 audio devices
 kded(81671)/phonon (kded module) PhononServer::findDevices: Solid offers 0 
 video devices
 ...
 kded(81671)/phonon (kded module) PhononServer::findDevices: groups: ()
 kded(81671)/phonon (kded module) PhononServer::findDevices: already found 
 devices: QSet()
 kded(81671)/phonon (kded module) PhononServer::findDevices: Audio Playback 
 Devices: ()
 kded(81671)/phonon (kded module) PhononServer::findDevices: Audio Capture 
 Devices: ()
 kded(81671)/phonon (kded module) PhononServer::findDevices: Video Capture 
 Devices: ()
 kded(81671)/kded4 *Kded::loadModule: Successfully loaded module phononserver
 ```
 
 I'll add a more complete log as an attachment.
 
 
 Diffs
 -
 
   solid/solid/CMakeLists.txt a7f1f07 
   solid/solid/backends/iokit/iokitdevice.cpp f113c16 
   solid/solid/backends/iokit/iokitmanager.cpp fcd748e 
   solid/solid/devicemanager.cpp a465169 
 
 Diff: https://git.reviewboard.kde.org/r/122404/diff/
 
 
 Testing
 ---
 
 On OS X 10.9.5/MacPorts with kdelibs 4.14.4 (git/master), kde4-runtime 
 (git/master) and kde4-workspace (git/master); phonon 4.8.3 with 
 phonon-backend-gstreamer 4.8.2 .
 
 
 File Attachments
 
 
 full trace with debug output
   
 https://git.reviewboard.kde.org/media/uploaded/files/2015/02/03/67a59590-84a4-4d30-88aa-332d9f068a2c__Solid-20150203.log
 
 
 Thanks,
 
 René J.V. Bertin

Review Request 122403: [OS X] adaptations to phonon component.


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

Review request for KDE Software on Mac OS X, KDE Runtime, Phonon, and Solid.


Repository: kde-runtime


Description
---

I'm trying to make the kcm_phonon Multimedia control panel more useful on OS X, 
so that ideally users will be able to choose different devices for the 
different playback (or recording) categories.

This RR outlines the current state of my draft attempt to provide that support. 
I'm handicapped by my lack of experience with both IOKit and Solid, and would 
thus appreciate a helping hand.

I'm currently at a stage where Solid returns a list representing the audio 
devices present in my system, but apparently not with the correct metadata for 
phononserver to consider them:

```
kded(81671)/phonon (kded module) PhononServer::findDevices: Solid offers 5 
audio devices
kded(81671)/phonon (kded module) PhononServer::findDevices: Solid offers 0 
video devices
...
kded(81671)/phonon (kded module) PhononServer::findDevices: groups: ()
kded(81671)/phonon (kded module) PhononServer::findDevices: already found 
devices: QSet()
kded(81671)/phonon (kded module) PhononServer::findDevices: Audio Playback 
Devices: ()
kded(81671)/phonon (kded module) PhononServer::findDevices: Audio Capture 
Devices: ()
kded(81671)/phonon (kded module) PhononServer::findDevices: Video Capture 
Devices: ()
kded(81671)/kded4 *Kded::loadModule: Successfully loaded module phononserver
```

Companion RR for the (required) changes to kdelibs:solid which is probably 
where most of the work will have to be done : 
https://git.reviewboard.kde.org/r/122404/ . That RR also has a full output log 
showing the available information about my devices.


Diffs
-

  phonon/CMakeLists.txt b6ba4ec 
  phonon/kcm/audiosetup.cpp 835799c 
  phonon/kded-module/hardwaredatabase e353eea 
  phonon/kded-module/phononserver.cpp 9a47423 

Diff: https://git.reviewboard.kde.org/r/122403/diff/


Testing
---

On OS X 10.9.5/MacPorts with kdelibs 4.14.4 (git/master), kde4-runtime 
(git/master) and kde4-workspace (git/master).


Thanks,

René J.V. Bertin

Re: Review Request 122404: [OS X]: draft support for audio in the IOKit backend


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

(Updated Feb. 3, 2015, 1:43 p.m.)


Review request for KDE Software on Mac OS X, kdelibs, Phonon, and Solid.


Repository: kdelibs


Description (updated)
---

While trying to bring a functional Multimedia kcm to OS X (see 
https://git.reviewboard.kde.org/r/122403/), I realised that Solid's IOKit 
backend doesn't currently support audio devices.

This RR outlines the current state of my draft attempt to provide that support. 
I'm handicapped by my lack of experience with both IOKit and Solid, and would 
thus appreciate a helping hand.

I'm currently at a stage where Solid returns a list representing the audio 
devices present in my system, but apparently not with the correct metadata for 
phononserver to consider them, despite the fact that I pretend all devices to 
be pluripotent (i.e. AUDIOCONTROL|AUDIOINPUT|AUDIOOUTPUT) :

```
kded(81671)/phonon (kded module) PhononServer::findDevices: Solid offers 5 
audio devices
kded(81671)/phonon (kded module) PhononServer::findDevices: Solid offers 0 
video devices
...
kded(81671)/phonon (kded module) PhononServer::findDevices: groups: ()
kded(81671)/phonon (kded module) PhononServer::findDevices: already found 
devices: QSet()
kded(81671)/phonon (kded module) PhononServer::findDevices: Audio Playback 
Devices: ()
kded(81671)/phonon (kded module) PhononServer::findDevices: Audio Capture 
Devices: ()
kded(81671)/phonon (kded module) PhononServer::findDevices: Video Capture 
Devices: ()
kded(81671)/kded4 *Kded::loadModule: Successfully loaded module phononserver
```

I'll add a more complete log as an attachment.


Diffs
-

  solid/solid/CMakeLists.txt a7f1f07 
  solid/solid/backends/iokit/iokitdevice.cpp f113c16 
  solid/solid/backends/iokit/iokitmanager.cpp fcd748e 
  solid/solid/devicemanager.cpp a465169 

Diff: https://git.reviewboard.kde.org/r/122404/diff/


Testing
---

On OS X 10.9.5/MacPorts with kdelibs 4.14.4 (git/master), kde4-runtime 
(git/master) and kde4-workspace (git/master).


File Attachments


full trace with debug output
  
https://git.reviewboard.kde.org/media/uploaded/files/2015/02/03/67a59590-84a4-4d30-88aa-332d9f068a2c__Solid-20150203.log


Thanks,

René J.V. Bertin

Re: Review Request 122404: [OS X]: draft support for audio in the IOKit backend