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

Add Inputvalue.disable_on_enter #4849

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

Conversation

Gouvernathor
Copy link
Member

A nice feature that was in a cookbook and is so straightforward that it deserves to be builtin.

@Gouvernathor Gouvernathor added the enhancement A proposed or requested enhancement or improvement to renpy's features. label Jul 18, 2023
@mal
Copy link
Member

mal commented Jul 18, 2023

I don't think this makes sense on the base input value. It seems too niche to see widespread use, while also being too simple for most cases that want to manipulate the editable state of inputs, such that anyone really wanting this functionality is probably going to have to define their own class anyway to do the extra bits.

Perhaps there could be some limited value to separate ActionInputValue (or something) class which could take an action to perform on enter, with a special case for using it's own Disable, but even then, allowing the key press to propagate and using keysym with a button is probably a better choice for cross-platform accessibility (as there's a clear corresponding click/tap action for touch devices).

@shawna-p
Copy link
Contributor

It seems too niche to see widespread use

I strongly disagree; I have an InputValue subclass with this functionality which I port around to every project I work on. If you want any kind of input screen that involves more than one input value or involves anything aside from just one singular input (e.g. selecting other character features + inputting a name), you need to be able to dismiss the keyboard on mobile, and being able to disable input on non-mobile devices allows you to use keyboard shortcuts again.

In fact, I'd argue this has good reasons to be the default - if someone isn't familiar with input values, they will use renpy.input, and that's fine, but in virtually every other scenario I've ever used input, I always want a way to disable it. The most common use case for input is, I believe, naming the player character. I use this kind of input for character names on customization screens, and I don't want to return anything until the player is done customizing the other aspects of their character on that screen.

@mal
Copy link
Member

mal commented Jul 18, 2023

This isn't about facilitating the disabling of the input, that is already possible with the existing Disable action attached to the InputValue. It's about disabling the field once enter is pressed, which in isolation isn't terribly useful unless you've done a bunch of other groundwork to allow re-enabling it, or some other way of triggering progression and processing of the value provided.

@shawna-p
Copy link
Contributor

Once again, I strongly disagree with your idea that this is not "terribly useful". I am saying this as someone who has worked on dozens of games and always needs this kind of functionality for usable mobile input, and for screens with multiple input values. It isn't relevant that there is an alternative way to do this with the existing Disable action - the question is, can we make this easier to accomplish, and the answer is, yes, by adding an incredibly simple argument which won't affect any existing input values unless they opt into it.

This functionality is already built into the game in the file_slots screen with a clear example on how to set up and use this sort of input - the issue is that this functionality is exclusive to the FilePageNameInputValue class by default. The setup involves the following:

default my_input = VariableInputValue("something")
button:
  key_events True
  action my_input.Toggle()
  input value my_input

If I use key to disable the input when Enter is pressed and I have multiple input values on the screen which could be active, I have to disable every one of them, when it would be simpler to just have each input be in charge of disabling itself.

I feel you're approaching this from the wrong angle - the question has never been "can Ren'Py already do this", to which the answer is clearly yes, it can. The question is if we can make it easier for devs to add this functionality into their games, and the answer to that is also yes. I've given several reasons why I think it's a useful feature to boot.

@mal
Copy link
Member

mal commented Jul 18, 2023

I'm not saying this has no use, and that we can't do better in the realm on input management, we absolutely can, however I do feel like this feature as presented falls in a middle ground of scratching an itch indicative of a more general issue without really leading towards a solution of that wider problem.

For example, to the point about multiple inputs specifically, we know at the engine level that only one input can be active at once, so perhaps something to look at as part of a broader solve would be in ensuring that is the case so it's harder to get into the state of having multiple active inputs in the first place (i.e. when one is enabled, others can't be), which would benefit not only this issue, but the wider ecosystem.

Please don't take this as my being against improving the situation, I'm not. I also deal with this issue when building games, I'm just not convinced that this particular addition moves us towards a better system overall and am wary of expanding a public API for what may be a short term solve but that will need to be supported for the next decade and beyond.

@Gouvernathor
Copy link
Member Author

Gouvernathor commented Jul 18, 2023

Your proposition of taking another Action as a parameter, to be run when enter is pressed (or as a parameter to a subclass of InputValue, whatever), is more generalized than this, sure, but it also doesn't solve the issue in any simple way. You can't pass a value's Disable action to the value itself at the moment of its own creation, you have to first define the value in a screen variable, then at best set an enter_action attribute to the Disable attribute of the same value. That's a two-liner and that's pretty ugly in screens, in addition to messing with screen optimization. And it's not that much simpler or evident for your typical creator than Fen's initial solution of a subclass.
If we stick to an improvement where what happens at enter is passed by parameter, self.Disable can only be a special-case triggered by a flag value. So, your solution could be a supplement to this one, but I don't think it would have much practical usability on its own.

That being said, I didn't understand everything in that matter of multiple inputs active at the same time but instead of disabling just itself, the input value could run the DisableAllInputValues action. If we assume that only one input is active at any given time, the result is the same - only the one active input, catching the enter keystroke, would get disabled. And we can branch back to your proposition of rather taking an action as a parameter - though I think adding a little syntax sugar on that one wouldn't hurt.

Also, you seem to describe this as something added to all InputValues and while implementation-wise it's true, it's only documented as a new feature on each separate subclass. Just like the returnable feature.

@mal
Copy link
Member

mal commented Jul 18, 2023

I did explicitly note that using it's own disable would need to be special cased ...

It's a moot point anyway as that was more an example of a broader approach than it was any real advocation for such a thing (the problems extend beyond that).

I've laid out my opinion: very specifically that this aims to treat a symptom - disabling an input in a multi-input scenario - rather than the cause - that Ren'Py was designed with a view towards there being only one input on screen, and multi-inputs being sufficiently advanced that creators should handle that in their entirety - and that if we want to do a better job making multi-inputs more accessible it's much wider topic than this, so I'll leave it there.

Remember that it is just my opinion and thankfully we don't all have to agree, that's why we have Tom. 😛

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A proposed or requested enhancement or improvement to renpy's features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants