Adding hooks to add the keyboard on screen and be able to dismiss it.

Adding hooks to add the keyboard on screen and be able to dismiss it.

This CL does not add keyboard support yet. Just keyboard support in the UI.

Adding keyboard maping file and connected to host popup menu.

Review-Url: https://codereview.chromium.org/2869303003
Cr-Commit-Position: refs/heads/master@{#471509}
Committed: https://chromium.googlesource.com/chromium/src/+/bec93af7a19f8e40834ce58c953090a1ab951d3d

[Yuwei, 2017-05-11 20:53:02]

yuweih@chromium.org changed reviewers:
+ yuweih@chromium.org

[Yuwei, 2017-05-11 20:53:02]

Here are few things, I'm still thinking about solutions for some of them:

* keyboardWillShow/keyboardWillHide may be called multiple times before it's
actually shown/hidden. You may need to either get the size from [[UIScreen
mainScreen] bounds] or use a boolean to guard that.
* Clicking the FAB will hide the keyboard. You may want to keep the focus on the
keyboard when the FAB is clicked.
* Looks like viewDidLayoutSubviews will only be called once at the beginning of
the animation. This will stretch the content and shrink it down to the normal
size. Not sure how to fix this yet...
* It looks like I can still get an animation without the UIView animation..?
* DesktopViewport will reset the position after the view size has changed. You
might want to add a parameter or something to prevent the call to ResizeToFit
and call UpdateViewport instead.

[Yuwei, 2017-05-11 21:08:49]

On 2017/05/11 20:53:02, Yuwei wrote:
> Here are few things, I'm still thinking about solutions for some of them:
> 
> * keyboardWillShow/keyboardWillHide may be called multiple times before it's
> actually shown/hidden. You may need to either get the size from [[UIScreen
> mainScreen] bounds] or use a boolean to guard that.
> * Clicking the FAB will hide the keyboard. You may want to keep the focus on
the
> keyboard when the FAB is clicked.
> * Looks like viewDidLayoutSubviews will only be called once at the beginning
of
> the animation. This will stretch the content and shrink it down to the normal
> size. Not sure how to fix this yet...
> * It looks like I can still get an animation without the UIView animation..?
> * DesktopViewport will reset the position after the view size has changed. You
> might want to add a parameter or something to prevent the call to ResizeToFit
> and call UpdateViewport instead.

Another approach that might simplify things a bit is to not bother changing the
size of the view, simply change the surface size in gestureInterpreter instead
and
adjust the position of the FAB related to the visible area frame rather than to
the
frame of the GLKView. In that case we can easily animate the constrains of the
viewport.

This seems less cleaner though. Let me think...

[nicholss, 2017-05-12 15:00:04]

On 2017/05/11 20:53:02, Yuwei wrote:
> Here are few things, I'm still thinking about solutions for some of them:
> 
> * keyboardWillShow/keyboardWillHide may be called multiple times before it's
> actually shown/hidden. You may need to either get the size from [[UIScreen
> mainScreen] bounds] or use a boolean to guard that.

keyboard will show should only be called once. I am not sure where you get this
information.

> * Clicking the FAB will hide the keyboard. You may want to keep the focus on
the
> keyboard when the FAB is clicked.

The FAB will not dismiss the keyboard because it lost input focus, it hides it
now because I am using an alert as a menu which is not a long term solution.

> * Looks like viewDidLayoutSubviews will only be called once at the beginning
of
> the animation. This will stretch the content and shrink it down to the normal
> size. Not sure how to fix this yet...

> * It looks like I can still get an animation without the UIView animation..?

If you are adjusting things like layers then you get animations automatically.

> * DesktopViewport will reset the position after the view size has changed. You
> might want to add a parameter or something to prevent the call to ResizeToFit
> and call UpdateViewport instead.

I think we should hold off messing with this too much until we get into polish.
I would rather focus on the other features.

[nicholss, 2017-05-12 17:51:56]

On 2017/05/12 15:00:04, nicholss wrote:
> On 2017/05/11 20:53:02, Yuwei wrote:
> > Here are few things, I'm still thinking about solutions for some of them:
> > 
> > * keyboardWillShow/keyboardWillHide may be called multiple times before it's
> > actually shown/hidden. You may need to either get the size from [[UIScreen
> > mainScreen] bounds] or use a boolean to guard that.
> 
> keyboard will show should only be called once. I am not sure where you get
this
> information.

OH! I found the situation where the keyboard will show many times, on switching
the keyboard... ok will fix.

[chromium-reviews, 2017-05-12 18:02:50]

Or when it's third party keyboard, like GBoard.

<nicholss@chromium.org>于2017年5月12日 周五上午10:51写道：

> On 2017/05/12 15:00:04, nicholss wrote:
> > On 2017/05/11 20:53:02, Yuwei wrote:
> > > Here are few things, I'm still thinking about solutions for some of
> them:
> > >
> > > * keyboardWillShow/keyboardWillHide may be called multiple times
> before it's
> > > actually shown/hidden. You may need to either get the size from
> [[UIScreen
> > > mainScreen] bounds] or use a boolean to guard that.
> >
> > keyboard will show should only be called once. I am not sure where you
> get
> this
> > information.
>
> OH! I found the situation where the keyboard will show many times, on
> switching
> the keyboard... ok will fix.
>
> https://codereview.chromium.org/2869303003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nicholss, 2017-05-12 18:37:54]

On 2017/05/12 18:02:50, chromium-reviews wrote:
> Or when it's third party keyboard, like GBoard.

GBoard is means switching keyboards.

Fixed.

> 
> <mailto:nicholss@chromium.xn--org>2017512-d09qs76nd1vw2d 周五上午10:51写道：
> 
> > On 2017/05/12 15:00:04, nicholss wrote:
> > > On 2017/05/11 20:53:02, Yuwei wrote:
> > > > Here are few things, I'm still thinking about solutions for some of
> > them:
> > > >
> > > > * keyboardWillShow/keyboardWillHide may be called multiple times
> > before it's
> > > > actually shown/hidden. You may need to either get the size from
> > [[UIScreen
> > > > mainScreen] bounds] or use a boolean to guard that.
> > >
> > > keyboard will show should only be called once. I am not sure where you
> > get
> > this
> > > information.
> >
> > OH! I found the situation where the keyboard will show many times, on
> > switching
> > the keyboard... ok will fix.
> >
> > https://codereview.chromium.org/2869303003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Yuwei, 2017-05-12 19:36:36]

LGTM with nits

https://codereview.chromium.org/2869303003/diff/70001/remoting/ios/app/host_v...
File remoting/ios/app/host_view_controller.mm (right):

https://codereview.chromium.org/2869303003/diff/70001/remoting/ios/app/host_v...
remoting/ios/app/host_view_controller.mm:154: [UIView animateWithDuration:0.3
Define the magic number as a constant?

https://codereview.chromium.org/2869303003/diff/70001/remoting/ios/app/host_v...
remoting/ios/app/host_view_controller.mm:171: _keyboardSize = CGSizeZero;
Could you just store the height instead of the size?

[nicholss, 2017-05-12 21:46:43]

Thanks!

https://codereview.chromium.org/2869303003/diff/70001/remoting/ios/app/host_v...
File remoting/ios/app/host_view_controller.mm (right):

https://codereview.chromium.org/2869303003/diff/70001/remoting/ios/app/host_v...
remoting/ios/app/host_view_controller.mm:154: [UIView animateWithDuration:0.3
On 2017/05/12 19:36:36, Yuwei wrote:
> Define the magic number as a constant?

Done.

https://codereview.chromium.org/2869303003/diff/70001/remoting/ios/app/host_v...
remoting/ios/app/host_view_controller.mm:171: _keyboardSize = CGSizeZero;
On 2017/05/12 19:36:36, Yuwei wrote:
> Could you just store the height instead of the size?

I thought of doing that but I think we might need the width for iPad, it does a
funny split screen thing. So I wanted the full size of the keyboard for future
support.

[nicholss, 2017-05-12 21:46:54]

The CQ bit was checked by nicholss@chromium.org

[nicholss, 2017-05-12 21:46:54]

The patchset sent to the CQ was uploaded after l-g-t-m from yuweih@chromium.org
Link to the patchset: https://codereview.chromium.org/2869303003/#ps90001
(title: "Adding kKeyboardAnimationTime.")

[commit-bot: I haz the power, 2017-05-12 21:47:54]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 22:57:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 22:57:53]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nicholss, 2017-05-12 23:02:52]

The CQ bit was checked by nicholss@chromium.org

[commit-bot: I haz the power, 2017-05-12 23:04:32]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-13 00:36:57]

CQ is committing da patch.

Bot data: {"patchset_id": 90001, "attempt_start_ts": 1494630172764810,
"parent_rev": "46e542d6522f4508b6139269c40fbca018a6efdb", "commit_rev":
"bec93af7a19f8e40834ce58c953090a1ab951d3d"}

[commit-bot: I haz the power, 2017-05-13 00:37:17]

Description was changed from

==========
Adding hooks to add the keyboard on screen and be able to dismiss it.

This CL does not add keyboard support yet. Just keyboard support in the UI.

Adding keyboard maping file and connected to host popup menu.
==========

to

==========
Adding hooks to add the keyboard on screen and be able to dismiss it.

This CL does not add keyboard support yet. Just keyboard support in the UI.

Adding keyboard maping file and connected to host popup menu.

Review-Url: https://codereview.chromium.org/2869303003
Cr-Commit-Position: refs/heads/master@{#471509}
Committed:
https://chromium.googlesource.com/chromium/src/+/bec93af7a19f8e40834ce58c9530...
==========

[commit-bot: I haz the power, 2017-05-13 00:37:18]

Committed patchset #6 (id:90001) as
https://chromium.googlesource.com/chromium/src/+/bec93af7a19f8e40834ce58c9530...

[Yuwei, 2017-05-13 01:58:57]

Spotted bugs after you checked it in :P

Should be trivial to fix though

https://codereview.chromium.org/2869303003/diff/90001/remoting/ios/app/host_v...
File remoting/ios/app/host_view_controller.mm (right):

https://codereview.chromium.org/2869303003/diff/90001/remoting/ios/app/host_v...
remoting/ios/app/host_view_controller.mm:150: [[[notification userInfo]
objectForKey:UIKeyboardFrameBeginUserInfoKey]
You should use UIKeyboardFrameEndUserInfoKey instead. The Begin one is the
previous keyboard frame.

Maybe you can just use endFrame - beginFrame to calculate the delta rather than
storing the previous one?

https://codereview.chromium.org/2869303003/diff/90001/remoting/ios/app/host_v...
remoting/ios/app/host_view_controller.mm:155: [UIView animateWithDuration:0.3
This should use kKeyboardAnimationTime

https://codereview.chromium.org/2869303003/diff/90001/remoting/ios/app/host_v...
remoting/ios/app/host_view_controller.mm:158: f.size.height += deltaHeight;
Should be -=

[nicholss, 2017-05-15 15:34:14]

will update the next cl with these changes.

https://codereview.chromium.org/2869303003/diff/90001/remoting/ios/app/host_v...
File remoting/ios/app/host_view_controller.mm (right):

https://codereview.chromium.org/2869303003/diff/90001/remoting/ios/app/host_v...
remoting/ios/app/host_view_controller.mm:155: [UIView animateWithDuration:0.3
On 2017/05/13 01:58:57, Yuwei wrote:
> This should use kKeyboardAnimationTime

Done.

https://codereview.chromium.org/2869303003/diff/90001/remoting/ios/app/host_v...
remoting/ios/app/host_view_controller.mm:158: f.size.height += deltaHeight;
On 2017/05/13 01:58:57, Yuwei wrote:
> Should be -=

Delta can be negative. So I think the math works out.

[Yuwei, 2017-05-15 18:20:53]

https://codereview.chromium.org/2869303003/diff/90001/remoting/ios/app/host_v...
File remoting/ios/app/host_view_controller.mm (right):

https://codereview.chromium.org/2869303003/diff/90001/remoting/ios/app/host_v...
remoting/ios/app/host_view_controller.mm:158: f.size.height += deltaHeight;
On 2017/05/15 15:34:13, nicholss wrote:
> On 2017/05/13 01:58:57, Yuwei wrote:
> > Should be -=
> 
> Delta can be negative. So I think the math works out.

I think the problem here is this is the delta of the keyboard size rather than
the delta of the view size.