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

Allow TextBox to specify type of text being input #6408

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

frenzibyte
Copy link
Member

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 the TextBox classes to respect the type at a TextBox level w.r.t. certain functionalities such as clipboard exporting, key event logging, character display, etc. This includes removing BasicPasswordTextBox entirely as its behaviour can be achieved by simply:

new BasicTextBox
{
    InputProperties = new TextInputProperties(TextInputType.Password, allowIme: false)
}

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.

@bdach
Copy link
Collaborator

bdach commented Nov 4, 2024

Compilation failures here

@frenzibyte
Copy link
Member Author

Noticed, fixed.

Copy link
Contributor

@smoogipoo smoogipoo left a 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?).

osu.Framework/Input/TextInputProperties.cs Outdated Show resolved Hide resolved
Co-authored-by: Dan Balasescu <[email protected]>
base.StartTextInput(allowIme);
ScheduleCommand(() => Imm.SetImeAllowed(WindowHandle, allowIme));
base.StartTextInput(properties);
ScheduleCommand(() => Imm.SetImeAllowed(WindowHandle, properties.AllowIme));
Copy link
Collaborator

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

Suggested change
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?

Copy link
Member Author

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.

Copy link
Member

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.

Comment on lines 1006 to 1008
string detail = d is ICanSuppressKeyEventLogging kd && kd.SuppressKeyEventLogging
? e.GetType().ReadableName()
: e.ToString();
Copy link
Collaborator

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

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?

Copy link
Member Author

@frenzibyte frenzibyte Nov 9, 2024

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);
Copy link
Collaborator

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.

Copy link
Member Author

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).

Copy link
Member Author

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.

Copy link
Collaborator

@bdach bdach Nov 11, 2024

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.

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

}
}

public static bool SupportsIme(this TextInputType type) => type == TextInputType.Name || type == TextInputType.Text;
Copy link
Member

Choose a reason for hiding this comment

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

SupportsIme doesn't match the style of IsPassword and IsNumerical. Change to a switch or change the others to boolean logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure that matters.

Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants