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

Add continuous scrolling speed adjustment #1649

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

FireChickenProductivity
Copy link
Contributor

Add the ability to adjust continuous scrolling speed while scrolling by dictating a number_small. The speed becomes the speed setting multiplied by the dictated number divided by ten. The acceleration gets reset every time the speed gets changed. The scrolling speed reverts to the default after the current scroll is finished.
closes #1648

@FireChickenProductivity
Copy link
Contributor Author

I added the ability to set the scrolling speed when starting continuous scrolling.

plugin/mouse/mouse_scroll.py Outdated Show resolved Hide resolved
plugin/mouse/mouse_scroll.py Outdated Show resolved Hide resolved
plugin/mouse/mouse.talon Outdated Show resolved Hide resolved
@FireChickenProductivity
Copy link
Contributor Author

I found a bug that should be resolved before merge.

@FireChickenProductivity
Copy link
Contributor Author

Actually, the "issue" seems to happen without my pull request. Is it intended for some reason for using the continuous scrolling action twice in the same direction to stop the scrolling completely?

@AndreasArvidsson
Copy link
Collaborator

Yes that is on purpose. For people using a foot switch, noise or other to toggle the scrolling on and off.

@FireChickenProductivity FireChickenProductivity deleted the adjustable-scrolling-speed branch January 2, 2025 22:08
@FireChickenProductivity FireChickenProductivity restored the adjustable-scrolling-speed branch January 2, 2025 22:08
@AndreasArvidsson
Copy link
Collaborator

@knausj85 Will test this. If someone else has time before that please go ahead.

@@ -181,13 +206,17 @@ def mouse_scroll_continuous(new_scroll_dir: Literal[-1, 1]):
continuous_scroll_mode = "scroll down continuous"
Copy link
Member

Choose a reason for hiding this comment

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

As written, I believe this will always display "scroll down continuous"

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 tested that and can confirm, but I am not sure if I should change that here if the preference is for atomic pull requests.

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 fixed that.

@@ -0,0 +1,3 @@
tag: user.continuous_scrolling
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to ensure this coexists well with the tag user.numbers_unprefixed. Since the talon files have the same specificity, it's ambiguous which one talon will choose. Probably just add not tag: user.continuous_scrolling to that file

Copy link
Collaborator

@lunixbochs lunixbochs Jan 12, 2025

Choose a reason for hiding this comment

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

specificity doesn't work on commands right now besides exact matches. it's pretty complicated to factor in .talon specificity in the command parser. action-level overrides are much easier to reason about

Copy link
Member

Choose a reason for hiding this comment

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

In this case, the commands are not an exact match. unprefixed numbers use a different capture, so I don't think an action really nets us anything in this context either.


from talon import Context, Module, actions, app, cron, ctrl, imgui, settings, ui

DEFAULT_CONTINUOUS_SCROLLING_SPEED_FACTOR: int = 10
Copy link
Member

Choose a reason for hiding this comment

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

I'm inclined to make this a setting. Any reason not to do so?

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 can make that change.

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 made it a setting.

@knausj85
Copy link
Member

Actually, the "issue" seems to happen without my pull request. Is it intended for some reason for using the continuous scrolling action twice in the same direction to stop the scrolling completely?

I initially thought this was a bug too; these commands used to be compatible with repeaters and such. I guess I missed that memo!

If needed, we can explore how to make these more friendly for hotkeys / pedals in a separate issue

I tested this a bit and it's a great improvement. I've a few minor suggestions before we merge.

@knausj85
Copy link
Member

Actually, the "issue" seems to happen without my pull request. Is it intended for some reason for using the continuous scrolling action twice in the same direction to stop the scrolling completely?

There is a separate few bugs with this logic though that we might as well fix while we're here:

  • the imgui should probably close & tag be cleared when this happens.

I took a quick swing at fixing that.

@FireChickenProductivity
Copy link
Contributor Author

I identified an issue where if the scrolling speed becomes less than one but non-zero the result is no scrolling. Should I address that by rounding up? (This can happen when trying to set the scrolling speed to a small number)

@FireChickenProductivity
Copy link
Contributor Author

It looks like float values for the scrolling speed get truncated, so changing the scrolling speed from some values to certain other values through voice commands has no effect. I am not sure if we should try to address that to make increasing speed value always increase the actual scrolling speed, but the issue is worth noting.

@@ -0,0 +1,3 @@
tag: user.continuous_scrolling
-
<number_small>: user.mouse_scroll_set_speed(number_small)
Copy link
Collaborator

Choose a reason for hiding this comment

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

On the community backlog session we recommended that you add a graphical display of the presence of this feature.

@@ -115,10 +127,12 @@ def mouse_gaze_scroll_toggle():

def mouse_scroll_stop() -> bool:
"""Stops scrolling"""
global scroll_job, gaze_job, continuous_scroll_mode, control_mouse_forced
global scroll_job, gaze_job, continuous_scroll_mode, control_mouse_forced, continuous_scrolling_speed_factor, ctx
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need ctx here unless you're writing directly to it.


def mouse_is_continuous_scrolling():
"""Returns whether continuous scroll is in progress"""
if continuous_scroll_mode:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider return continuous_scroll_mode != ""

new_scroll_dir: Literal[-1, 1],
speed_factor: Optional[int] = None,
):
global scroll_job, scroll_dir, scroll_start_ts, ctx
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove ctx as above

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.

implement scrolling speed adjustment commands
5 participants