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] Keyboard character selection popup #4886

Merged
merged 13 commits into from Apr 9, 2019

Conversation

Projects
None yet
4 participants
@Frenzie
Copy link
Member

commented Apr 9, 2019

Other than positioning, this is a feature-complete proof of concept. For the moment this PR is mainly to gather potential feedback before/while tidying it all up.

Screenshot_2019-04-09_16-29-30

@Frenzie Frenzie added the UX label Apr 9, 2019

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

self.hold_callback = function()
if not self.key_chars then return end

local popup_focus_manager = FocusManager:new{

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 9, 2019

Author Member

@poire-z I started out writing this as a small(ish) inline function, but I figure I should separate it out into a VirtualKeyPopup (or some such).

This comment has been minimized.

Copy link
@poire-z

poire-z Apr 9, 2019

Contributor

Yes, just to avoid creating and garbage collecting these few methods functions (even if it's little compared to the other stuff created in there :)

local key_chars = self.key_chars
--logger.dbg(key_chars)
--error()
local extra_key_chars = {}

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 9, 2019

Author Member

This here is the basic principle on which it's based: a 12-key grid of 4*3, manually assigned. 9 of those are directly accessible through a tap and 8 swipe directions.

It could be made to expand further on top, but that doesn't seem like the best idea as far as ease of use goes.

This comment has been minimized.

Copy link
@poire-z

poire-z Apr 9, 2019

Contributor

Not sure while just reading the code: do all these keys are also tap'able (so, the whole thing works without swipe)? Or it's just showing what's available?
I guess so, if there are these extra keys.
Do the extra_key row shows when there is none defined for that key?

Also, if this is shown on Hold, you might want to handle HoldPan/HoldPanRelase to support a swipe started from a Hold :)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 9, 2019

Author Member

These are only tappable. I suppose making swiping possible could be an idea.

table.insert(group, blank)
end
end
table.insert(popup_focus_manager.layout, layout_horizontal)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 9, 2019

Author Member

It works well with the keyboard, go me! ;-) (Okay, just for thinking of it.)

UIManager:show(popup_focus_manager)
--error()

UIManager:setDirty(self, function()

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 9, 2019

Author Member

This stuff down here is super messy obviously, just for quick testing.

background = Blitbuffer.COLOR_WHITE,
radius = 0,
padding = self.keyboard.padding,
CenterContainer:new{

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 9, 2019

Author Member

I haven't really thought about positioning yet. It needs to go on top of the key, but it shouldn't go outside the screen.

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Apr 9, 2019

Member

Yeah, this is going to be tricky with 'A', for instance, whether it's a qwerty or azerty layout ;).

I haven't checked what kind of info we have, but the simplest solution I can think of is to shift the whole popup to the opposite direction by the overflow (i.e., off-screen) amount, so that it's at the edge of the screen, while staying as close as possible around the original key, in these cases.

Sidebar: I'd make the border around the original key bolder/a different color to make it more obvious, which'd be neat in general, but pretty much necessary when it gets shifted like that ;).

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 9, 2019

Author Member

However, it's only tricky with characters like the A if we want to use the full 12 of them. It's really easy to do it in the definition instead.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 9, 2019

Author Member

I mean like this:

Screenshot_2019-04-09_17-20-26

local _a_ = {
    [1] = "a",
    north = "ë",
    northeast = "é",
    --northwest = "è",
    east = "ê",
    --west = "ẽ",
    south = "ę",
    southeast = "",
    --southwest = "ė",
    [3] = "ē",
}

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Apr 9, 2019

Member

True, but it breaks the swipe paradigm a tiny bit (and, for instance, we do kind of like our à's in french ;)), and has to be layout-specific.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 9, 2019

Author Member

I think those swipes are already broken by their location regardless. But, um, everything's layout-specific, so I'm not quite sure what you mean there. ;-)

Light-gray highlight:

Screenshot_2019-04-09_17-50-47

This comment has been minimized.

Copy link
@poire-z

poire-z Apr 9, 2019

Contributor

And ê should probably be associated to swipe north, as the ^ points up.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 9, 2019

Author Member

The acute & grave accents happen to correspond to mnemonic gestures in this layout, but in the English/international keyboard I think the diaeresis makes more sense in the prime location than the more or less French-only circumflex. Besides, most diacritics are on top. ;-)

That being said, I haven't done an actual usage frequency analysis.

This comment has been minimized.

Copy link
@poire-z

poire-z Apr 9, 2019

Contributor

OK, so that will be for the french layout only then.
(It's going to be a bit of a mess to keep the layouts sync'ed...)

height = self.height,
}
table.insert(group, virtual_key)
table.insert(layout_horizontal, virtual_key)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 9, 2019

Author Member

Plus the padding.

Frenzie added some commits Apr 9, 2019

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

Basic positioning. I'll force it in the frame later tonight or tomorrow. In any case it seems to be about ready for some minor refactoring and actual review.

Screenshot_2019-04-09_19-02-12
Screenshot_2019-04-09_19-02-22

Frenzie added some commits Apr 9, 2019

@Frenzie Frenzie changed the title WIP: [UX] Keyboard character selection popup [UX] Keyboard character selection popup Apr 9, 2019

Frenzie added some commits Apr 9, 2019

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

@poire-z @NiLuJe Alright, ready for actual review.

@poire-z
Copy link
Contributor

left a comment

Looks good.
No std.lua layout with some actual extended keys in that PR?

inputbox = nil,
layout = {},
}

This comment has been minimized.

Copy link
@poire-z

poire-z Apr 9, 2019

Contributor

Why not move all the VirtualKeyPopup: methods here ?
I see there's in one of them a reference to VirtualKey, so there may be a circular reference that leads to how you laid them. But it looks like it's defined in a function that is created at runtime, so may be it can be looked up fine if you put them here?

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 9, 2019

Author Member

I could feed the VirtualKeyPopup down in VirtualKeyboard so they're both defined properly.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 9, 2019

Author Member

The advantage of VirtualKey:init() is that it has all the modifier etc. logic, so that'd result in some wasted memory on all keys though.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 9, 2019

Author Member

Also there's the backspace thing.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 9, 2019

Author Member

Maybe as an extra property hold_callback_default?

This comment has been minimized.

Copy link
@poire-z

poire-z Apr 9, 2019

Contributor

Not really following you :)
I just meant: move the code (just the lines, no indentation shifts needed) AFTER the local VirtualKeyPopup = FocusManager:new{}, not INSIDE :)
Just to group object and method definition in the file.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 9, 2019

Author Member

That can't work because VirtualKey isn't defined yet.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 9, 2019

Author Member

I have no idea what you even mean by inside. Where they were before? :-P

This comment has been minimized.

Copy link
@poire-z

poire-z Apr 9, 2019

Contributor

Fine with me with latest commit :)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Apr 9, 2019

Author Member

Alright, merging then so nightly users can test.

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

I'd prefer to keep keep actual key layouts to separate PRs. :-)

@Frenzie Frenzie merged commit 23f9032 into koreader:master Apr 9, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@Frenzie Frenzie deleted the Frenzie:key-popup branch Apr 9, 2019

Frenzie added a commit to Frenzie/koreader that referenced this pull request Apr 9, 2019

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

This is actually much better than before.

Frenzie added a commit that referenced this pull request Apr 9, 2019

[UX] VirtualKeyPopup: respond to hold & pan release (#4887)
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.
@Biep

This comment has been minimized.

Copy link

commented Apr 11, 2019

Would inverting (as in night mode) the pop-up keyboard be a good idea? It would make it stand out more from the permanent keyboard.
Or would having a black-on-white popup at night be too disrupting?

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

@Biep

This comment has been minimized.

Copy link

commented Apr 11, 2019

Ah - hardware limitation. Waiting for the next generation of e-ink: whiter, faster, and non-ghosting.

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.