search

scp-fs2open / fs2open.github.com

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

FRED2 hotkeys in event editor allow impossible (and crash-causing) sexp combinations #4405

Open MoerasGrizzly opened 2 years ago

MoerasGrizzly commented 2 years ago

FRED2 hotkeys in event editor allow impossible (and crash-causing) sexp combinations.

Steps to reproduce: Copy a sexp. Right click another sexp When paste (overwrite) is not possible, press the hotkey for paste (overwrite). when paste (add child) is not possible, press the hotkey for paste (add child).

Whilst I'm here anyway, the "delete" hotkey never works in the event editor.

jg18 commented 2 years ago

There are related issues with being able to use these keyboard shortcuts when you shouldn't be.

For example, if you try Ctrl+V in the Events Editor on a node when nothing is on the clipboard, FRED crashes with an assertion failure.

Not sure yet what's going on with the keyboard shortcut for Delete Item not working.

jg18 commented 2 years ago

Looks like this'll be a really hard problem to fix, due to the hackish nature of the FRED codebase (see FRED's About dialog).

The code that determines which of Delete Item/Cut/Copy/Paste/Add-Paste should be enable is tightly interwoven with the code that builds the right-click menu.

I looked into pulling the former apart from the latter and ultimately gave up, deciding that it'd make a bad situation much worse.

While the reported issue is definitely legit, the conclusion may have to be that FRED users use these keyboard shortcuts "at their own risk" and it's assumed they know what they are doing.

If FREDders use the context menu only, then it should be guaranteed that the menu options are enabled only when they're applicable.

It's probably best to not even add keyboard support for "Delete Item". Since there'll be no safeguards that it'd be usable only when it should be, and since the action is extremely difficult to reverse, we don't want to make it extra easy for people to inadvertently delete stuff.

The best solution there is probably to remove the keyboard shortcut for Delete from the UI, so it's clear that deleting by keyboard shortcut isn't supported.

Unfortunately, I think we'll have to punt to QtFRED for providing a proper solution here.

MoerasGrizzly commented 2 years ago

Ah, I experienced an assert failure but I couldn't reproduce it. Good to know I wasn't dreaming. Thanks for your work on this :-)

jg18 commented 2 years ago

Unfortunately, I think we've hit the limit on what we can fix during this RC cycle.

Further fixes will require more sweeping (and thus riskier) changes and consequently are better done sometime after a stable release is out.

Even though we can't guarantee any ETA for a fix, we appreciate the report, especially given repro instructions.

z64555 commented 2 years ago

Punting to 22.4 upon jg18's recommendation.

TRBlount commented 1 year ago

@jg18 Should we keep this on the board for 23.0 or shift it to 23.2?

Goober5000 commented 1 year ago

Bumping to 23.2.

Goober5000 commented 1 year ago

Bumping to 23.4. As jg18 said:

Further fixes will require more sweeping (and thus riskier) changes and consequently are better done sometime after a stable release is out.

Goober5000 commented 9 months ago

I spoke with @jg18 and the situation is pretty much unchanged. He doesn't have the bandwidth to take this on currently, and neither do I, so I suggest punting this to 24.2.

Essentially, this part of the code needs to be refactored into a two-stage process: 1) determine whether the action is allowed, and 2) build the popup menu. After the refactoring, the "allowed" checks can be used for both the popup menu and the shortcut keys.