Setting to reposition cursor.#685
Conversation
|
Hey, Thanks for this PR! I had a quick look to your changes and I had some questions: The first one is, why making Also, do you see a lag checking the value of Finally, and I see this as a blocker, what about touchpads? Unlike touchscreens, the absolute position of the fingers is not available. I think I already mentioned the last reason in a couple of bug reports. It is a pity that it is so difficult to get it right, but unfortunately it is. Honestly, I think that it'd be better to implement this feature in your window manager that doing it here, the result will be way better. |
|
You're welcome. Thank you for making the rest of the program. :) Yes, I agree that it would be best to make this feature available on a per-gesture-definition basis. I forget exactly why I made it global but I probably intended to change it later if you liked the idea. This may also address the issue of reloading the client when the config file changes. I will give it a try. I did not notice any lag from any or all of these changes, but this is ultimately dependent on the user's update event frequency if they have this feature set to include GESTURE_UPDATE. I'm not sure when the default rate is or if there is a reasonable maximum we could assume. I also haven't run any execution time profiling on the call graph. It just feels wrong to thrash the settings map. I think it would be fine to make this setting available only for touchscreens. That shouldn't be confusing so long as it's documented. I actually think repositioning the cursor after touchpad gestures would be less intuitive and probably less commonly desirable anyway. Maybe support could be added later if people really want it? I'm not sure how to get the mouse speed, resolution, and acceleration factors but it's probably possible somehow. Even if the measurements aren't pixel perfect, that might be ok if the cursor is being repositioned with every gesture update event, because the margin for error would not compound as much over a shorter distance. The best place for this feature might be LibInput itself, but I don't know if that would be possible and functionality like per-gesture configuration would definitely be impossible that way. I do think it makes more sense in Touchegg than in every window manager though, because the code is more centralised and window managers don't usually offer the sort of gesture configuration that Touchegg does anyway. I don't think this code would be much different in a window manager-- the window manager would still need to use something like LibInput and Xlib. The only advantage I can think of is that it would have already cached the window positions and dimensions, but the code in this P.R. doesn't need that. |
You could read the settings on gesture begin and store the value. This allows to read settings once and use the same value during the entire gesture. I think that all actions are implemented this way.
libinput is a generic library used by compositors and it is up to the compositor to implement specific behaviors, in this case, move or not the pointer. In Wayland, every compositor implements its own logic, even if that means duplicating efforts across compositors. The reason is that the compositor is the only one that has all the information. By the way, if this is your Compiz plugin: The source is here: I think it'll be easier to add touch-screen support there rather than hacking it in Touchégg. After a quick look, I think that this MotionNotify event is what handles the mouse movement: The docs:
Sometimes, it is not possible to provide a good user experience building something generic and it is required to build a plugin for the desktop environment. For example, I had to build a GNOME plugin because otherwise the end result won't be the same: I'm still reticent to add this feature because of the lack of trackpad support and because users will try to use this to drag and drop, and with more than 1 finger, drag and drop is not precise. But, while I think you should write your own DE plugin, if you manage to implement this in a way that D-Bus is compatible with old clients, only for actions where it makes sense (probably only mouse click) and add it to the GUI, I'll be happy to merge it. |
|
Ok. I haven't looked at the GUI at all yet but I will take a look at that once the rest is done. I will try adjusting the documentation too, so you can see how it might look for users. I am not very familiar with D-Bus. Have you thought about how you would like to implement A.P.I. version control? I did add some simple versioning to my other P.R. (#671) but I don't know if that is the best approach. It's frustrating that D-Bus does not yet seem to support nullable types. I don't actually use Compiz myself these days. My use-case is essentially the same though. My window manager, i3, supports dragging to reposition windows on the current workspace by holding a modifier (in my case, the Super key). There is a visual indication of where the window would be placed while the cursor is moved around, which is why I need this feature to support GESTURE_UPDATE. I imagine there are other desktop environments that would benefit from this feature for something similar. |
No, I have no idea how to add new fields, I'd have to experiment a bit. I hope that just add them at the end works, but I don't know if that's the case without testing it. |
|
Adding new fields is actually pretty easy. I can set that up as a separate P.R. if that's how you want to do it. The way I did it in the other P.R. I have open was by adding a separate interface for the new A.P.I. There might be better ways of doing it but this seemed to work. You can see that commit here. It seemed to work as expected in my testing but you should definitely test it yourself to make sure. The simplest approach seems to be using a different interface for every version of the A.P.I. I don't know if this is how D-Bus versioning is normally done and there could be problems with this approach that I'm not aware of, since I'm not very familiar with D-Bus. The only disadvantage I'm aware of is that the dbus.h file would end up collecting more interface definitions over time, whenever the A.P.I. is changed. Of course, old definitions could be removed when support is no longer needed. This feels a bit messy but it seems like the best approach from what I've seen. Another option might be to add a version field to the A.P.I., maybe based on the Touchegg version or epoch. I'm not sure how much that would actually help reduce the need for new interfaces when the A.P.I. changes in the future though. My impression is that most changes would probably still require a new interface. There are some ways of extending the feature set of an interface without breaking compatibility, like I did here, but it is probably more efficient to simply update the interface and avoid that conditional logic. Also, as I said earlier, null data types are not yet supported, so this approach would sometimes require transmitting unnecessary data. Here are some links about D-Bus version control. I'm not confident enough to say whether they represent the current best practice but I have not found any information that suggests otherwise. |
I'm mostly worried about old clients not working after those new fields are added. But it can be easily tested.
It is fine if you keep it here. I'd suggest to split your work in little commits that compile and make sense on their own. For more information: |
|
Ok, I'll take a look at those links and re-work this P.R. when I have a chance. I've been conflicted about whether this setting should be per-action or per-gesture. I looked at the code again and adding support per action would certainly be easier but it feels less intuitive to me. I suppose that's because I think of this setting as expressing simply whether the cursor should follow the fingers (gesture), irrespective of the gesture's effect. User preference seems more likely to depend on how they're moving their fingers than what action the movement is supposed to cause. What do you think?
Extending the A.P.I. might be inherently backward compatible in some cases but my approach has been to add a separate interface that is detected and preferred by clients that know of it. I believe I tested this by reverting all my changes on the client side and not the server side. My other P.R. is a bit simpler so I would suggest testing it there first. Incidentally, I noticed that some of the data types declared for D-Bus do not match those used by the program. This may not matter but it would be easy to correct with any update to the interface. |
2 participants
This adds a global setting "reposition_cursor", which can be set to:
The reason I made these changes is to allow for drag-like effects, such as #665.
Code style not yet fully compliant with guidelines (pending #684).
This currently adds two doubles to the interface regardless of whether the setting is enabled. D-Bus does not currently support nullable data types, although there are work-arounds. Another option would be adding a separate signal definition for use depending on this setting's value or adding a new signal specifically for cursor position.
The setting is currently cached (only) when the gesture controller is instantiated. This means changing its value currently requires the client to be restarted. The obvious fix would be to not cache the setting but I think it would be nice to avoid potentially high frequency map lookups (depending on the gesture update event poll rate). One option would be to have the config system emit a D-Bus signal, but I don't know if you like that idea. Perhaps there are some simpler approaches I haven't thought of.