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

ploopyco: Expose toggle scrolling as a custom keycode #23728

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

beaudrychase
Copy link

@beaudrychase beaudrychase commented May 16, 2024

Description

The goal of this PR is to allow users to create keymaps with both momentary and toggle control over drag scroll by mapping these functions to two separate custom keycodes. This will allow people to switch between momentary and toggle control of drag scroll through VIA, instead of having to flash new firmware with different macros set.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@drashna
Copy link
Member

drashna commented May 18, 2024

Just a heads up, develop has a pretty massive rework of all of the ploopy code, to unify it into a more cohesive base. (#22967) As such, the files that you're editing no longer exist on that branch, and this change will cause merge conflicts.

I would recommend rebasing this to target develop instead.

@drashna drashna self-assigned this May 18, 2024
@drashna drashna requested review from drashna and a team May 18, 2024 06:31
@beaudrychase
Copy link
Author

Oh sick, I hadn't seem that PR. Honestly, this was a quick and dirty PR for me to get the functionality I wanted and something to link to in this issue. I can for sure clean this up and rebase on top of that commit you linked.

@beaudrychase beaudrychase changed the base branch from master to develop May 18, 2024 18:44
@beaudrychase beaudrychase force-pushed the madromys-toggle-scroll-via branch from f09a037 to 5c08ade Compare May 18, 2024 19:48
@beaudrychase beaudrychase marked this pull request as draft May 18, 2024 20:14
@beaudrychase beaudrychase marked this pull request as ready for review May 19, 2024 01:49
@beaudrychase beaudrychase force-pushed the madromys-toggle-scroll-via branch 2 times, most recently from ddcf04f to e6d5b5d Compare May 19, 2024 02:22
@beaudrychase beaudrychase changed the title Exposed toggle scrolling as a key in via Expose toggle scrolling as a custom keycode May 19, 2024
keyboards/ploopyco/ploopyco.c Outdated Show resolved Hide resolved
@drashna drashna requested a review from a team May 19, 2024 04:33
@github-actions github-actions bot added keymap via Adds via keymap and/or updates keyboard for via support labels May 19, 2024
@beaudrychase beaudrychase requested a review from drashna May 19, 2024 12:57
@beaudrychase beaudrychase changed the title Expose toggle scrolling as a custom keycode ploopyco: Expose toggle scrolling as a custom keycode May 19, 2024
@beaudrychase beaudrychase force-pushed the madromys-toggle-scroll-via branch from 0ce8dc5 to c6bd882 Compare May 20, 2024 03:02
@beaudrychase
Copy link
Author

This is my first time contributing to this project; is there anything I need to do to get this merged in? @drashna I also think this PR is in a done state. @drashna thank you so much for your work on the Ploopy firmware and for taking the time to review this PR.

@drashna drashna requested a review from a team May 22, 2024 03:06
@drashna
Copy link
Member

drashna commented May 22, 2024

Generally, we want two approvals from collaborators or other exports for the PR before it's merged.

So, it's mostly a waiting game. And usually, it can take a couple of weeks. There are a lot of PRs, and we all do this in our free time.

@beaudrychase
Copy link
Author

That's a sensible policy. Thank you again for all of your efforts!

@@ -175,7 +175,7 @@ bool process_record_kb(uint16_t keycode, keyrecord_t* record) {
}

if (keycode == MOMENTARY_DRAG_SCROLL) {
is_drag_scroll = record->event.pressed;
is_drag_scroll ^= 1
Copy link
Author

@beaudrychase beaudrychase May 25, 2024

Choose a reason for hiding this comment

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

This was a subtle thing I wanted to change.
Behavior before this commit:

  1. Press TOGGLE_DRAG_SCROLL and enter drag scroll mode.
  2. Press and hold MOMENTARY_DRAG_SCROLL leaves drag scroll mode and enters mouse mode.
  3. Release MOMENTARY_DRAG_SCROLL does nothing, the device stays in mouse mode instead of going back into drag scroll.

Behavior after this commit:

  1. Press TOGGLE_DRAG_SCROLL and enter drag scroll mode.
  2. Press and hold MOMENTARY_DRAG_SCROLL temporarily leaves drag scroll mode and enters mouse mode.
  3. Release MOMENTARY_DRAG_SCROLL returns device to drag scroll mode
  4. Press TOGGLE_DRAG_SCROLL to leave drag scroll mode.

I'm using my Adept with a button mapped to each of these keys and I think the new behavior is much more intuitive.

@beaudrychase
Copy link
Author

I just read this PR: #21426
It seems like my work here won't be needed once that one is merged in since ploopyco won't have to implement drag scroll. That PR also exposes momentary and toggle versions of drag scroll as keycodes. These ploopyco will just need to define POITNING_DEVICE_MODES_ENABLE. I'd be cool with just closing this PR, it's up to y'all.

@light-idea
Copy link

light-idea commented Aug 11, 2024

I was just working on something similar to add custom keycodes that invert the scroll direction (for switching between Mac and Windows). Thanks for the info on the new pointing device modes.

If this does get merged, you may want to swap the momentary and toggle keycodes in ploopyco.h, so that the old and new firmware use the same keycode for toggle. It might also make sense to increment the VIA firmware version, if that would allow VIA to detect between the old and new firmware features.

@@ -18,6 +18,8 @@

#pragma once

#define VIA_FIRMWARE_VERSION 1
Copy link
Author

Choose a reason for hiding this comment

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

@light-idea Thanks for your suggestion to version bump VIA. Does this have the intended effect? I followed the commit you linked in your comment.

I was also wondering about your choice of config.h. You chose to modify .../ploopyco/trackball/rev1_005/config.h instead of .../ploopyco/trackball/config.h. I think defining it in either should have the intended effect. What considerations are there with this?

Choose a reason for hiding this comment

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

Looking it over again, it's probably supposed to go in .../keymaps/via/config.h.

But it does not seem to have the intended effect. I was hoping VIA would only show the custom keycodes supported by the current firmware version. But if I load a draft definition into VIA, it always shows the the newest firmware keycodes no matter what I have loaded on the trackball...

Comment on lines +35 to +36
TOGGLE_DRAG_SCROLL,
MOMENTARY_DRAG_SCROLL,
Copy link
Author

Choose a reason for hiding this comment

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

@light-idea good catch with the ordering of these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review keyboard keymap via Adds via keymap and/or updates keyboard for via support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants