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.

Sign up for GitHub

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

Jump to bottom

[3.x] Add support for multiple virtual keyboard types #58537

Merged
merged 1 commit into from
Aug 5, 2022
Merged

[3.x] Add support for multiple virtual keyboard types #58537

merged 1 commit into from
Aug 5, 2022

Conversation

brianwinterpixel
Copy link
Contributor

@brianwinterpixel brianwinterpixel commented Feb 25, 2022 •
edited
Loading

3.x version of #58536

Compatibility is maintained by retaining the old OS::show_virtual_keyboard function binding and adding a new binding for OS::show_virtual_keyboard_type that allows specifying the virtual keyboard type.

Differences between this PR and the linked 4.0 PR:

  • iOS does not support the password keyboard type before iOS version 11.0, so it might not work depending on which iOS version you export to.

@brianwinterpixel brianwinterpixel force-pushed the feature/virtual-keyboard-types-3.x branch from 082fb88 to 7dffa7f Compare February 25, 2022 19:05
@brianwinterpixel brianwinterpixel marked this pull request as ready for review February 25, 2022 19:05
@brianwinterpixel brianwinterpixel requested review from a team as code owners February 25, 2022 19:05
@brianwinterpixel brianwinterpixel changed the title Add support for multiple virtual keyboard types [3.x] Add support for multiple virtual keyboard types Feb 25, 2022
@Calinou Calinou added this to the 3.5 milestone Feb 25, 2022
@m4gr3d
Copy link
Contributor

m4gr3d commented Feb 26, 2022 •
edited
Loading

@brianwinterpixel To avoid breaking compatibility, you can leave the original show_virtual_keyboard method and make it a delegate to the newly added method.
e.g:

void show_virtual_keyboard(const String &p_existing_text, bool p_multiline) {
    show_virtual_keyboard(p_existing_text, p_multiline ? KEYBOARD_TYPE_MULTILINE : KEYBOARD_TYPE_DEFAULT);
}

Of course, this is only necessary where the function is publicly exposed (e.g: to gdscript), and the internal versions can be removed in favor of the new one.

m4gr3d
m4gr3d requested changes Feb 26, 2022
View reviewed changes
platform/android/java/lib/src/org/godotengine/godot/GodotIO.java Outdated Show resolved Hide resolved
platform/android/java/lib/src/org/godotengine/godot/input/GodotEditText.java Outdated Show resolved Hide resolved
scene/gui/line_edit.h Outdated
Comment on lines 61 to 70
enum VirtualKeyboardInputType {
KEYBOARD_TYPE_DEFAULT,
KEYBOARD_TYPE_MULTILINE,
KEYBOARD_TYPE_NUMBER,
KEYBOARD_TYPE_NUMBER_DECIMAL,
KEYBOARD_TYPE_PHONE,
KEYBOARD_TYPE_EMAIL_ADDRESS,
KEYBOARD_TYPE_PASSWORD,
KEYBOARD_TYPE_URL
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to refer to the enum defined in os.h instead of redefining a new one here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've seen this pattern used elsewhere in the engine, such as Viewport::Usage <-> VisualServer::ViewportUsage.
There is a bit of an interdependency between OS, core_bind.h::_OS, and LineEdit. To keep bindings straightforward, I think this is fine.

@brianwinterpixel brianwinterpixel force-pushed the feature/virtual-keyboard-types-3.x branch 2 times, most recently from ded3316 to 3e599ac Compare February 27, 2022 20:24
@m4gr3d
Copy link
Contributor

m4gr3d commented Jun 26, 2022

@brianwinterpixel Were you able to address the review feedback?

@brianwinterpixel
Copy link
Contributor Author

@brianwinterpixel Were you able to address the review feedback?

I believe so. It's been a while since I've touched this but if I recall correctly it should be good to go.

akien-mga
akien-mga approved these changes Jul 2, 2022
View reviewed changes
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

This looks really well implemented, kudos!

I'm reviewing a bit late to include it in 3.5 but this should be fine for 3.6. Would be good to get reviews from @godotengine/ios and @godotengine/html5 too.

doc/classes/OS.xml Show resolved Hide resolved
doc/classes/OS.xml Outdated Show resolved Hide resolved
doc/classes/LineEdit.xml Outdated Show resolved Hide resolved
platform/android/java/lib/src/org/godotengine/godot/input/GodotEditText.java Show resolved Hide resolved
platform/android/os_android.cpp Outdated Show resolved Hide resolved
platform/iphone/os_iphone.mm
Comment on lines +463 to +465
case KEYBOARD_TYPE_MULTILINE: {
AppDelegate.viewController.keyboardView.keyboardType = UIKeyboardTypeDefault;
} break;
Copy link
Member

Choose a reason for hiding this comment

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

Is it expected that iOS doesn't have any multiline mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe there is any applicable multiline option for iOS. Someone else may know better.

See: https://developer.apple.com/documentation/uikit/uikeyboardtype?language=objc

scene/gui/line_edit.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga modified the milestones: 3.5, 3.x Jul 2, 2022
@akien-mga akien-mga requested a review from m4gr3d July 2, 2022 18:53
@brianwinterpixel brianwinterpixel force-pushed the feature/virtual-keyboard-types-3.x branch from 3e599ac to e5f3d95 Compare July 2, 2022 19:46
@brianwinterpixel brianwinterpixel force-pushed the feature/virtual-keyboard-types-3.x branch from e5f3d95 to c98cae3 Compare July 2, 2022 19:49
@Faless
Copy link
Collaborator

Faless commented Jul 2, 2022

Added my comment to the master branch version: #58536 (comment)

m4gr3d
m4gr3d approved these changes Jul 3, 2022
View reviewed changes
Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

Looks good for Android!

@brianwinterpixel brianwinterpixel force-pushed the feature/virtual-keyboard-types-3.x branch from c98cae3 to ce24b48 Compare July 7, 2022 18:22
@akien-mga akien-mga modified the milestones: 3.x, 3.5 Aug 5, 2022
@akien-mga akien-mga merged commit 26762a7 into godotengine:3.x Aug 5, 2022
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga modified the milestones: 3.5, 3.6 Aug 5, 2022
@brianwinterpixel brianwinterpixel deleted the feature/virtual-keyboard-types-3.x branch August 5, 2022 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akien-mga akien-mga akien-mga approved these changes

@m4gr3d m4gr3d m4gr3d approved these changes

Assignees
No one assigned
Labels
enhancement platform:android platform:ios platform:web topic:input topic:porting
Projects
None yet
Milestone
3.6
Development

Successfully merging this pull request may close these issues.
5 participants
@brianwinterpixel @m4gr3d @Faless @akien-mga @Calinou