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

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

Open
naomimyselfandi opened this issue Oct 22, 2024 · 3 comments
Open

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

naomimyselfandi opened this issue Oct 22, 2024 · 3 comments
Labels
discussion This issue has (or wants) a discussion sexps A feature or issue related to SEXPs

Comments

@naomimyselfandi
Copy link
Contributor

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.

@naomimyselfandi naomimyselfandi changed the title [sg]et-variable-by-index considered highly confusing [sg]et-variable-by-index considered confusing Oct 22, 2024
@Goober5000 Goober5000 added discussion This issue has (or wants) a discussion sexps A feature or issue related to SEXPs labels Oct 23, 2024
@Goober5000
Copy link
Contributor

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
Copy link
Contributor Author

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:

  • The user hasn't added any variables at all. This feels like less of a problem, because "you need to create something before you can use it" is pretty intuitive; that said, we could make the option available, but have it expand to a list with only single greyed-out item, "No variables - use Add Variable to create one", to be explicit.
  • The user has added variables, but none of the right type. This is where I've seen users get confused. We could do the same thing and show a single greyed-out item, "No [type] variables - use Add Variable, or Modify Variable to convert a [type] variable", but that's pretty long. Another approach would be to have Replace Variable show variables of the wrong type if none of the correct type exist, and present an error message on trying to use them: "I expected a [type] variable here, but [variable] was defined as a [other type]. If that was a mistake, you can use the Modify Variable option to convert it."

@Goober5000
Copy link
Contributor

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.

Goober5000 added a commit to Goober5000/fs2open.github.com that referenced this issue Nov 3, 2024
Hide these SEXP operators to reduce confusion for FREDders.  They still work, but using them properly requires a certain amount of knowledge of the inner workings of variables, and the operators can break if new variables are added causing the list to be re-indexed.

Implements half of scp-fs2open#6400.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion This issue has (or wants) a discussion sexps A feature or issue related to SEXPs
Projects
None yet
Development

No branches or pull requests

2 participants