DevHeads.net

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

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: <a href="https://git.reviewboard.kde.org/r/125043/diff/" title="https://git.reviewboard.kde.org/r/125043/diff/">https://git.reviewboard.kde.org/r/125043/diff/</a>

Testing
For now only on OS X with kdelibs 4.14.11 .

Thanks,

René J.V. Bertin

Comments

Re: Review Request 125043: expose the WheelMouseZooms global set

By =?utf-8?Q?Ren=C... at 09/04/2015 - 10:22

(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: <a href="https://git.reviewboard.kde.org/r/125043/diff/" title="https://git.reviewboard.kde.org/r/125043/diff/">https://git.reviewboard.kde.org/r/125043/diff/</a>

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 set

By Marco Martin at 09/04/2015 - 09:39

hmm, looks like a feature in a now frozen repo, not sure is a good practiche..

- Marco Martin

On Sept. 4, 2015, 11:51 a.m., René J.V. Bertin wrote:

Re: Review Request 125043: expose the WheelMouseZooms global set

By Martin =?ISO-88... at 09/04/2015 - 09:38

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.

- Martin Gräßlin

On Sept. 4, 2015, 1:51 p.m., René J.V. Bertin wrote:

Re: Review Request 125043: expose the WheelMouseZooms global set

By =?utf-8?Q?Thoma... at 09/04/2015 - 16:04

git cherry-pick <some_hash>

"git log --oneline renes_fixes" prints you a list of commits in the "renes_fixes" branch

123456 expose WheelMouseZooms

You *can* use this to eg. grep for a commit message and pass the matching hash to git cherry-pick, but that's of course not unambigious (that is why there are the hashes)

And yes: they can use quickgit.kde.org to click their way to download a particular patch - if you meant that.

- Thomas

On Sept. 4, 2015, 2:22 nachm., René J.V. Bertin wrote:

Re: Review Request 125043: expose the WheelMouseZooms global set

By =?utf-8?Q?Ren=C... at 09/04/2015 - 15:47

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.

On Sept. 4, 2015, 4:22 p.m., René J.V. Bertin wrote:

Re: Review Request 125043: expose the WheelMouseZooms global set

By Martin Klapetek at 09/04/2015 - 15:05

Sure it does. Here is a random git commit from plasma-workspace -- <a href="https://quickgit.kde.org/?p=plasma-workspace.git&amp;a=commit&amp;h=e2df9af6e2df4ba1c07dc40d43ecd591584db498" title="https://quickgit.kde.org/?p=plasma-workspace.git&amp;a=commit&amp;h=e2df9af6e2df4ba1c07dc40d43ecd591584db498">https://quickgit.kde.org/?p=plasma-workspace.git&amp;a=commit&amp;h=e2df9af6e2df...</a> - 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"

- Martin

On Sept. 4, 2015, 4:22 p.m., René J.V. Bertin wrote:

Re: Review Request 125043: expose the WheelMouseZooms global set

By =?utf-8?Q?Ren=C... at 09/04/2015 - 14:47

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.

On Sept. 4, 2015, 4:22 p.m., René J.V. Bertin wrote:

Re: Review Request 125043: expose the WheelMouseZooms global set

By Luigi Toscano at 09/04/2015 - 12:14

Exactly, a separate repository with patches does not make sense (git already manages patches).

- Luigi

On Set. 4, 2015, 4:22 p.m., René J.V. Bertin wrote:

Re: Review Request 125043: expose the WheelMouseZooms global set

By Jeremy Paul Whiting at 09/04/2015 - 12:10

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.

- Jeremy

On Sept. 4, 2015, 8:22 a.m., René J.V. Bertin wrote:

Re: Review Request 125043: expose the WheelMouseZooms global set

By =?utf-8?Q?Ren=C... at 09/04/2015 - 12:04

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.

On Sept. 4, 2015, 4:22 p.m., René J.V. Bertin wrote:

Re: Review Request 125043: expose the WheelMouseZooms global set

By Luigi Toscano at 09/04/2015 - 10:43

I disagree: a separate branch makes definitely more sense than a separate repository (which would lead more confusion and divide the code).

- Luigi

On Set. 4, 2015, 4:22 p.m., René J.V. Bertin wrote:

Re: Review Request 125043: expose the WheelMouseZooms global set

By =?utf-8?Q?Ren=C... at 09/04/2015 - 10:40

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.

On Sept. 4, 2015, 4:22 p.m., René J.V. Bertin wrote:

Re: Review Request 125043: expose the WheelMouseZooms global set

By =?utf-8?Q?Thoma... at 09/04/2015 - 09:21

kcontrol/input/kmousedlg.ui (line 82)
<https://git.reviewboard.kde.org/r/125043/#comment58702>

You *must* ask the i18n team about this (requires an exception) on <a href="mailto:kde- ... at kde dot org">kde- ... at kde dot org</a>

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)

kcontrol/input/mouse.cpp (line 184)
<https://git.reviewboard.kde.org/r/125043/#comment58701>

read this line closely, then fix it ;-P

- Thomas Lübking

On Sept. 4, 2015, 11:51 vorm., René J.V. Bertin wrote:

Re: Review Request 125043: expose the WheelMouseZooms global set

By =?utf-8?Q?Ren=C... at 09/04/2015 - 10:40

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

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.

On Sept. 4, 2015, 4:22 p.m., René J.V. Bertin wrote:

Re: Review Request 125043: expose the WheelMouseZooms global set

By Luigi Toscano at 09/04/2015 - 09:41

kde-workspace 4.14? It's still tracked on our i18n branches, but given that the last release is out (see <a href="https://mail.kde.org/pipermail/release-team/2015-August/008835.html" title="https://mail.kde.org/pipermail/release-team/2015-August/008835.html">https://mail.kde.org/pipermail/release-team/2015-August/008835.html</a>), I think we will stop tracking that branch soon. (PS: the list is <a href="mailto:kde-i18n- ... at kde dot org">kde-i18n- ... at kde dot org</a>). This is quite dead code at this point...

- Luigi

On Set. 4, 2015, 1:51 p.m., René J.V. Bertin wrote:

Re: Review Request 125043: expose the WheelMouseZooms global set

By =?utf-8?Q?Ren=C... at 09/04/2015 - 07:53

kcontrol/input/mouse.cpp (lines 183 - 185)
<https://git.reviewboard.kde.org/r/125043/#comment58700>

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: