-
Notifications
You must be signed in to change notification settings - Fork 335
[VSC-1541] Enhance/hints viewer openocd #1476
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
Conversation
|
Download the artifacts for this pull request: |
brianignacio5
left a comment
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.
Some code suggestions to simplify functionality.
|
Pull request has been marked as |
675b284 to
65efe87
Compare
f3793f8 to
57799e8
Compare
57799e8 to
10c0173
Compare
src/extension.ts
Outdated
| ); | ||
| }; | ||
| // Store display hints notification (until VS Code is closed) | ||
| context.workspaceState.update("idf.showHintsNotification", true); |
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.
Why is this state necessary? I don't see being disabled on extension deactivation. Maybe is not necessary ?
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.
The idea behind it was to have a reset of the hints viewer's notifications for each session. I don't remember why I went with this implementation of the Notification System for Hints Viewer.
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.
I've pushed a new commit in which I converted idf.showHintsNotification from runtime workspace state to proper configurable setting that will persist in workspace settings
brianignacio5
left a comment
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.
LGTM without using the workspaceState setting, which doesn't seem to be configurable.
bcbd569 to
33c0402
Compare
brianignacio5
left a comment
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.
Missing translation and documentation of new setting.
d8d4fe9 to
8f4b8bc
Compare
brianignacio5
left a comment
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.
LGTM
Fabricio-ESP
left a comment
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.
Confirmed working.
Thanks for the change on how the hints are displayed.
brianignacio5
left a comment
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.
Please review existing status bar items implementation and use of registerIDFCommand instead of vscode.commands.registerCommand
997eb9c to
8501057
Compare
brianignacio5
left a comment
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.
LGTM with minor change regarding code organization.
- Refactor code - Add Translations
- fix errors related to errorHints view
- change === to includes - remove checking for openOCD hints, hints from openocd will not be present in text editor
Before this fix, if there were multiple hints matches, there would be multiple notifications
Convert idf.showHintsNotification from runtime workspace state to proper configurable setting
Added validation for checking if any workspace is open
- Add translations for new hints viewer status bar item implementation - Remove translation from old notification system for hints viewer - Clean some unnecessary comments
- Move hints status bar creation to createCmdsStatusBarItems function - Change id from "idfErrorHints" to "espIdf.errorHints" - Use "registerIDFCommand" instead of "vscode.commands.registerCommand"
8501057 to
364a879
Compare
Description
This enhancement:
- Focus on the ESP-IDF hints bottom panel
- Mutes "Found Hint" notification for this VS Code session (until VS Code is closed)
Type of change
Steps to test this pull request
v0.12.0-esp32-20250226). List of error-hints pair is taken from:<ESP_TOOLS_INSTALLATION_PATH>/tools/openocd-esp32/<OPEN_OCD_VERSION>/openocd-esp32/share/openocd/espressif/tools/esp_problems_hints.yml<ESP_IDF_INSTALLATION_PATH>/<ESP_IDF_VERSION>/esp-idf/tools/idf_py_actions/hints.ymlHow has this been tested?
As described above
Test Configuration:
Checklist