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

Steal @Starmapo input text #3178

Closed
wants to merge 6 commits into from

Conversation

Geokureli
Copy link
Member

@Geokureli Geokureli commented Jun 12, 2024

Just learned this exists, @Starmapo if you tried to show me this, before, I missed it. I saw Funkin was using it

I have not yet begun to consider the ramifications of this, so let's start with seeing if it clears coverage CI and code climate, I guess. If you could list - in detail - the changes, improvements and features of this that would be a great!

@Geokureli Geokureli added this to the 5.9.0 milestone Jun 12, 2024
@Geokureli Geokureli changed the title add @Starmapo input text steal @Starmapo input text Jun 12, 2024
@Geokureli Geokureli changed the title steal @Starmapo input text Steal @Starmapo input text Jun 12, 2024
@Geokureli
Copy link
Member Author

Because it extends TextField methods that don't fit flixel's checkstyle settings. looking into a way to suppress these

@Starmapo
Copy link
Contributor

I'll list the improvements later once I can properly check the differences. I'd just like to warn you first that, at least in its current state, it does not compile for Flash, as the TextField extension sets the stage variable which is required for some of the input stuff to work, and this variable isn't able to be modified on Flash.

@Geokureli
Copy link
Member Author

Geokureli commented Jun 12, 2024

I'll list the improvements later once I can properly check the differences.

No rush!

I'd just like to warn you first that, at least in its current state, it does not compile for Flash, as the TextField extension sets the stage variable which is required for some of the input stuff to work, and this variable isn't able to be modified on Flash.

Aight, then we'll need a workaround

@Geokureli
Copy link
Member Author

Geokureli commented Jun 12, 2024

Also note: this is going to flixel, not flixel-ui. The plan for releasing this is:

  • add user flag FLX_NEW_INPUT_TEXT and counter-flag FLX_LEGACY_INPUT_TEXT (or similarly named) to FlxDefines. make LEGACY the default behavior, if flixel-ui is being used (for backwards compatibility), where the new input text` is a typedef for the old one
  • PR to Flixel-ui to check for the NEW flag to make the legacy input text a typedef for the new one
  • make all other flixel-ui systems work with either
  • add deprecation warning to old input text, mentioning the NEW flag
  • in a future major release, make NEW the default behavior and LEGACY the user-flag

Lastly, @ShaharMS made https://github.com/ShaharMS/texter. They wanted to put it in flixel as well, but I thought it should stay in flixel-ui, which was a bad call. We should study theirs, as well, it boasts about "RTL support", which would be nice!, and might work on flash, iunno

This also reminds me, we should make a demo for this, mostly so I'm aware of all it's features, in comparison to the old one

@Starmapo
Copy link
Contributor

Unfortunately the Flash issue is deeper than I assumed, since this text input uses private variables and functions from OpenFL classes, which are not available when compiling to Flash. I suppose that's the consequence of relying heavily on OpenFL's text input implementation.

We should study [texter], as well, it boasts about "RTL support", which would be nice!, and might work on flash, iunno

It seems to extend the flixel-ui text input, so if that one works on Flash, I think texter would too.

@ShaharMS
Copy link

Unfortunately the Flash issue is deeper than I assumed, since this text input uses private variables and functions from OpenFL classes, which are not available when compiling to Flash. I suppose that's the consequence of relying heavily on OpenFL's text input implementation.

We should study [texter], as well, it boasts about "RTL support", which would be nice!, and might work on flash, iunno

It seems to extend the flixel-ui text input, so if that one works on Flash, I think texter would too.

IIRC texter's input text does work on flash, and i don't mind providing an implementation for flixel

the main question is how advanced we want the input to be (multiline, copy-paste, selection shortcuts...)
all should be somewhat easy to implement if i remember my design from back then correctly

@Starmapo
Copy link
Contributor

I believe the only thing missing from texter that's in my implementation is full support for text selection; other than that, it seems like a better contender for a text input inside the main Flixel library.

@ShaharMS
Copy link

yeah just noticed after doing some testing

for me there are 2 main question about the implementation:

  • should it support "advanced" selection functions (arrows + ctrl + shift stuff)
  • should Bidirectional text support be implemented, and if so in which form:
    • dynamic (inputs letters at different places, depending on user expectation): pros: may be faster, cons: messier to implement, harder to export clipboard data out of
    • engine (processes the last input with the last owrd, for example): pros: easier to make more consistent, interfaces well with external text sources, easily disabled. cons: performance may be impacted, as it tokenizes strings.

the other stuff is pretty much settled i think (common shorcuts, multiline, alignments...)

@Geokureli
Copy link
Member Author

Let's start with "what is the best way to detect character input and other text-related keyboard input". I'd like to make a standalone util for this. if possible, I would like to make things that work with both FlxText and FlxBitmapText

@ShaharMS
Copy link

Already done in texter, and not too complex - 3/2 event listeners:

  • window.onTextInput
  • window.onKeyDown
  • window.onFocusLost (needed in web)

TextInput for regulat text, KwyDown for keyboard shortcuts & special characters, and the focus one is for js for when you click out of the openfl element

@Geokureli
Copy link
Member Author

Geokureli commented Jun 13, 2024

  • window.onTextInput
  • window.onKeyDown
  • window.onFocusLost (needed in web)

Any chance you can reduce that down to barebones, I don't really understand what bidi means and the code is a little hard to follow.

what would be nice is if it reduced all events down to a single action that anything could implement, like

enum TypingAction
{
   ADD_CHAR(charCode:Int);
   MOVE_CURSOR(type:MoveCursorAction);
   MOVE_SELECTION(type:MoveCursorAction);
   COMMAND(cmd:TypingCommand);
}

enum MoveCursorAction
{
    LEFT;
    RIGHT;
    UP;
    DOWN;
    HOME;
    END;
    LINE_LEFT;
    LINE_RIGHT;
    WORD_LEFT;
    WORD_RIGHT;
}

enum TypingCommand
{
   COPY;
   CUT;
   PASTE;
   SELECT_ALL;
}

@Starmapo
Copy link
Contributor

Starmapo commented Jun 17, 2024

Let's start with "what is the best way to detect character input and other text-related keyboard input". I'd like to make a standalone util for this. if possible, I would like to make things that work with both FlxText and FlxBitmapText

I do like that idea, though does that mean the base FlxText and FlxBitmapText classes will have input support? Or will you still have to use extended classes like FlxInputText?

I ask because I'm down to write at least a draft of the code, I've finished up my final works for school so I'm a lot more free now.

@Geokureli
Copy link
Member Author

I do like that idea, though does that mean the base FlxText and FlxBitmapText classes will have input support? Or will you still have to use extended classes like FlxInputText?

No, FlxText and FlxBitmapText will not have input, both will have extensions for input versions, the stand-alone tool will be there to unify the behavior of both

@Starmapo Starmapo mentioned this pull request Jun 19, 2024
13 tasks
@Starmapo
Copy link
Contributor

Starmapo commented Jun 19, 2024

I've opened a draft for my new text input implementation here #3191. It's still heavily work-in-progress, any feedback or suggestions are very much appreciated, mainly in regards to the coding structure.

@Starmapo
Copy link
Contributor

Starmapo commented Aug 7, 2024

@Geokureli You can close this one now btw

@Geokureli Geokureli closed this Aug 7, 2024
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.

3 participants