Skip to content

Refactor events to fix the method modeling panel getting stuck showing "start modeling"#3439

Merged
robertbrignull merged 7 commits intomainfrom
robertbrignull/modeling-mode
Mar 6, 2024
Merged

Refactor events to fix the method modeling panel getting stuck showing "start modeling"#3439
robertbrignull merged 7 commits intomainfrom
robertbrignull/modeling-mode

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

This PR fixes the method modeling panel getting stuck showing "start modeling" if you open the model editor before you open the QL sidebar panel.

The problem was that when the webview loaded we weren't setting the language field correctly in the webview. There needs to be a call to setViewState() after determining the language.

I recommend reviewing commit by commit.

The actual fix is made in d441342 which is the third commit. The two commits before that are refactoring methods so we can reuse code. For this fix I've not worried about being inefficient with sending webview messages. For example since setDatabaseItem calls setViewState we don't have to call it before in this case, but I felt the complexity of having branches and relying on methods always calling other methods was not worth the minimal cost of calling setViewState twice when loading initial data.

The commits after the fix could be moved to a separate PR if preferable. I noticed another potential bug which was that we had both the setMethod and the setSelectedMethod webview messages which both set the method in the view but setMethod doesn't contain all the other fields. This could mean if we used setMethod we could end up with state like isModified or modeledMethods being outdated and incorrect. Luckily we avoided the bug because every time we did call setMethod (except when setting the method to undefined) we also called setSelectedMethod, so essentially setMethod is unnecessary. Indeed we can remove it and replace it with setSelectedMethod and a new setNoMethodSelected message and also simplify class interfaces by changing where we listen to modeling store events from.

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.

@robertbrignull robertbrignull requested review from a team as code owners March 5, 2024 12:11
Copy link
Copy Markdown
Contributor

@norascheuch norascheuch left a comment

Choose a reason for hiding this comment

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

The code changes look good! I haven't tested it locally though.

@robertbrignull
Copy link
Copy Markdown
Contributor Author

I've been playing around with it trying to test it manually and I haven't found any bugs or anything else wrong.

@robertbrignull robertbrignull merged commit 83a6b5a into main Mar 6, 2024
@robertbrignull robertbrignull deleted the robertbrignull/modeling-mode branch March 6, 2024 12:06
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