-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable touchpad inertial scrolling #2364
Conversation
Thanks, can you make changes based on devel branch? |
Okay, I've rebased my codes. The falling CI check is code style, I'll fix it quickly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me, with some minor comments. @metalefty?
@@ -1230,6 +1230,29 @@ xrdp_wm_clear_popup(struct xrdp_wm *self) | |||
return 0; | |||
} | |||
|
|||
/*****************************************************************************/ | |||
int | |||
xrdp_wm_mouse_touch(struct xrdp_wm *self, int gesture, int param) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest macros or an enumerated type for the gesture
parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'm hesitated here because I don't know the best place to put the macros. Should I put them in the 'xrdp_constant.h'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since xrdp_wm_mouse_touch()
is a local function only used in this file I'd add them directly above the definition of teh function. Also, the function could be a static int
rather than an int
, or have I missed something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tought it's the same as xrdp_wm_mouse_click()
and xrdp_wm_key()
. Make a decleration in 'xrdp.h' or keep it a local method? I prefer making a deleration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine too - it's at least consistent with the other functions.
if (device_flags & PTRFLAGS_WHEEL_NEGATIVE) | ||
{ | ||
xrdp_wm_mouse_click(self, 0, 0, 5, 0); | ||
// [MS-RDPBCGR] In negative scrolling, rotation distance is negative. | ||
delta = (device_flags & WheelRotationMask) | ~WheelRotationMask; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a reference to 2.2.8.1.1.3.1.1.3 here? It took me a while to figure out what line 1805 was doing and I needed the reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌I'll add more comments and the reference link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some additional changes, I'll have a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. done.
Overall, LGTM. BTW, what do you think making a macro for delta calculation? |
@metalefty The delta calculation is not quite complex and reuseful at present. I suggest not using macros. |
Okay, approved. Thanks for your work! |
@seflerZ - my apologies. I've just merged a PR which conflicts slightly with yours. Can you rebase and we'll merge this. It might be worth squashing too - it's not a big PR. I'll leave that up to you. |
@matt335672 It's ok. It would be a quick rebase. Done. |
Merged. @seflerZ - thanks for your contribution |
See this PR
@metalefty