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

Editor: report warnings about possibly unlinked event handlers #2603

Conversation

ivan-mogilko
Copy link
Contributor

@ivan-mogilko ivan-mogilko commented Dec 4, 2024

As suggested by @messengerbag, as a most primitive solution for #2550.
...and a complementary to #2556

This adds another type of post-compilation warning that reports a script function that looks like it's a event handler, but not linked to the object events.

The feature uses default function names, as configured in Interaction tables.
For example: cEgo_Look, Room_Load and so forth.

The reported warning uses following pattern:
Function "ObjectName_EventName" looks like an event handler, but is not linked on ObjType (ID) ScriptName's Event pane

Example:
warnings--eventhandlers

TODO:
Option in Editor Preferences that turns this on and off?

Need to decide what happens if user double clicks the warning. Do we want it to go to the script, or to the object's Events Pane?
Or have both options, called from a context menu?
See also: #2589

Unfortunately, Autocomplete data, which we use for this kind of scanning, contains the index of the function's starting character, rather than a line number. I think the autocomplete may be amended to also have line numbers for script tokens.

@ivan-mogilko ivan-mogilko added context: ui/ux context: game building related to compiling the game from input assets labels Dec 4, 2024
@ericoporto
Copy link
Member

ericoporto commented Dec 4, 2024

This is great! I think an option in Editor Preferences to turn this off is fine, just important to make sure the default for this is on. If this check is thought as being a build setting, then perhaps it's best that is done in the general settings in the build section.

Need to decide what happens if user double clicks the warning. Do we want it to go to the script, or to the object's Events Pane?
Or have both options, called from a context menu?

Having both options seems fine but the default double click option should perhaps be going to the events pane where an action needs to be done. Like it could go to the script where the thing is but if someone gets a bunch of those they will probably want to go through it in batch like manner double clicking from the list.

I don't remember if the output panel can show a tooltip with the contents of a line when mouse is over it, but it would be useful for reading long lines.


While both things are similar, in a different language/platform, a function looking like a specific function feels like something a linter would point out, and the other feels like something that a linker would pick up. I don't know if this is relevant, just mentioning.

@ivan-mogilko ivan-mogilko force-pushed the 362--checkeventfunctions-notinevents branch from fd20666 to 6c20709 Compare December 5, 2024 16:23
@ivan-mogilko
Copy link
Contributor Author

Added following:

  • Double clicking on "unlinked function" warning zooms into the script file. I was conflicted here about opening an object's event tab, because maybe user would like to see the function itself first, before deciding what to do with it?
  • Double clicking on "missing function" warning zooms (at least tries to) into the event tab where this function is assigned.
    In order to do this I had to move the parts of "Go to Definition" command code from ScriptEditorBase to GUIController.
    Also changed ShowItemPaneByName to have a bool return type.

@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented Dec 5, 2024

On a side note, first I checked if it's going to be quick to add "line number" to function entry in AutoComplete, but no. Unfortunately, AutoComplete parser simply does not have a record of "current line number". So adding this would require more work on its own.

On another hand, apparently the "SpeechCenter" plugin does NOT use AGS Editor's AutoCompleteData, as I suspected previously, but has its own script parser. In retrospect that makes sense, because it has to find particular function CALLS, not function declarations.

I don't know many editor plugins, and from the ones I heard of this is probably the only one that does thorough script parsing. So idk if there's any editor plugins that would access AutoCompleteData, maybe not.

There's currently a problem with AutoComplete entry classes (ScriptToken and descendants) not using properties. They have bare public fields in them, which is really bad for a public interface.
I'm not an expert in C#, and don't quite know how it works in case of dynamic assembly linking (as in case of editor->plugin link). But I think the AutoComplete token classes must be modified to have readonly public properties in them, and have the regular fields made private, to secure them from direct access by plugins.

Depending on a message class, there may be "Go to Script" and "Go to Object" commands.

OutputPanelItem provides list of actions and implements them.
@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented Dec 6, 2024

I went a little further, and moved message-specific behavior to OutputPanelItem class, which is a descendant of ListViewItem. This class wraps CompileMessage, and implements interaction with it.
Initially I had an idea to have subclasses per a type of CompileMessage, but decided to keep a single "base" class for the time being.

OutputPanelItem returns a list of Actions, which OutputPanel fills into the context menu.
There are 2 methods: Action(string command) and DefaultAction(). First is called when user selects a menu item, and second is called whenever user double-clicks an item.

In practice:

  • a "Missing handler" message shows "Go to Object" menu command;
  • a "Possibly unlinked handler" message shows both "Go to Script" and "Go to Object" menu commands.

I think I'll stop here with functional changes.
On second thought I decided to not add editor preferences now, unless users ask for it, because such warnings are not likely to appear in large quantities; and they may be too important to ignore.

There's still an issue that Editor does not support going to a room object, but I suggest to address this problem separately, because it will affect "Goto Definition" as well.

@ivan-mogilko ivan-mogilko marked this pull request as ready for review December 6, 2024 14:45
@ericoporto
Copy link
Member

This works, but I think there is some weird interaction in case of room scripts. If a room script has like an oObject_Interact and it is not linked to the object, it will show the message, double clicking it will go to the script - I understand, I saw the issue to address the problem separately - but the big issue is that it then clears all warning messages. I think this is just some other bug in AGS Editor, I believe this is part of #1439.

So in summary, it looks like this works, but particularly for room scripts, it is hard to follow reported warnings. But this looks very useful, and the issue looks like something that can be fixed.

@ivan-mogilko
Copy link
Contributor Author

it will show the message, double clicking it will go to the script - I understand, I saw the issue to address the problem separately - but the big issue is that it then clears all warning messages.

Do you mean that it clears instantly after opening a room script, or after some other action from your side?

@ericoporto
Copy link
Member

Ah, I understand now, it is that I instinctively save after editing. I always ctrl+s after editing a file, which makes the room file rebuild, which then clears all warnings.

2024-12-07.18-45-39.mp4

@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented Dec 8, 2024

Ah, I understand now, it is that I instinctively save after editing. I always ctrl+s after editing a file, which makes the room file rebuild, which then clears all warnings.

Hmm, this behavior on video looks strange.
So you are in a room script of a room which had warnings reported for, you save, but all the warnings disappear, including the warnings related to that room.
I can see how it may be annoying that all the other room warnings disappear. That may a be an issue, but it's related to the output panel behavior. (EDIT: but frankly that's consistent with the standard IDE behavior: when you recompile 1 file, it only displays result of this last recompilation, not other things. Do I remember right that in ags3 rooms are recompiled when their scripts are saved? Maybe that behavior can be modified separately.)

But I think that current room warnings should reappear again, since you did not fix them and saved again. Probably the handler checks are skipped in some case.

@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented Dec 8, 2024

Do I remember right that in ags3 rooms are recompiled when their scripts are saved?

So, that's where this issues comes from. Saving just the room script does not recompile neither the script nor the room, and yet the output pane is cleared.

But furthermore, there's no such thing as "save room script" or "save script" in AGS Editor. There's just "Save" that saves whole game... That's why it clears the output.

This is a general problem though. If you had compilation errors, and hit Ctrl + S, that will also clear them until the next Build or Run command.

@ivan-mogilko
Copy link
Contributor Author

Well, it seems that the remaining issues are general issues of the rooms and output panel, so i'll merge this feature.

@ivan-mogilko ivan-mogilko merged commit e085482 into adventuregamestudio:master Dec 10, 2024
21 checks passed
@ivan-mogilko ivan-mogilko deleted the 362--checkeventfunctions-notinevents branch December 10, 2024 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
context: game building related to compiling the game from input assets context: ui/ux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants