Editor: add local variables to watch panel automatically

[ericoporto]

This change assumes local variables are usually meaningful in the context of the function being debugged.
The user can still add and remove variables to watch, for meaningful global variables.

This is a draft that perhaps should have been an issue, but anyway, I wanted to show to see what is useful or not from this. The limitation here is the GetListOfLocalVariablesForCurrentPosition function, which relies on parsers in both AutoComplete and ScintillaWrapper which aren't perfect - perhaps they could be improved with more testing.

2024-06-19.19-55-54.mp4

Made a fix for the blinking in the video above please try and see what you think of this change.

watch_panel.mp4

[ericoporto]

Added a very minimal left click context menu to add things to watch pane - I tried deducing if a string was actually a variable but it isn't simple, you need to check the autocomplete cache of all scripts using FindVariable and it still may come up empty if one of them failed to parse correctly, which can be very frustrating, it turns out a dumber approach that allows adding any word to the watchpane is better.

This is useful for things in the global context that aren't automatically added to the watch pane but that you still want to keep track.

[ivan-mogilko]

Sorry for not responding earlier, I'd like to review and deal with this PR draft now if possible.
@ericoporto could you please rebase this, it seems to have few conflicts with the recent ags4 branch?

[ericoporto]

Right, I will do it tonight. The reason I never took it off of draft status is it had an issue that it would occasionally lock the AGS game when I ran. After I rebase, if I find the issue again I will mention it.

The last commit is probably safe without the rest of it btw.

[ericoporto]

Hey, I did the rebase but I haven't been able to test it yet. :/

[ivan-mogilko]

There's something funny, having new Type anywhere in the code would add a local variable "Type" entry (of course it won't be resolved).

For example:

int arr[] = new int[1];

will add "int" variable to the watch panel.

Not sure which part of this feature is causing this.
EDIT: this is a mistake in ScintillaWrapper.GetListOfLocalVariablesForCurrentPosition()

I'll continue testing.

[ivan-mogilko]

So, I noticed that this method GetListOfLocalVariablesForCurrentPosition requires ScintillaWrapper, and as a consequence you have to get ScriptEditor control. While we may assume that a active script's control should be available when you hit a breakpoint, I think this should be avoided.

Looking at GetListOfLocalVariablesForCurrentPosition, it seems like this method does not have to be in ScintillaWrapper at all, because it does not access any of scintilla's functionality, but only AutoComplete functionality. It may be possible to move this method to AutoComplete class, where it would accept only IScript, and work with its Text and AutoCompleteData directly.

EDIT: hmm, the only thing that we need from scintilla in that case is line's position. I suppose that this position may be get without scintilla's goto by calculating linebreaks in script text.

At last, even if above is not done, I think the position may be received without calling "Goto", AFAIK Scintilla has a method to return text position for each given line, it may be exposed in ScintillaWrapper too.

[ivan-mogilko]

Overall, I think that this is a pretty good PR. This extra feature is achieved with a small amount of code, and is rather straightforward. The only questionable thing there is how local variables are scanned every time, but they are limited by a single function's scope, and it's rather the problem of autocomplete system. IMO it's a shame that AutoComplete does not cache local variables as it does globals. But this may be improved separately.

Something that bothers me slightly in the code is that extra stuff is done under _updateItemLock when syncing local vars, usually it's a good thing to minimize time spent under a lock by doing everything that does not have to be protected before or after. Maybe this code could be reorganized. OTOH I found a place in my earlier code that also could be improved for a similar purpose (one where it calls UpdateSingleWatch in a loop).

Bugs:

• ScintillaWrapper.GetListOfLocalVariablesForCurrentPosition() adds type names if it meets "new Type" syntax. This may be fixed separately from this PR, and in ags3 branch too, as it likely affects autocomplete in general.

---

I suggest adding a checkable option in the panel's context menu that turns automatic local variables on and off, in case a user does not want to see them.

Then, in my opinion, the context menu should not let Remove or Clear automatically added local variables. Since they get re-added after a single step anyway.

To summarize:

• local variables are added automatically only if this option is enabled;
• automatically added variables are always on top of the list;
• cannot be removed by Remove or Clear menu commands;
• removed completely if user unchecks the option.

---

"Add to Watch" menu command is a very nice addition, but it has couple of bugs:

1. It is enabled for any symbols not just variables,
2. When a variable gets added to the list this way, there's an extra empty item inserted before for some reason.

[ericoporto]

Thanks for the review, did you not get the issue where ags occasionally locks itself? That is the biggest issue for me, I couldn't progress much because I kept hitting the locking issue, it happens when debugging a game, I noticed in my Sandwalker game but I also got it in others, if I kept using the step function it would eventually lock the game - alt+tab to it would find a hang window.

@ivan-mogilko hey, I am having a hard time upgrading a 3.6.1 project to ags4, it looks like SaveAndDisposeBitmapAsync is getting called with the "Rooms\\1\\background0.png" before the directories are created, not super sure on this but at least it looks like it. This on current ags4 branch.

I "fixed" by using

        private Task SaveAndDisposeBitmapAsync(Bitmap bmp, string filename) => Task.Run(() =>
        {
            using (bmp)
            {
                string dir = Path.GetDirectoryName(filename);
                Directory.CreateDirectory(dir);
                bmp.Save(filename, ImageFormat.Png);
            }
        });

[ivan-mogilko]

did you not get the issue where ags occasionally locks itself? That is the biggest issue for me, I couldn't progress much because I kept hitting the locking issue, it happens when debugging a game, I noticed in my Sandwalker game but I also got it in others, if I kept using the step function it would eventually lock the game - alt+tab to it would find a hang window.

No, but I did not test for too long.
Does it happen in random moments, or at particular places?

To clarify, this locks the running game, not the editor, correct?

[ericoporto]

Correct, the Editor keeps working, it's only the game. I will make a video to show, because it's weird, and Windows gets unhappy that the game window is hang so it does that thing where it shows the "it has stopped working" message and gives you the option to kill it.

[ivan-mogilko]

This may be debugged by attaching another instance of MSVS to a running game process, after it was started by the Editor.

[ericoporto]

2024-11-04.20-11-35.mp4

Just to show the video of what I mean, if I "step into" a few times, it's like there is some desync, I can't actually load the game because it's actually halted by the editor and just hitting play (to go to the next break point) or pause (which I think just shows the next instruction) brings the game back and you can continue debugging. I will try using two MSVS so I can attach the second one to the engine and try to pickup what is going on.

Edit: erh, not sure what I was expecting... The engine is just looping in the while inside the break_into_debugger() function in the debug.cpp file. I am not sure if I am doing the double debugging of the debugger correctly, but it seems that for some reason at some point the Editor misses one BREAK command back from the engine.

Btw, I see I am getting a bunch of RECVVAR commands, I am curious if I made it that it could send all the vars in one command if it would make such situation better.

// my very small room script
Overlay* ovr[20];

function room_RepExec()
{
  int max = 20;
  for(int i=0; i<max; i++)
  {
    int randx = Random(Screen.Width-1);
    int randy = Random(Screen.Height-1);
    if(ovr[i] == null) {
      ovr[i] = Overlay.CreateGraphical(0, 0, 2100 + Random(10));
    }
    ovr[i].X = randx;
    ovr[i].Y = randy;
  }
}

[ericoporto]

I suggest adding a checkable option in the panel's context menu that turns automatic local variables on and off, in case a user does not want to see them.

Uhm, this is an interesting idea, I tried one way of making it here

ericoporto@9565600

The problem is... There is a bug in Windows 11 that the checkbox scales weirdly and may not be visible in context menus... (I think the bug is reported here dotnet/winforms#9258). Anyway, in my PC the checkbox is sort of hard to understand it's a checkbox.

I think this could be put in the Editor Preferences and be done, and if in the future we have more needs for the watchpanel we may add some toolbar in it like the log panel.

[ivan-mogilko]

The problem is... There is a bug in Windows 11 that the checkbox scales weirdly and may not be visible in context menus... (I think the bug is reported here dotnet/winforms#9258). Anyway, in my PC the checkbox is sort of hard to understand it's a checkbox.

I do not think that this is a good reason to not add an option into context menu. That's cosmetic issue vs convenient control element. Besides, there are already multiple checkable options in the Editor's menus: "Word Wrap" in the Edit menu, and items in Layout menu. Maybe we'll have a need to have other checkable options in context menus as well. That would be annoying to have to constantly think of workarounds for each such option only because of menu drawing issue.

I see following alternatives here:

1. Just add it, and tolerate that it does not look well on some systems until there's a fix.
2. Find a way to adjust their size with a custom code, or draw checkable items ourselves (do those menus allow to override paint operation?).
3. Add 2 options, one of which enables and another disables this mode, and one of them is either disabled or hidden depending on current choice.

Personally, I would just do p1, and then look for p2 solution as a separate task in spare time...

[ivan-mogilko]

and if in the future we have more needs for the watchpanel we may add some toolbar in it like the log panel.

Ah yes, toolbar is another good alternative.
Although, i'd consider sharing commands between toolbar and the context menu, but that's a matter of ui design.

Edit: erh, not sure what I was expecting... The engine is just looping in the while inside the break_into_debugger() function in the debug.cpp file. I am not sure if I am doing the double debugging of the debugger correctly, but it seems that for some reason at some point the Editor misses one BREAK command back from the engine.

I'll see if I can reproduce this today.

[ivan-mogilko]

I found that it's very easy to reproduce the hanging problem: just place a breakpoint inside a repeating script, and hold F5. It happens in a split second.

My impression is that this is because _communicator.SendMessage is being called from multiple threads and its private members are not being protected by any lock. EDIT: alternatively, DebuggingController, that owns communicator, could send messages under a lock.
Look like this is a general issue. It does not occur often maybe because it requires a big chain of messages to be sent simultaneously to trigger the problem.

On a separate note, the message exchange between editor and engine is implemented by waiting for an "ack" after each message, which may slow things down in theory, for very large number of messages, but idk if that's an immediate issue.

[ivan-mogilko]

@ericoporto I created a commit in my repo here: ivan-mogilko@145d5a9

This seems to fix the engine getting locked in the waiting state.

Please try if that works for you?

[ericoporto]

I confirm that fix makes the error go away!!!!

[ericoporto]

• local variables are added automatically only if this option is enabled;
• cannot be removed by Remove or Clear menu commands;
• removed completely if user unchecks the option.

I believe I did these.

• automatically added variables are always on top of the list;

I haven't yet done this, still haven't figured it out, I will try again tomorrow.

[ivan-mogilko]

automatically added variables are always on top of the list;
I haven't yet done this, still haven't figured it out

Is not that how it currently works? Last time I checked the code, i noticed that you are inserting local variables at index 0 in the list.

[ericoporto]

I thought it was a task list but I couldn't figure it what I did wrong.

Will look into the Add to Watch issues then.

[ericoporto]

I think I fixed the problems in the new features of auto-local vars and add to watch!

It is enabled for any symbols not just variables,

I don't plan to fix this in this PR, couldn't find a reliable way to filter for only variables.

Edit: was looking into another Scintilla based custom ide that has a watch pane just now

https://github.com/GeorgRottensteiner/C64Studio/blob/master/C64Studio/Documents/SourceASM.cs

I think we actually managed to organize things alright but it's nice to see other "similar" projects for ideas.

[ivan-mogilko]

Technically it is all working. I found several minor issues.

1. The Watch Panels' context menu now looks strangely different from other menus, because the "check" icon has non-standard size.
   Here's the comparison between script's context menu and watch panel's:
   

2. I'd suggest moving "Add to watch panel" command lower, because "Go to definition" and "Find all usages" seem logically connected commands. (Maybe "Goto Sprite" may be considered to be of same group?)

3. I noticed that if there's nothing useful under the cursor, the "Add to Watch Panel" command is disabled, but the text is truncated to just "Add", which looks weird. Is there's a way to make it "Add to Watch Panel" (without any name in the middle), for consistency with "Go to Definition"?

4. I just got an idea that it would be cool if we could drag & drop a word from a script to watch panel. Maybe later.

5. If I enable "Autowatch local variables" while at a breakpoint, the variables are not added to the list until I make a single step.

[ericoporto]

OK, I managed to implement 1, 2, 3, 4 and 5!

This PR ended up being 11 different commits, should I squash everything or better leave them as they are? The way it is I think at least documents a little why each piece of the code was added, at least per features/issues.

"Add to Watch" menu command is a very nice addition, but it has couple of bugs:

• It is enabled for any symbols not just variables,

This is the only thing I don't plan to implement, I am leaving this one for the future. I have no idea how to properly do this.