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

[UX] VirtualKeyPopup: respond to hold & pan release #4887

Merged
merged 3 commits into from Apr 9, 2019

Conversation

Projects
None yet
2 participants
@Frenzie
Copy link
Member

commented Apr 9, 2019

As suggested by @poire-z in #4886 (comment)

This is actually much better than before.

It subtly changes the behavior of every key this way, but I don't think that's a bad thing.

[UX] VirtualKeyPopup: respond to hold & pan release
As suggested by @poire-z in #4886 (comment)

This is actually much better than before.

@Frenzie Frenzie added the UX label Apr 9, 2019

@Frenzie Frenzie added this to the 2019.05 milestone Apr 9, 2019

@Frenzie Frenzie requested a review from poire-z Apr 9, 2019

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

@poire-z Don't merge, the behavior isn't 100 % right yet. I didn't notice until on-device testing. ;-)

Frenzie added some commits Apr 9, 2019

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

I'll go ahead and merge this one 'cause it's such a definite improvement. ;-)

@Frenzie Frenzie merged commit 69e3830 into koreader:master Apr 9, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@Frenzie Frenzie deleted the Frenzie:hold-pan-stuff branch Apr 9, 2019

virtual_key._onHoldReleaseKey = virtual_key.onHoldReleaseKey
virtual_key.onHoldReleaseKey = virtual_key._keyOrigHoldPanHandler
virtual_key._onPanReleaseKey = virtual_key.onPanReleaseKey
virtual_key.onPanReleaseKey = virtual_key._keyOrigHoldPanHandler

This comment has been minimized.

Copy link
@poire-z

poire-z Apr 9, 2019

Contributor

What does all that allow?
Not really understanding - looks to me you hold on the original keyboard key - and this virtual key (even with the same letter) is another VirtualKey object, so when you release that second one, the first one should still work. (?)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 9, 2019

Author Member

This doesn't allow anything. ;-) It restricts the central key in the popup from responding as if there were an intentional hold release after opening. But after opening, it felt odd when it didn't respond like any other key.

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

Some small remarks:

  • On my Kobo, the popup keys are not vertically aligned with the main keyboard keys, while they are on the emulator. So, may be a missing scaleBySize() somewhere (or just me having missed updating my small private hacks in that file for this change, so please check on your Kobo :)
  • With flash keyboard enabled, when taping another key in the popup, the taped key is border-flashed. But when panning and releasing on another key, it's the middle key that is border-flashed, and not the selected other key.
@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

If by not vertically aligned you mean off by a pixel or two then it's probably just something like a wrong number of subtracted borders or something. If you truly mean not vertically aligned then I'm fairly sure it's just you. ;-)

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

Yeah, I mean off by two pixels or three :)

Other remarks:

  • Dunno if that should happen on popup key tap, but may be on popup key panrelease, the popup could close, as it happens on some(all?) android virtual keyboards - to avoid that additional tap to close it
  • Possibly, for the extra keys definitions (local _e_ =), it would be nice if they were put into an independant file, so we can import them easily into the other layouts (fr_keyboard.lua), without having to redefine them all (even if of fr, we may tweak a few of them to get ê at north). Not sure if there's a way in Lua like in python: from extra_keys import *.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.