Skip to content

Conversation

@williamsyang-work
Copy link
Contributor

What it does

Removes the prompt for config submission if the user closes the file. We found it was counter-intuitive UI. This will force the user to use the button commands in the nav bar.

How to test

Close the custom analysis json config file. No prompts should show up!

Follow-ups

Really just continued UI/UX improvements for custom analysis.

Review checklist

  • [x ] As an author, I have thoroughly tested my changes and carefully followed the instructions in this template

@williamsyang-work williamsyang-work force-pushed the remove-on-close-event branch 2 times, most recently from efc74e3 to 045d887 Compare August 13, 2025 20:09
@marcdumais-work
Copy link
Contributor

Looks good generally. Once minor comment to address and I'll approve.

);

if (submit === 'Yes') {
if (this.userClickedSubmit) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The implementation works as described. However, I wonder if we should open a confirmation dialog before closing the config editor so that the user can cancel to not lose the configuration accidentally? The implementation could be similar to when closing a code editor with changes that have not been saved. WDYT @williamsyang-work ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

During the original implementation of the customization UI I tried implementing that and wasn't able to. I think that stopping the closing of a file is one of those things that vscode reserves for itself and doesn't give extension developers. But I would need to verify that.

There are some other bugs we've found that I think should be addressed first. We can scope this in and attempt an implementation later?

Copy link
Contributor

@bhufmann bhufmann Aug 19, 2025

Choose a reason for hiding this comment

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

Ok. Thanks for the explanation. Too bad that the vscode APIs seem to be too restrictive. Still worth investigating a bit more to see if we manage to achieve this. We can scope this in later after your other bugfixes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tested the config editor feature a bit more and noticed a few UX issues (without your PR). For example,

  • when creating a config, the editor opens. The editor content is not saved at that moment. The name in header of the config editor is cursive. When I open a file from the file explorer, the config editor content is replaced and I get the submit configuration toast message is shown. With your PR, the editor for config would just be gone.
  • when creating a config, the editor opens. Now I change the config. The editor content is now saved in a temporary file and the name in the header of the config editor is not cursive indicating a permanent opened editor. When I open a file from the file explorer, the new file opens and the name of the config editor is strikethrough (I guess the temporary file is deleted?) Then the submit configuration toast message is shown. With your PR, the editor for config would just be gone.

Both cases need some UX improvements. Are those cases part of the bugfixes that you're working on?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think these UX issues stem from #334.

@williamsyang-work williamsyang-work force-pushed the remove-on-close-event branch 4 times, most recently from dd9ea97 to 4305b21 Compare August 19, 2025 17:55
This will force the user to use the button commands in the nav bar.

Signed-off-by: Will Yang <william.yang@ericsson.com>
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