Proposed fix for microsoft/vscode-makefile-tools/issues/290 #291

jdmchp · 2022-03-11T21:11:29Z

As mentioned in issue 290, we would like to propose making the local (to the host) options of debugging and running optional. This makes the vscode-makefile-tools extension friendlier to use with other extensions that allow debugging in embedded systems.

ghost · 2022-03-11T21:11:41Z

All CLA requirements met.

…ment does not seem to work. Or I am doing something wrong

jdmchp · 2022-03-11T21:33:17Z

src/tree.ts

@@ -103,6 +103,12 @@ export class LaunchTargetNode extends BaseNode {
            item.collapsibleState = vscode.TreeItemCollapsibleState.None;
            item.tooltip = localize("launch.target.currently.selected.for.debug.run.in.terminal",
                                    "The launch target currently selected for debug and run in terminal.\n{0}", this._toolTip);
+            // enablement in makefile.outline.setLaunchConfiguration is not


Trying set this TreeItem to be disabled via enablement in the command: makefile.outline.setLaunchConfiguration

src/tree.ts

andreeis · 2022-03-25T19:42:34Z

src/configuration.ts

+                extension.getState().configureDirty = true;
+                updatedSettingsSubkeys.push(subKey);
+            }
+
            // Final updates in some constructs that depend on more than one of the above settings.


I may be wrong, but I think you don't need configureDirty to be true here. Toggling between view or hide the UI elements doesn't need the extension to go through the reconfigure phase. In case you encountered issues which inspired you to write this (as opposed to looking at other similar code above which may have other reasons) let me know. It doesn't hurt but sometimes, for large projects, configure may be very long and we don't want to let it happen without being necessary.

Same question for calling readCurrentLaunchConfiguration. This doesn't take long, even one less reason to insist on removing it, but do you remember whether you needed that call?

as opposed to looking at other similar code above which may have other reasons)

Yes, basically I followed the pattern without understanding the implications. I will update the PR using your input.
Thanks a lot for the feedback.

src/tree.ts

andreeis · 2022-03-25T20:04:35Z

@jdmchp, thank you for taking the time to implement this. I reviewed and left a few comments (to which you responded already). But the most significant suggestion I have is about the new setting. Having the experience of another similar extension based on cmake build technologies, which is a few years older, we ended up needing to show/hide more things, not just the debug UI elements. Instead of adding a new top level setting for each UI element that we may need to hide/show in future, let's follow that pattern of having all the visibility settings encapsulated in one object top level setting, to which we can add in time based on necessities.

The GitHub repository for the CMake Tools VSCode extension is here: https://github.com/microsoft/vscode-cmake-tools. You can follow in package.json and in the typescript sources how settings like "cmake.touchbar.visibility" or "cmake.statusbar.visibility" are defined and used. We don't have a "status bar" concept in the Makefile Tools extension and no "touch bar" special support yet so I imagine you can name the settings collection like "makefile.panel.visibility", with one entry for launch and in future, when somebody will need it, another entry to build (cmake tools extension needed this for debug and build, to hide/show them based on user scenarios that I don't know very well).

Later edit: actually, 3 settings: one for debug, one for run and one for build. Currently you have debug/run together into one setting.

jdmchp · 2022-03-25T20:15:14Z

@andreeis ,

let's follow that pattern of having all the visibility settings encapsulated in one object top level setting, to which we can add in time based on necessities.
3 settings: one for debug, one for run and one for build. Currently you have debug/run together into one setting.

Got it. Let me look at the CMake Tools VSCode extension. The pattern of having a single central place to enable/disable functionality makes more sense.

Thanks for the guidance.

andreeis · 2022-03-29T22:32:46Z

@jdmchp, I forgot to let you know earlier. The CI tests were not hanging because of changes in your PR. I just fixed the problem somewhere in the extension and the fix will get merged in soon.

…al features. Use OptionalFeatures to handle all optional features in a single place. If optional feature is controlled via "enablement", then the featute can be controlled via package.json changes and one change to the optionalFeatures object. Avoid ѕetting the dirty flag for the configuration when a change of properties is detected. Minimize operations when changing only flags that control run/debug local features

jdmchp · 2022-03-31T21:14:10Z

Hello @andreeis ,
I updated this PR so that multiple optional features can be enabled/disabled by the makefile.panel.visibility property. This property now has an array of optional features that can be toggled. Also, I removed any unnecessary operations (like setting the state's dirty flag). I added two features that can be toggled independently of each other "local debugging" and "local running". I did not make the build feature optional, since to me, the extension's whole point of being is to build. But that could be easily done. If a new feature option is needed to be enabled/disabled and enablement is used to control it, then a change to package.json and a change to this object will be all that is needed.

Thanks for you feedback.

bobbrow · 2022-03-31T22:12:07Z

package.json

@@ -502,6 +504,27 @@
                    "default": null,
                    "description": "%makefile-tools.configuration.makefile.compileCommandsPath.description%",
                    "scope": "resource"
+                },
+                "makefile.panel.visibility": {
+                    "type": "array",


It wasn't immediately obvious to me why this would be an array of objects instead of just an object. Do we need to support an array of objects for this, or will a single object suffice?

Hello @bobbrow , thanks for the quick reply. My goal is to have a list of features that can be toggled independently of each other. Being a list, I thought of using an array. The goal being able to add optional (toggle-able features) easily. The original PR had a single optional feature (build/debug locally).

An object would be a better representation of what you want. If you do an array of objects, then this would be valid syntax in the settings:

{ "makefile.panel.visibility": [ { "enableLocalDebugging": false }, { "enableLocalDebugging": true } ] }

It would be better to just do an object:

{ "makefile.panel.visibility": { "enableLocalDebugging": false } }

The use of an array made iterating through the possible different 'option flags' easy. I can refactor to make the makefile.panel.visibility and object and then iterate through its properties instead.
Thanks for the input.

I updated the PR. Using an object makes the code more readable. So, thanks for that suggestion.

I am new to typescript/json. Things flow differently when using java/xml (what I am used to).

…makefile.panel.visibiluty to makefile.optionalFeatures

bobbrow · 2022-04-01T22:10:15Z

package.json

@@ -502,6 +504,22 @@
                    "default": null,
                    "description": "%makefile-tools.configuration.makefile.compileCommandsPath.description%",
                    "scope": "resource"
+                },
+                "makefile.optionalFeatures": {


I think the old name makefile.panel.visibility was better. "Optional features" is a bit too vague. And if we copy the pattern from CMake Tools, the individual settings can have simpler names like "debug" and "run" because the context is already understood.

I see your point. Let me put that back. Do you think I should have a type in configurations.ts that matches the makefile.panel.visibility exactly:

// as described in makefile.panel.visibility type MakefilePanelVisibility = { enableLocalDebugging: boolean, enableLocalRunning: boolean };

I am not sure what the conventions are. But having an easily relatable name for the type that represents the object that is encapsulated by makefile.panel.visibility makes sense to me.

OK. Changed the property back to makefile.panel.visibility and adjusted types to be relatable to that name.

I'll let @andreeis comment on that. She knows the code better than I do. I tend to chime in more on the stuff that our users would notice like settings, commands, and such.

OK. Thanks.

@andreeis , if there is any more work needed on this PR, please let me know. Thanks in advance for your input.

@jdmchp, thank you for continuing to work on this. I was out of office, I am going to review and test your updates today.

…ug related to restoring defaults for optional features.

andreeis · 2022-04-21T21:53:36Z

@jdmchp, I am pushing a commit with the following changes I did:

fixed some linter complaints in the new code (anything related with ; or spacing or empty lines, return types, etc...)
renamed enableLocalDebugging to debug and enableLocalRunning to run (since name of the parent setting "makefile.panel.visibility" sets the context of these settings that now have smaller name)
minor refactoring in tree.ts, since you added updateTree I just called it in other places where we were doing what updateTree does now
fixed a bug in src/configuration.ts (updateOptionalFeaturesWithWorkspace) where I found a bug while testing, which I explain in the comment (we were not restoring defaults properly in some case)
fixed a bug in the settings update mechanism (src/configuration.ts - initFromStateAndSettings - onDidChangeConfiguration). You forgot to push a new subKey for "makefile.panel.visibility" and we were not tracking the telemetry about settings changes, like for the others.

Additionally, the telemetry was broken for variables like "makefile.name1.name2.name3" etc... and I have PR #309 to fix that. It is separate from your PR.

jdmchp · 2022-04-21T22:18:09Z

@andreeis
Thanks for the clean up and the fixes for the 2 bugs (when makefile.panel.visibility is not completely defined, and not pushing the telemetry key). I updated my fork locally and tested too. Looks good.

andreeis · 2022-04-28T23:43:30Z

@jdmchp, thank you again for implementing this support. It is included in Makefile Tools 0.5.0 which was released today.

Proposed fix for microsoft/issues/290

aeca119

Adding enablement for makefile.outline.setLaunchConfiguration. Enable…

e8b412d

…ment does not seem to work. Or I am doing something wrong

jdmchp commented Mar 11, 2022

View reviewed changes

src/tree.ts Outdated Show resolved Hide resolved

andreeis reviewed Mar 25, 2022

View reviewed changes

src/tree.ts Outdated Show resolved Hide resolved

bobbrow reviewed Mar 31, 2022

View reviewed changes

Use an object (not an array) to represent optional features. Renamed …

94d47e8

…makefile.panel.visibiluty to makefile.optionalFeatures

bobbrow reviewed Apr 1, 2022

View reviewed changes

Went back to makefile.panel.visibility. Adjusted types to match the name

390d10a

andreeis mentioned this pull request Apr 21, 2022

Fix telemetry bug related to object settings #309

Merged

Fix linter issues, one rename, bug in settings change telemetry and b…

db768a4

…ug related to restoring defaults for optional features.

andreeis approved these changes Apr 25, 2022

View reviewed changes

andreeis merged commit eef3c42 into microsoft:main Apr 25, 2022

andreeis mentioned this pull request Apr 13, 2023

Variable expansion - initial support #457

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposed fix for microsoft/vscode-makefile-tools/issues/290 #291

Proposed fix for microsoft/vscode-makefile-tools/issues/290 #291

jdmchp commented Mar 11, 2022

ghost commented Mar 11, 2022 •

edited by ghost

Loading

jdmchp Mar 11, 2022

andreeis Mar 25, 2022

jdmchp Mar 25, 2022

andreeis commented Mar 25, 2022 •

edited

Loading

jdmchp commented Mar 25, 2022

andreeis commented Mar 29, 2022

jdmchp commented Mar 31, 2022

bobbrow Mar 31, 2022

jdmchp Mar 31, 2022 •

edited

Loading

bobbrow Apr 1, 2022

jdmchp Apr 1, 2022

jdmchp Apr 1, 2022

bobbrow Apr 1, 2022

jdmchp Apr 1, 2022

jdmchp Apr 1, 2022

bobbrow Apr 1, 2022

jdmchp Apr 1, 2022

jdmchp Apr 8, 2022

andreeis Apr 11, 2022

andreeis commented Apr 21, 2022

jdmchp commented Apr 21, 2022

andreeis commented Apr 28, 2022

Proposed fix for microsoft/vscode-makefile-tools/issues/290 #291

Proposed fix for microsoft/vscode-makefile-tools/issues/290 #291

Conversation

jdmchp commented Mar 11, 2022

ghost commented Mar 11, 2022 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreeis commented Mar 25, 2022 • edited Loading

jdmchp commented Mar 25, 2022

andreeis commented Mar 29, 2022

jdmchp commented Mar 31, 2022

Choose a reason for hiding this comment

jdmchp Mar 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreeis commented Apr 21, 2022

jdmchp commented Apr 21, 2022

andreeis commented Apr 28, 2022

ghost commented Mar 11, 2022 •

edited by ghost

Loading

andreeis commented Mar 25, 2022 •

edited

Loading

jdmchp Mar 31, 2022 •

edited

Loading