GDevelopApp / GDevelop-extensions

Repository of behaviors, actions, conditions and expressions to be used in GDevelop for creating games
https://gdevelop.io
MIT License
131 stars 52 forks source link

Mobile keyboard #11

Closed Bouh closed 5 years ago

Bouh commented 5 years ago

Describe the extension

This extension add a behavior for allow to use keyboard mobile in your games ! I've created a example, you can play it on itch.io here. All is downloadable bellow 👇

Event added :

Checklist

Extension file

Extension_keyboard_mobile_JSON.zip

Example_Keyboard_mobile.zip

4ian commented 5 years ago

Nice! I've just quickly checked, this works well on my phone. Will be super useful!! I've not checked the JS entirely but:

Bouh commented 5 years ago

I've two other issues :

"Always" is not necessary in the conditions.

What is the condition for?

I've maybe found fix about infinite opening and closing. Maybe this fix will fix backwards. The things are strange, when i hide HTML input with position: relative;height: 0;width: 0;left: -10px; We have issues, but not with : position: fixed;z-index: -1;top: -30px;

4ian commented 5 years ago

Opera : keyboard hover game, and game don't fit screen

Not sure if it's worth spending time fixing things in Opera Mobile - its usage is really low, not sure how good it is in standards implementation and we should more generally aim for Chrome on Android and Safari on iOS.

position: fixed;z-index: -1;top: -30px;

fixed sounds like a proper positioning. Not sure about using z-index, but you should use visibility: hidden or display: none (try display: none but I'm not sure if you can then trigger the focus on the element).

4ian commented 5 years ago

The keyboard in an infinite state of opening and closing very rapidly. Even after closing chrome. Device required restart.

For this one sounds really strange, not sure what can be happening.. is this the latest version of Chrome?

It writes backwards everything

Could this be related to the fact that the selection range is not being set at the end?

4ian commented 5 years ago

(Sorry for the spam, discovering things progressively).

I think you might be overcomplicating things, which could lead to the strange behaviors. Instead, this should work:

gdjs._extensionMobileKeyboard.openKeyboard = function () {
    var selectInput = document.getElementsByTagName("input")[0];
    if (selectInput) selectInput.focus();
}

Note that as mentioned before you should be passing the id and use getElementById instead of using getElementsByTagName which won't work with more than one input (and will break if you have other inputs on the webpage).

gdjs._extensionMobileKeyboard.closeKeyboard = function () {
    var selectInput = document.getElementsByTagName("input")[0];
    if (selectInput) selectInput.blur();
}

Finally, try to read more about event handling in JavaScript, your usage of events listeners is not correct. I'm pretty sure this leads to the infinite loop you had:

Meaning that you add a new event listener 60 frames per second. This means that after 10 seconds, if I press any key, this function will be called 600 times! No wonder browser is crashing ;)

Instead:

This code says "WHEN there is a keyup event happening on selectInput, THEN launch this function". It's some kind of asynchronous, event driven way of doing things is quite usual in JS. It takes a bit of time to get use to this, but it's important to understand that JS will take care of calling the events once they are registered. You must never register them twice, and you must unregister them.

  var element = document.getElementById(theIdOfTheInput);
  element.parentNode.removeChild(element);

How to find theIdOfTheInput => well, that's why you have to store it on the behavior or on the object (preferably on the behavior) ;) i.e: in "onCreate":

const input = document.createElement("input");//create input
input.type = "text";
var uniqueId = "GDevelop_Mobile_Keyboard_Input" + Date.now() +'-'+Math.floor(Math.random()*100000); // Create an identifier that is unique
input.id = uniqueId;

Finally, store the uniqueId on the behavior:

var behaviorName = eventsFunctionContext.getBehaviorName("Behavior");
eventsFunctionContext.getObjects("Object").getBehavior(behaviorName)._uniqueId = uniqueId; // (not tested)
Bouh commented 5 years ago

I've found a mobile with this issues, i will try to debug it in next days, it's not my phone ^^ Due to the many problems, it possible this extension may not work on many devices. And i'am a little busy this weekend (24h Le Mans), i will take a look next week.

4ian commented 5 years ago

I've been busy too, I can surely help this week to simplify the code (read my previous comments). Hopefully that should solve the issues.

On Fri, Jun 14, 2019 at 9:04 PM Bouh notifications@github.com wrote:

I've found a mobile with this issues, i will try to debug it in next days, it's not my phone ^^ Due to the many problems, it possible this extension may not work on many devices. And i'am a little busy this weekend (24h Le Mans), i will take a look next week.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/4ian/GDevelop-extensions/issues/11?email_source=notifications&email_token=AAJYRAXSKDT6L2OHGOXGCVTP2P2THA5CNFSM4HXVKXTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXX2PEI#issuecomment-502245265, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJYRAVHWCTF55OKV73YGF3P2P2THANCNFSM4HXVKXTA .

Bouh commented 5 years ago

It doesn't seem possible to have the behavior of an object because it's empty.

eventsFunctionContext.getObjects("Object").getBehavior(behaviorName) Is undefined

eventsFunctionContext.getObjects("Object") returns a array and so it would be necessary to put eventsFunctionContext.getObjects("Object")[0] But even with this or without it, line doesn't work

debug screen image

Edit : seem empty only in onCreated

4ian commented 5 years ago

Ah right, onCreated is kind of broken right now :/ :/

Let's use for now an hardcoded id and I can think about a fix for next version.

Bouh commented 5 years ago

I will save the id in Object for moment. If you fix it soon i can compile branch myself and continue with behavior idea. Nevermind i will use Object for now

4ian commented 5 years ago

Let's go with the object for now, the behavior should not be used twice at all so it's perfectly fine :) Don't forget to set a name with a prefix like _mobileKeyboardExtensionInput.

Bouh commented 5 years ago

Fixed : infinite state openning, writes backwards, del key delete everything (samsung)

-I've hardcoded the object but maybe i should use loop ? eventsFunctionContext.getObjects("Object")[0]._mobileKeyboardExtensionInput = { "_uniqueId": arg };

-I've almost hide the input, it's complicated because when we hide it focus can't be effective on it and keyboard don't will be open.

-Blue lines are present sometime depending of words, seem to be spell checker for mobile. I don't know how to get rid of it. I searched in CSS but I didn't see much, and in HTML autocomplete="off" doesn't do anything image

-I think of countries that write from right to left if I keep my function to delete letters and the function to have the cursor at the end, for this countries the keyboard will not work at all I think. I should test this.

New version can be tested here

Source Keyboard_mobile.zip

I've asked feedbacks on Discord 🤞

(PS : We can open the functions from the eventsheet, but the "Behavior functions" panel does not open, I think you forgot to load it)

4ian commented 5 years ago

I've hardcoded the object but maybe i should use loop ?

No that seems fine, the object is guaranteed to be here :)

You're still doing:

selectInput.addEventListener("keyup", function (event) {

in the doStepPreEvents. Do you think you can move it to onCreated? I don't see any reason why it would not work when using it in onCreated.

Remember, events must be registered only once, otherwise you expose yourself to some very nasty bugs, memory issues/crashes.

I think of countries that write from right to left if I keep my function to delete letters and the function to have the cursor at the end, for this countries the keyboard will not work at all I think. I should test this.

Depends, maybe the cursor position will actually be properly handled by the browser. By there is no support for rtl (right to left) for text objects I think :/ Should be tested though, might be handled by Pixi.js

Bouh commented 5 years ago

We have another thing to refactorize ? Because before publishing I prefer to ask people one last time to make a test with the extension. And i need clean all comments ;)

Keyboard_mobile.zip

4ian commented 5 years ago

Let me give another look soon. Ideally I would fix the onCreated of behaviors so that this can work with multiple objects.

4ian commented 5 years ago

Sorry did not had the time to do this this week. Still hoping to do this this week-end!

4ian commented 5 years ago

Just took a quick look: the calls to gdjs._extensionMobileKeyboard.openKeyboard/gdjs._extensionMobileKeyboard.closeKeyboard won't work with multiple objects. Precisely, they will always act on the same object. Why?

Because each time the behavior is created, you're creating a new function "gdjs._extensionMobileKeyboard.openKeyboard" (and some for close). In this function, you're calling getUniqueIdInObject, and then inside this function you're getting the id from eventsFunctionContext (by getting the behavior and the object).

The problem is that eventsFunctionContext will always refer to the same thing in memory. You will always reuse the "eventsFunctionContext" from the onCreate, not the eventsFunctionContext of the object on which the action to open/close the keyboard is called.

The solution? Pass eventsFunctionContext as argument to these functions. These functions must not refer to anything outside of their scope. Read more about JavaScript closures to see the idea: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Closures

Bouh commented 5 years ago

On line 61, in keyup event i close keyboard like this, but i can't spend eventsFunctionContext in closeKeyboard, because we are in onCreated function.

if (event.keyCode === 13) {//Enter key
        gdjs._extensionMobileKeyboard.closeKeyboard != undefined ? gdjs._extensionMobileKeyboard.closeKeyboard() : null;
}

I've found a solution for that, here the project with extension Keyboard_mobile.zip

4ian commented 5 years ago

Almost there. The way you're calling closeKeyboard, without arguments, mean that it will only close the keyboard for the latest eventsFunctionContext, i.e: only for the latest behavior that was created.

You should consider all gdjs._extensionMobileKeyboard.XXX functions as "static": they cannot rely on things outside of them. Otherwise, this means that you're making them only work for the latest behavior that was created - resulting in super strange results.

How to do then? In the "keyup" event, you can still use uniqueId. This means that uniqueId will be remembered by the closure (i.e: the function) that is "keyup". You can then call a function gdjs._extensionMobileKeyboard.closeKeyboardById:

gdjs._extensionMobileKeyboard.closeKeyboardById = function (uniqueID) {
    var selectInput = document.getElementById(uniqueID);
    if (selectInput) selectInput.blur();
    selectInput.style.setProperty("visibility", "hidden");
}

and refactor the other function as:

gdjs._extensionMobileKeyboard.closeKeyboard = function (eventsFunctionContext) {
    var uniqueID = gdjs._extensionMobileKeyboard.getUniqueIdInObject(eventsFunctionContext);
    gdjs._extensionMobileKeyboard.closeKeyboardById(uniqueID);
}

With this you have:

Two other things:

    selectInput.style.removeProperty("visibility");
    if (selectInput) selectInput.focus();

should be:


    if (selectInput) {
      selectInput.focus();
      selectInput.style.removeProperty("visibility");
    }

Otherwise, a null/undefined selectInput will crash the game because you're trying to access to its style.

var a = 3 
var test = function () {
var a = 5; // "a" here is scoped in the function "scope". It won't erase the "a" that is outside.
return a;
}

console.log(a); // 3
console.log(test()); // 5
console.log(a); // 3
Bouh commented 5 years ago
gdjs._extensionMobileKeyboard.closeKeyboard = function (eventsFunctionContext) {
    var uniqueID = gdjs._extensionMobileKeyboard.getUniqueIdInObject(eventsFunctionContext);
    gdjs._extensionMobileKeyboard.closeKeyboardById(uniqueID);
}

eventsFunctionContext exist already in parent scope, if eventsFunctionContext is as argument in fonction and available in JSevent by default, how is defined which of both will be used in function, argument overwrite and if argument eventsFunctionContext is empty, eventsFunctionContext is undefined ? Strange to give a name already exist in parent scope.

New file : Keyboard_mobile.zip

4ian commented 5 years ago

eventsFunctionContext exist already in parent scope, if eventsFunctionContext is as argument in fonction and available in JSevent by default, how is defined which of both will be used in function

It's always the variable declared in the "inner scope" that will be used. It's the variable that is the "closest" to your code in a way :)

var a = 3
var test = function (a) {
a += 2;
return a;
}

console.log(a); // 3
console.log(test(4)); // 6
console.log(a); // 3

Thanks I'll check the changes asap :)

4ian commented 5 years ago

This looks mostly correct :) I think we can still streamline the code and make it more robust against potential issues (like check if input is not null/undefined before destroying it - yeah you never now, someone could go and destroy it for some reason, murphy's law!). I'll clean that and try to publish this soon.

in the meantime, if you can proof test it with a few devices would be great :) Ideally we should see if two inputs at the same time are working.

Bouh commented 5 years ago

I will ask on discord for testing. Wait before publish, i try to make a example with multiple input. Something like this but it's bugged, i need to be sure if the bug come from extension or my example.

Bouh commented 5 years ago

I have made changes, I don't need doStepPreEvents and doStepPostEvents, instead the text is refreshed only when a key is pressed. 👌 This is a good optimization, in addition to which it takes away the fact that when we act on object entry_text with doStepPreEvents and doStepPostEvents, it acts strangely because the code is executed before or after our events.

I made an example with two text fields, I let you see how it works, it uses only one entry_text ! And it is scalable in number of text fields.

I am also thinking of adding the same actions/conditions as for the pc keyboard.

Or we can find a way from the extension to send to GDJS the mobile inputs over the PC one if the keyboard is open. This way we can use the events of the keyboard already present and it supports the pc keyboard, and mobile keyboard.

I've updated my website you can try my example online, and of course project sources files with the extension updated too.

Keyboard_mobile.zip

4ian commented 5 years ago

I have made changes, I don't need doStepPreEvents and doStepPostEvents, instead the text is refreshed only when a key is pressed. 👌

This is a great improvement! Really appreciate these simplifications! You can go ahead and delete the useless functions, we now have to clean up all of this :)

I made an example with two text fields, I let you see how it works, it uses only one entry_text ! And it is scalable in number of text fields.

Nice, would it work too with multiple text entry objects? I think people will use one per object that have to capture text. Basically, as a user, I add two texts, then for each one two text entry objects, then when I detect a click on a text, I open the keyboard for the associated text entry. Would that work?

(I know this is less scalable, but people will do this and it's I think more intuitive for new users)

This is getting good :) The example is working fine on my (Android) phone.

Bouh commented 5 years ago

And voilà, the extension is clean now, and working in a new example more simple.

Keyboard_mobile.zip

4ian commented 5 years ago

Awesome, I'll take a look later today and will merge this if it looks ready :)

4ian commented 5 years ago

(still on my to-do list sorry for the delay!)

Bouh commented 5 years ago

It's not urgent, put it at the end of the list;)

4ian commented 5 years ago

Let's continue the discussion here, I've created a PR :) https://github.com/4ian/GDevelop-extensions/pull/15