scp-fs2open / fs2open.github.com

Origin Repository for SCP FreeSpace 2 Open
https://www.hard-light.net/
Other
407 stars 162 forks source link

`[sg]et-variable-by-index` considered confusing #6400

Open naomimyselfandi opened 1 week ago

naomimyselfandi commented 1 week ago

There've been multiple instances of new FREDers trying to use get-variable-by-index and set-variable-by-index instead of Replace Variable, and being extremely confused by variables as a result. These operators are extremely niche, and now that we have containers, I'd even go so far as to call them obsolete. I think these operators should be hidden in FRED to avoid confusion.

Variable types also probably play into the confusion: creating a variable but getting the type wrong causes Replace Variable to be greyed out, which isn't good for discoverability; there might be room for a discussion about how to tweak this facet of the UI.

Goober5000 commented 1 week ago

Perhaps. I'd rather not hide a perfectly usable feature just because it might be confusing though. Are there other ways to improve the situation, perhaps by adding detail to the SEXP help?

That's a good point about Replace Variable... one thing that comes to mind is adding a tooltip, something like "There are no [numeric|string] variables that can be used here."

naomimyselfandi commented 3 days ago

I don't agree that [sg]et-variable-by-index are "perfectly usable". They require the FREDer to be aware of variable indexes, which, to my knowledge, aren't shown anywhere in FRED; they require the FREDer to adopt a naming convention such that FRED reordering variables won't break them; and they can fail at runtime with no advance warning. While the first two problems could be addressed through documentation, that doesn't change the fact that using these operators requires knowledge of implementation details. I have a hard time calling that "perfectly usable".

Beyond that, this statement is at odds with the way the SCP uses deprecation. There are multiple examples of a new feature being accompanied by the deprecation of operators that are superseded by that feature. Containers solve the same problem that [sg]et-variable-by-index do, but they do it without the above problems. Deprecating them is therefore consistent with the de facto deprecation policy.

Lastly, I never said that they "might" be confusing; I said that I've seen people been confused by them. I'm not speculating.


As for Replace Variable, something like that's what I'm thinking of, but I don't think a tooltip is the right solution; users often don't read them, especially when they're confused (and beyond that, they'd need to hover over it, but the option being greyed out makes them unlikely to do that). Having thought about it further, I think there are two cases:

Goober5000 commented 2 days ago

I don't agree that the introduction of a new feature necessarily means the old feature should be deprecated, but the rest of your points in the first part are pretty solid. Especially the fact that the proper use of these operators require knowledge of implementation details. Fair enough, that's a pretty convincing case.

In the second part, those are some good thoughts. I don't think we should allow users to select a variable of the wrong type, because it feels like a gotcha: "you think you can use this, but nope! psych!". It feels like it's almost there, though.

I'll think about the UI thing. The SEXP operator hiding can be done with just a few lines of code, but the UI modification is going to be trickier.