-
Notifications
You must be signed in to change notification settings - Fork 50
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
Refactor support for multiple Meson projects #195
Conversation
CC @tristan957 @wolfpld. Could you please test this PR with your multiple projects case? |
I'll probably take a look Saturday or Sunday. Does the current situation with 1.18.1 work for you? |
I think we have some significant communication issues here. This is the first time you mention having a build dir preconfigured before opening the workspace in VS Code. Only now can I see what your comments about preferring a root-level Nota bene, if we are talking about breaking workflows, the workflow in which you select to configure the workspace, in the very dialog you are refactoring in this PR, has been broken for half a year, since May, and no one noticed. Regarding your question, I don't know how these changes affect me. Because of the communication problems I do not know where the development is going. I asked some questions in the PRs yesterday and I am still waiting for your answer. At this point, I don't know if I want to continue putting time and effort into contributing to this repository, or if I should just fork, do what I need for my use case, and rely on that instead. Ultimately, as @tristan957 said, you "definitely need more contributors to this extension". It's up to you how you want to handle that. |
0d5ba93
to
5144ce4
Compare
@wolfpld sorry if it wasn't clear what was broken, it seemed pretty obvious to me since just opening any project results in the mentioned problems. In any case, this PR fixes tones of things, not all related to multi projects repository, so I think it would be a good improvements anyway. |
I agree.
I wouldn't say that my use cases support these statements, but at this point it doesn't matter. There are things that need to be ironed out in my implementation and your proposed solution fixes most of the pressing issues (albeit not in an ideal way from my point of view).
As mentioned earlier, this was the result of a well-intentioned change (
Would that be to set things such as build type, or such? Maybe this should be exposed in the GUI somehow? |
Some relevant notes about the internal implementation of VS Code mementos: The memento data is stored in The All mementos used by this extension are stored in a row with the key
The memento state can be reset with the following command: |
5144ce4
to
ef51f37
Compare
You left a merge conflict in |
ef51f37
to
a9fc4cd
Compare
It would also be great to get some of this split out into separate commits. Some of the changes seem unrelated to multiple meson projects. I like the refactoring you did with dialogs.ts |
a9fc4cd
to
d73ddfe
Compare
Fixed.
done. |
d73ddfe
to
75735cf
Compare
75735cf
to
c79d406
Compare
I found a problem that was probably always present in the code, but is now better exposed. Assuming you have selected a project root, if the "Meson project detected in this workspace but does not seem to be configured..." dialog appears and you click the "no" button or the "x" close button, the project is not configured and the extension continues to work as expected. For example, you can "select project root directory" and it will work. However, if you close the initial dialog with the Esc key instead, the extension will not respond. No command (e.g. "select root dir" or "reconfigure") will work in this case. Another problem: When the "select project root directory" command is selected (but not when it is displayed in response to the "multiple meson projects detected..." dialog) and you click outside the selection popup, the popup closes and the VS Code window reloads, and the "multiple meson projects detected..." dialog reappears, even if you have already made this selection. |
c79d406
to
e79163e
Compare
I think this is how vscode is expected to work... microsoft/vscode#62853. Pressing escape hide that notification, but it's still running. In our code that means we are still awaiting for the response.
Right, if |
@tristan957 I think we should merge and release with this PR. Any objection? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some nit picks, then let's merge it and tag a release tomorrow.
aef9609
to
0f8cfec
Compare
Running them from command panel would fail because there is no builddir configured.
The very first thing the extension must do is determining the Meson project source directory. If there are multiple workspaces, or if the repository contains multiple projects in subdirs, there could be more than one potential Meson project. In that case ask the user to select one. The build directory can then be determined relative to the selected source directory. This also adds a command to allow switching to a different Meson project root directory. Unfortunately this requires a full window reload for now.
New workspace could have been added, for example.
0f8cfec
to
82aa006
Compare
Rebased and it uses @tristan957's sorting algo. Good to go? |
There were many issues, including: