Skip to content
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

WIP: run all tests button and context menus #191

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lukester1975
Copy link
Contributor

First commit is the run/run all buttons in the meson view.

Second commit adds context menus all over the shop. Makes more sense on my old multiple builddir branch (e.g. I had hacked a very unpleasant debug command, but given the debug provider stuff, doesn't seem worth it unless that can be hooked in to; haven't looked), but maybe worth adding some now.

Personally, I don't like that clicking on a target builds it. Could add an inline button next to the open meson.build and just leave clicking on the node to expand it.

Also moves the reconfigure button (if/when multiple root/builddirs are added, it doesn't make sense to have a reconfigure button in the sidebar's header).

Build icon came from the cmake plugin. Whether that would need attribution somewhere I've no idea...

Cheers

Copy link
Contributor

@tristan957 tristan957 left a comment

Choose a reason for hiding this comment

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

Mind adding a picture of what this looks like?

package.json Show resolved Hide resolved
return item;
}

override getChildren() {
return this.tests.map((test) => new TestNode(this.id, test, this.isBenchmark));
}

run() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a return type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do, though it's from the IRunnable interface; not sure what the gain is other than more stuff to edit if the interface changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

To me, the gain is explicit return types. Helps when reading code. Everyone has different preferences though.

@lukester1975
Copy link
Contributor Author

run
build

I put back the header configure button as if there's no build dir, there would be no configure button... Was different with the multiple build dir support as there was a project-level node, with build dirs beneath it.

@tristan957
Copy link
Contributor

This looks solid to me. Are you ready to go?

@lukester1975
Copy link
Contributor Author

Two things:

  1. Personally, I don't like that clicking on a target builds it. Could add an inline button next to the open meson.build and just leave clicking on the node to expand it.

Feels like nowhere else in the ui does clicking on a non-leaf node not expand it. But maybe there's precedent elsewhere...

  1. build icon attribution. I couldn't see anything in the cmake repo.

Cheers

@tristan957
Copy link
Contributor

tristan957 commented Nov 23, 2023

I agree with you on 1. If you want to add a build button, I think that would be a great addition!

Did you get the icons from the CMake or CMake Tools extension?

@lukester1975
Copy link
Contributor Author

  1. OK. Might get to it later.

It was cmake tools.

…project view.

Build icons from vscode-cmake-tools.
Added an inline build button on a target node.
@lukester1975
Copy link
Contributor Author

OK added a commit with that change.

I need to take a look at the icon though. It's ugly when hovering over a selected target node (OK with dark theme):

ugly

@lukester1975
Copy link
Contributor Author

BTW, icon is ugly in cmake plugin too.

Dark mode is fine.

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