Include in addModeledMethods whether the methods are unsaved or not

[robertbrignull]

This is an attempt to fix the behaviour around showing the "unsaved" label and enabling the save button. Previously it wasn't correctly showing a model as unsaved when generating from the LLM or modelling from source.

The approach in this PR works, but it feels a bit inelegant and brute-forcey. I also don't love that the listener has gained dependencies on a couple of different bits of state, but maybe it's ok as those bits of state don't actually change too often.

Any other suggestions for how to implement this? I tried a few variations of putting names of the affected models in AddModeledMethodsMessage, or putting the library/packageName on ModeledMethod, but both turned out to be pretty hard to do as we just don't have the info available when generating the modelled methods.

Checklist

• CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
• Issues have been created for any UI or other user-facing changes made by this pull request.
• [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

[charisk]

Have you considered splitting the messages for loading vs adding new modeled methods? If I'm reading the code right, we currently have the addModeledMethods message that we use in 3 different places - one of them during the first load and two of them when there are updates.

When handling those messages there are two steps:

• set modeled methods
• set any unsaved models

The last step only happens for updates (not when loading). So if we had two separate messages, only the one around updates would execute the 2nd step. We'd probably want to extract the logic for the first step into a re-usable function.

Would that work? Apologies if this is a dumb suggestion. Happy to talk through it.

BTW I think what you have is fine, but as you say, it's just a little inelegant.

[robertbrignull]

Have you considered splitting the messages for loading vs adding new modeled methods?

It's in intriguing idea. It may be worth doing as a refactor to make things easier to understand, so we aren't using one message type for two purposes. However I don't think it helps with the "inelegance" of needing to loop through all of externalApiUsages as the message for adding new methods would still need this logic.

I think I'll implement this approach in this PR because it will avoid adding a new unsaved: boolean field to the message, so in that way it is strictly cleaner than the current state of this PR.

[robertbrignull]

I've split addModeledMethods up into addModeledMethods and loadModeledMethods. This actually means we can remove overrideNone too because that was only set to true in the addModeledMethods case. I think it works out quite well.

[charisk]

Thanks for making the changes, I think that looks great!