Skip to content

Ensure modified methods updated after changing a method from the modeling panel#2929

Merged
charisk merged 1 commit intomainfrom
charisk/update-modified-methods
Oct 10, 2023
Merged

Ensure modified methods updated after changing a method from the modeling panel#2929
charisk merged 1 commit intomainfrom
charisk/update-modified-methods

Conversation

@charisk
Copy link
Copy Markdown
Contributor

@charisk charisk commented Oct 9, 2023

Small change to make sure we update the modified methods state when the modeled method gets updated. If we don't do that we can end up in a situation where the state/UI is out of date. For example the following bug exists in the current code:

  1. Open model editor
  2. Click view on some method
  3. Change to source (see it being marked as modeled)
  4. Click on another method from the method usages panel
  5. Change to flow summary

At this point the second method should be marked as modeled, but it isn't.

Checklist

N/A:

  • 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 charisk requested a review from a team as a code owner October 9, 2023 15:35
Copy link
Copy Markdown
Contributor

@starcke starcke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, happy to go with this as a bugfix - but I wonder if we can streamline the modeled and modified method calls?

activeState.databaseItem,
msg.method,
);
this.modelingStore.addModifiedMethod(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we often have to call addModifiedMethod(s) after methods like updateModeledMethod and addModeledMethods. I wonder if we can move the call to update the modified methods inside the store so that the caller does not have to remember to call both functions?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I'd definitely like to move towards that, it's a good idea. In general I can see a few simplifications/improvements we could make around the modeling store. But I think of this as v1, with a lot of features currently in flux, so I'd rather wait a bit before tidying things up. Basically let it evolve a bit and then come back and address some tech debt once we have a fuller picture of its use.

@charisk charisk merged commit e98611f into main Oct 10, 2023
@charisk charisk deleted the charisk/update-modified-methods branch October 10, 2023 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants