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

[CfgEditor] Fill .cfg parameters by default values #1493

Merged
merged 1 commit into from
Feb 12, 2023

Conversation

stamalakhov
Copy link
Contributor

@stamalakhov stamalakhov commented Feb 8, 2023

This commit fills .cfg paramters by default values using keyboard shortcut.

This commit fills default values in .cfg file (input and output paths for import/optimization/quantization).
Currently its optimal usage would be:

  1. create new configuration
  2. check needful steps
  3. press 'ctrl+shift +/' for filling .cfg parameters to default values. ('ctrl+d' is busy)

Issue: #1487
ONE-vscode-DCO-1.0-Signed-off-by: s.malakhov [email protected]

@stamalakhov stamalakhov force-pushed the set_default_BR branch 2 times, most recently from e209415 to b325949 Compare February 8, 2023 13:45
media/CfgEditor/index.js Outdated Show resolved Hide resolved
@stamalakhov stamalakhov force-pushed the set_default_BR branch 3 times, most recently from b21cee9 to 4797076 Compare February 9, 2023 08:19
seanshpark
seanshpark previously approved these changes Feb 9, 2023
Copy link
Contributor

@seanshpark seanshpark left a comment

Choose a reason for hiding this comment

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

LGTM thank you!

@stamalakhov stamalakhov self-assigned this Feb 9, 2023
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Comment on lines 287 to 296
if (this._activeWebviewPanel === webviewPanel) {
// there are race conditions with visibility
// another webviewPanel becomes visible while this one is disposed
// so 'activeCfgIsVisible' will not be set to false
vscode.commands.executeCommand(
"setContext",
"one.CfgEditor:activeCfgIsVisible",
false
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

By using the given context below, you don't need to be bothered to implement this custom context.

"when": "activeCustomEditorId == one.editor.cfg"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh... Ok. Thank you. I'll try!

@@ -101,6 +104,61 @@ function main() {
postMessageToVsCode({ type: "requestDisplayCfg" });
}

function setDefaultValues(name) {
const optimizationUsed = document.getElementById("checkboxOptimize").checked;
Copy link
Contributor

Choose a reason for hiding this comment

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

(Not for functionality, but for readability)

How about collecting related lines?

These optimization-related lines can be moved below, near to the updating lines,

const optimizationUsed = document.getElementById("checkboxOptimize").checked;
let optimizedPath = importedPath + ".opt";

and these quantization-related lines too.

  const qType = document.getElementById("DefaultQuantQuantizedDtype").value;
  const qSuffix = qType === "uint8" ? ".q8" : ".q16";
  let quantizedPath = optimizationUsed
    ? optimizedPath + qSuffix
    : importedPath + qSuffix;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I'll try to make it more clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks clear, thanks!

This commit fills .cfg paramters by default values using keyboard shortcut.

ONE-vscode-DCO-1.0-Signed-off-by: s.malakhov <[email protected]>
Copy link
Contributor

@dayo09 dayo09 left a comment

Choose a reason for hiding this comment

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

Thanks a lot! 😄

Copy link
Contributor

@seanshpark seanshpark left a comment

Choose a reason for hiding this comment

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

LGTM

@seanshpark seanshpark merged commit e35ac73 into Samsung:main Feb 12, 2023
@stamalakhov stamalakhov deleted the set_default_BR branch February 13, 2023 05:17
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.

3 participants