-
Notifications
You must be signed in to change notification settings - Fork 422
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
Allow TextBox
to specify type of text being input
#6408
base: master
Are you sure you want to change the base?
Conversation
Compilation failures here |
Noticed, fixed. |
463030f
to
5a34404
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks okay.
I'm not sure about IME being a property you have to set as opposed to being optional defaulting to true
, or maybe even null
and a sane default chosen per type (IME doesn't make sense for numbers/passwords, but maybe this is blocked at and SDL level?).
Co-authored-by: Dan Balasescu <[email protected]>
base.StartTextInput(allowIme); | ||
ScheduleCommand(() => Imm.SetImeAllowed(WindowHandle, allowIme)); | ||
base.StartTextInput(properties); | ||
ScheduleCommand(() => Imm.SetImeAllowed(WindowHandle, properties.AllowIme)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My question here would be why this is not
ScheduleCommand(() => Imm.SetImeAllowed(WindowHandle, properties.AllowIme)); | |
ScheduleCommand(() => Imm.SetImeAllowed(WindowHandle, properties.AllowIme && !properties.Type.IsPassword())); |
or similar (same for SDL2 pathway). Seems like it'd be a sane default?
Incidentally I'm not even sure why we still have this windows specific code. Why is it not all delegated to SDL? @Susko3 can you shine a light on that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made AllowIme
default and added an auxiliary check to disable IME where not supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bdach When I first tested the SDL2 implementation of text editing events, it wasn't working as expected on windows. Doing a quick test of the SDL3 impl, it seems to work just fine. I'll do a bit more testing and make a PR changing SDL3WindowsWindow
to use the SDL impl on windows.
osu.Framework/Input/InputManager.cs
Outdated
string detail = d is ICanSuppressKeyEventLogging kd && kd.SuppressKeyEventLogging | ||
? e.GetType().ReadableName() | ||
: e.ToString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this actually... suppress? Because in debug, when I run the game, and type into a password text box, key events are still logged. And that's because they're being logged in a completely different location, namely
osu-framework/osu.Framework/Input/ButtonEventManager.cs
Lines 135 to 136 in d4d9cca
if (handledBy != null) | |
Logger.Log($"{e} handled by {handledBy}.", LoggingTarget.Runtime, LogLevel.Debug); |
So does this really do anything? Or did it just break at some point and nobody noticed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it did regress, since key input used to be logged in this location until it was moved to KeyEventManager
. I'll correct it in this PR.
{ | ||
window.StartTextInput(allowIme); | ||
window.StartTextInput(properties); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't appear to work correctly on android. When switching from a normal textbox to a password textbox immediately, IME remains available. Conversely, when switching from a password textbox to a normal textbox immediately, IME remains unavailable.
Record_2024-11-08-11-57-01.mp4
I wonder what this looks like on iOS. (I own an iOS device now, but no mac, so I still can't check myself.)
Whether this is an SDL bug, or ours, is up for debate I guess. I can say that the naive solution, i.e.
diff --git a/osu.Framework/Input/SDLWindowTextInput.cs b/osu.Framework/Input/SDLWindowTextInput.cs
index 79fa1d848..c67bf7f77 100644
--- a/osu.Framework/Input/SDLWindowTextInput.cs
+++ b/osu.Framework/Input/SDLWindowTextInput.cs
@@ -10,6 +10,8 @@ internal class SDLWindowTextInput : TextInputSource
{
private readonly ISDLWindow window;
+ private TextInputProperties? startedInputProperties;
+
public SDLWindowTextInput(ISDLWindow window)
{
this.window = window;
@@ -42,10 +44,14 @@ protected override void ActivateTextInput(TextInputProperties properties)
window.TextInput += handleTextInput;
window.TextEditing += handleTextEditing;
window.StartTextInput(properties);
+ startedInputProperties = properties;
}
protected override void EnsureTextInputActivated(TextInputProperties properties)
{
+ if (startedInputProperties != null && startedInputProperties != properties)
+ window.StopTextInput();
+
window.StartTextInput(properties);
}
@@ -54,6 +60,7 @@ protected override void DeactivateTextInput()
window.TextInput -= handleTextInput;
window.TextEditing -= handleTextEditing;
window.StopTextInput();
+ startedInputProperties = null;
}
public override void SetImeRectangle(RectangleF rectangle)
does not appear to help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing roughly the same issue on iOS, I will have to look into libsdl-org/SDL#11406 (comment) again first to tell whether this is an issue on SDL's side or ours. Not sure about Android though, someone else will have to investigate this (libsdl-org/SDL#11406 (comment) may be relevant).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After further investigation on the iOS issue, SDL doesn't handle updating text input properties appropriately on iOS, regardless of whether text input is active or not. I've fixed this in the PR linked above (libsdl-org/SDL@f4d81f2) for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...so i guess at this point it's either up to me to fix android in sdl, or to accept it's broken over there?
i feel like my android devices might mysteriously stop working soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can try to take a stab at fixing this in SDL. On first glance, it doesn't seem to be an android-specific issue as SDL will ignore requests to show the keyboard if it's already active.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created a PR, but I'm not expecting it to go in soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ResetIme()
still calls SDL_StartTextInput()
without properties.
@peppy fwiw with this PR in addition to libsdl-org/SDL#11699, users will be able to auto-fill their account passwords from a password manager with ease. EDIT: Ok maybe still broken somewhat, need more braincells. |
Also has the bonus feature of allowing password managers (e.g. 1Password) to be integrated with the game, by specifying the text input type as
Username
/Password
in the textboxes. Unfortunately it's still quite broken in SDL but should be fixable (see libsdl-org/SDL#11406 (comment)).Since text input types are now identifiable with the new
TextInputType
field, I've applied a refactor on theTextBox
classes to respect the type at aTextBox
level w.r.t. certain functionalities such as clipboard exporting, key event logging, character display, etc. This includes removingBasicPasswordTextBox
entirely as its behaviour can be achieved by simply:I've gone for this refactor as it didn't sit well for me how these implementations still exist while there is now a new field specifying the type of text input.