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

New text input object #3191

Closed
wants to merge 14 commits into from
Closed

Conversation

Starmapo
Copy link
Contributor

@Starmapo Starmapo commented Jun 19, 2024

After #3178 was opened and I figured that my original text input library just would not work with Flash, I've started making a new one from the ground up, which does not depend on any internal OpenFL functions/variables.

It's still a work-in-progress as of now, you're free to request any changes of my implementation.

Showcase of its current state:

FlixelTesting_FmhZzLjwDU.mp4

To-do list:

  • FlxInputText
    • Multiline variable
    • Place caret according to the mouse
    • Text selection
    • Text scrolling
    • Action callbacks (input, delete, etc.)
    • Force case variable (flixel-ui)
    • Filter mode variable (flixel-ui)
    • Block other hotkeys while input text is active (e.g. volume mute)
    • Mobile support
    • Caret flashing on/off
    • Test how well it works with multiple text formats
    • Documentation

@Geokureli
Copy link
Member

Geokureli commented Jun 19, 2024

I've taken the quickest of glances at the source and this looks promising!

Edit: Multiline should already be handled by FlxText

- Regenerate text graphic when `passwordMode` changes
@Geokureli
Copy link
Member

Geokureli commented Jun 19, 2024

Also, fwiw, FlxBitmapInputText is not a requirement for this PR, if you only have time for FlxInputText we can always do bitmap text sometime later, after this merge

in fact not everything in the FlxInputText TODO needs to be done in the first pass either, my minimum requirement is whatever it takes to make people stop using addons and your flixel-text-input lib. we can see if Shahar wants to add RTL

@@ -19,6 +19,8 @@ class FlxInputText extends FlxText implements IFlxInputText
public var hasFocus(default, set):Bool = false;

public var maxLength(default, set):Int = 0;

public var multiline(get, set):Bool;
Copy link
Member

Choose a reason for hiding this comment

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

So I was wrong, it's not in FLxText, but it should be... or at last input text shouldn't be different from FlxText in this regard.

Copy link
Contributor Author

@Starmapo Starmapo Jun 19, 2024

Choose a reason for hiding this comment

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

Should I make a PR to add it to FlxText, or will you add it instead?

Copy link
Member

Choose a reason for hiding this comment

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

not sure when i'll get to it, but i can

@Starmapo
Copy link
Contributor Author

Also, fwiw, FlxBitmapInputText is not a requirement for this PR, if you only have time for FlxInputText we can always do bitmap text sometime later, after this merge

in fact not everything in the FlxInputText TODO needs to be done in the first pass either, my minimum requirement is whatever it takes to make people stop using addons and your flixel-text-input lib. we can see if Shahar wants to add RTL

Alright, I'll update the to-do list accordingly.

- Add `setSelection()` function
- `FlxInputText` variables are now destroyed properly
- Added `scrollH`, `scrollV`, `bottomScrollV`, `maxScrollH` & `maxScrollV` variables
- Return end of text if character isn't found at position
- Selection boxes are now clipped inside the text bounds
- Simplified getting the Y offset of a line
- Fix scrollV not being able to be modified directly
@Geokureli
Copy link
Member

Geokureli commented Jun 21, 2024

what is "text scrolling", is that and "force case" something that should also go in FlxText?

@Starmapo
Copy link
Contributor Author

Starmapo commented Jun 21, 2024

what is "text scrolling", is that and "force case" something that should also go in FlxText?

Text scrolling is being able to scroll the text horizontally and vertically (if all the text doesn't fit in the bounds of the sprite), which for FlxInputText the user can do by scrolling the mouse wheel or moving the mouse around while selecting text. I suppose it could also have some uses in FlxText independent of text input, maybe even the mouse wheel scrolling could be an optional feature for base FlxText (it seems to already be the case for OpenFL text fields).

forceCase is in flixel-ui's FlxInputText and it just forces all letters the user types to be either lowercase or uppercase, I don't think it has any use for non-input text.

@Geokureli
Copy link
Member

if these features are already in ui.FlxInputText then adding them to FlxText would require a PR to flixel-ui for compatibility. so just keep doing it how you're doing it and we'll migrate these to FlxText some other time. thanks for the info

- Selection sprites now just change their color instead of making new graphics
- scrollH can now be modified properly as well
- Word wrap no longer changes with multiline (multiline only affects adding new lines)
- Caret is now positioned properly with different alignments
- Caret is now clipped inside the text bounds
- Caret is now automatically resized when changing `bold`, `font`, `italic`, `size` or `systemFont` variables
- Fixed crash when pressing down a key while there isn't a focused input text
- Fixed selected text format overwriting the border color
- Fixed caret not being visible when text is empty
- Fixed selection boxes sometimes not being updated immediately
- Added `useSelectedTextFormat` variable
- Double press check is now when the mouse is released (same as OpenFL)
- Moved action callback types to an enum abstract
@Geokureli
Copy link
Member

Unsubscribing for the moment, if you need me for anything, ping me please

@Starmapo
Copy link
Contributor Author

There's still a handful of things left so I think I'll just close the PR for now and reopen it once it's ready.

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