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 keyboard support #80

Merged
merged 1 commit into from
May 9, 2024

Conversation

evant
Copy link

@evant evant commented Mar 28, 2024

Intial stab at this, I'm not sure all the code lives in the right place and looking for feedback on that.

Additional issues/questions:

  • Is there a way to get the 'standard' system keyboard shortcuts for this?
  • How to detect ctrl/meta + key press? I never saw them detected though I'm testing with the android emulator which does weird things with external keyboards.
  • keyboard input enabled by default? note: view needs to be focused to start receiving events
  • configure zoom & pan steps?

Fixes #78

Intial stab at this, I'm not sure all the code lives in the right place and looking for feedback on that.

Additional issues/questions:

- Is there a way to get the 'standard' system keyboard shortcuts for this?
- How to detech ctrl/meta + key press? I never saw them detected though I'm testing with the android emulator which does
	werid things with external keybaords.
- keyboard input enabled by default? note: view needs to be focused to start receiving events
- configure zoom & pan steps?

Fixes saket#78
key = screen
)
}
val record = backstack.first()
Copy link
Author

@evant evant Mar 28, 2024

Choose a reason for hiding this comment

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

There's currently a bug where you can still keyboard focus the gallery screen when the media viewer is brought up, I'm not familiar enough with circuit to know what the right fix was so just changed it to render only the current page while testing. That does break the swipe away animation since it won't render the behind page until the animation completes

Copy link
Owner

Choose a reason for hiding this comment

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

There's currently a bug where you can still keyboard focus the gallery screen when the media viewer is brought up

hmmmmmm I wonder if I can fix this by preventing touch/focus events from leaking behind the active screen.

Copy link
Author

Choose a reason for hiding this comment

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

On my side project where I'm using a custom navigation solution I solved this by not rendering the behind page unless you were in the middle of the transition.

Copy link
Owner

@saket saket left a comment

Choose a reason for hiding this comment

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

This is fantastic, thank you!

Are you blocked on this by any chance? I'd like to complete #4 before I merge this, but I can try finding a stop-gap if you need this in your project.

key = screen
)
}
val record = backstack.first()
Copy link
Owner

Choose a reason for hiding this comment

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

There's currently a bug where you can still keyboard focus the gallery screen when the media viewer is brought up

hmmmmmm I wonder if I can fix this by preventing touch/focus events from leaking behind the active screen.

@evant
Copy link
Author

evant commented Apr 4, 2024

Are you blocked on this by any chance?

Nope! We ended up adding separate zoom/pan support on top for keyboard support for now. It does mean you could technically do both independently but it seems to work ok enough.

@saket saket changed the base branch from trunk to saket/may8/keyboard-support May 9, 2024 17:39
@saket saket marked this pull request as ready for review May 9, 2024 17:39
@saket
Copy link
Owner

saket commented May 9, 2024

Took me some time, but I'm finally working on this. I'm going ahead and merging this as-is because your code looks great. I'll make a few adjustments and then land the changes on trunk soon. Thanks Eva!

@saket saket merged commit a8c424b into saket:saket/may8/keyboard-support May 9, 2024
}
return true
}
event.key == Key.Zero -> {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm curious, what was the reason for supporting Key.Zero? I haven't been able to find any other app/OS that resets zoom when the zero key is pressed.

Copy link
Author

Choose a reason for hiding this comment

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

Because my browser does, didn't think too much about it and don't have strong opinions

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.

Keyboard support
2 participants