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

Return key on hardware keyboards now sends messages #1513

Merged
merged 3 commits into from
Sep 25, 2017

Conversation

vivlim
Copy link

@vivlim vivlim commented Sep 18, 2017

Signed-off-by: Vivian Lim vivvnlim@gmail.com

Create a class KeyboardGrowingTextView which inherits from HPGrowingTextView, but defines keyCommands. keyCommands defined at any other view or controller layer weren’t being picked up by the OS, but they did work when added to HPGrowingTextView.

I was able to validate this on my machine based on release-v0.5.1 but I can't figure out how to build the project after the App Extension was added. If you have any tips please let me know.

Also let me know if you have any ideas on better ways to call from KeyboardGrowingTextView to RoomInputToolbarView, or why defining keyCommands at any other layer didn't work (I had trouble pinning down exactly which class is the First Responder, and where the OS was looking for keyCommands)

Create a class KeyboardGrowingTextView which inherits from HPGrowingTextView, but defines keyCommands. keyCommands defined at any other view or controller layer weren’t being picked up by the OS, but they did work when added to HPGrowingTextView.

Signed-off-by: Vivian Lim <vivvnlim@gmail.com>
@@ -1,5 +1,5 @@
<?xml version="1.0" encoding="UTF-8"?>
<document type="com.apple.InterfaceBuilder3.CocoaTouch.XIB" version="3.0" toolsVersion="12121" systemVersion="16F73" targetRuntime="iOS.CocoaTouch" propertyAccessControl="none" useAutolayout="YES" useTraitCollections="YES" colorMatched="YES">
<document type="com.apple.InterfaceBuilder3.CocoaTouch.XIB" version="3.0" toolsVersion="12121" systemVersion="16G29" targetRuntime="iOS.CocoaTouch" propertyAccessControl="none" useAutolayout="YES" useTraitCollections="YES" colorMatched="YES">
Copy link
Author

Choose a reason for hiding this comment

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

this looks like something that shouldn't have made it in, will revert.

Copy link
Author

Choose a reason for hiding this comment

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

done

Didn’t mean to change the systemVersion property, and am reverting it. 

Signed-off-by: Vivian Lim <vivvnlim@gmail.com>
@aaronraimist
Copy link
Contributor

I'm not affiliated with Riot but I will try to take a look at the code for the return/send key tomorrow. I didn't get to build your patch yet but to be clear this is designed to only change the function on the return key on external keyboards?

I would be hesitant to change the function of the return key for only external keyboards. I think ideally there should be a user configurable setting (affecting both on screen and external keyboards) that changes the function of the return key to send.

Copy link
Member

@giomfo giomfo left a comment

Choose a reason for hiding this comment

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

@vivlim: nice job!
Test is ok. I just commented one point.


- (void)keyCommandSelector:(UIKeyCommand *)sender {
if ([sender.input isEqualToString:@"\r"]){
// traverse the view hierarchy to get the RoomInputToolbarView
Copy link
Member

Choose a reason for hiding this comment

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

@vivlim you don't have to traverse the view hierarchy to get the RoomInputToolbarView.

This object is the delegate of the KeyboardGrowingTextView instance.
You may just check whether [self.delegate isKindOfClass: RoomInputToolbarView.class]

@ara4n
Copy link
Member

ara4n commented Sep 21, 2017

Agreed with @aaronraimist that it'd be best to have a "send on enter" option in general rather than making it specific to external keyboards. It would also be really really nice to have shift-enter to insert linebreaks (as per riot-web).

@vivlim
Copy link
Author

vivlim commented Sep 22, 2017

@ara4n Shift-enter works fine on external keyboards, this code only handles unmodified enter keys. 👍🏼
@aaronraimist correct. The code for handling onscreen keyboard returns is pretty different I think, UIKeyCommands are exclusively for hardware keyboards. I did not investigate the onscreen keyboard since the current behavior there is reasonable (if you’re already typing on the touchscreen, the send button is pretty easy to reach, and you still need to be able to insert linebreaks)

I’d like to note that this change opens up the possibility of adding additional UIKeyCommands for more shortcuts later on (i.e. cmd+number to switch between chats), they just need to be registered at this level since this is the delegate the OS inspects the keyCommands of.

@giomfo Thanks, I’ll remove the traversal. That’s much better.

@giomfo
Copy link
Member

giomfo commented Sep 22, 2017

Let's fix here the return key handling for the external keyboard.
I filed another issue #1529 to handle the "send on enter" option in general.

Signed-off-by: Vivian Lim <vivvnlim@gmail.com>
@vivlim
Copy link
Author

vivlim commented Sep 23, 2017

I've been having trouble with getting the app to build since the app extension was added, and I don't have an Apple Developer Program account... could someone please try the latest commit?

@giomfo giomfo merged commit 39bcb17 into element-hq:develop Sep 25, 2017
@vivlim
Copy link
Author

vivlim commented Sep 26, 2017

thanks!

@vivlim vivlim deleted the hardware-keyboard-return branch September 26, 2017 03:29
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.

4 participants