Skip to content
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

Merged
merged 5 commits into from
Sep 16, 2022
Merged

Conversation

seflerZ
Copy link
Contributor

@seflerZ seflerZ commented Sep 12, 2022

See this PR

@metalefty

@metalefty
Copy link
Member

Thanks, can you make changes based on devel branch?

@seflerZ
Copy link
Contributor Author

seflerZ commented Sep 13, 2022

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.

Copy link
Member

@matt335672 matt335672 left a 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)
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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?

Copy link
Contributor Author

@seflerZ seflerZ Sep 14, 2022

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.

Copy link
Member

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;
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. done.

@metalefty
Copy link
Member

Overall, LGTM.

BTW, what do you think making a macro for delta calculation?

@seflerZ
Copy link
Contributor Author

seflerZ commented Sep 15, 2022

@metalefty The delta calculation is not quite complex and reuseful at present. I suggest not using macros.

@metalefty
Copy link
Member

Okay, approved. Thanks for your work!

@matt335672
Copy link
Member

@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.

@seflerZ
Copy link
Contributor Author

seflerZ commented Sep 15, 2022

@matt335672 It's ok. It would be a quick rebase. Done.

@matt335672 matt335672 merged commit e169787 into neutrinolabs:devel Sep 16, 2022
@matt335672
Copy link
Member

Merged.

@seflerZ - thanks for your contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants