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

Kiota should show the "override" message only when there are changes not saved #4853

Closed
maisarissi opened this issue Jun 18, 2024 · 6 comments · Fixed by #4899
Closed

Kiota should show the "override" message only when there are changes not saved #4853

maisarissi opened this issue Jun 18, 2024 · 6 comments · Fixed by #4899
Assignees
Labels
enhancement New feature or request type:enhancement Enhancement request targeting an existing experience vscode-extension Work related to the vscode-extension
Milestone

Comments

@maisarissi
Copy link
Contributor

maisarissi commented Jun 18, 2024

After creating or editing and re-generating an existing API Plugin, when clicking on "Add API description", if there is no changes that were not saved, Kiota should not show the following message:
image
and should start the normal flow of adding an API description

The message should be showed when creating a new client/plugin and the generation is not done yet and the the user tries to "Add API description" or when one is editing, changes the selected paths and have not hit re-generate.

@maisarissi maisarissi added type:bug A broken experience status:waiting-for-triage An issue that is yet to be reviewed or assigned vscode-extension Work related to the vscode-extension labels Jun 18, 2024
@baywet
Copy link
Member

baywet commented Jun 18, 2024

@maisarissi do you make a distinction between "no changes have been made" and "changes that have been made are identical to the initial situation" in this scenario or not?
i.e. if I have 3 operations selected, add a new one, remove it. Is it a change or not in this scenario?

@baywet baywet added enhancement New feature or request type:enhancement Enhancement request targeting an existing experience and removed type:bug A broken experience status:waiting-for-triage An issue that is yet to be reviewed or assigned labels Jun 18, 2024
@baywet baywet added this to the Kiota v1.16 milestone Jun 18, 2024
@ElinorW ElinorW self-assigned this Jun 19, 2024
@maisarissi
Copy link
Contributor Author

maisarissi commented Jun 20, 2024

I wouldn't make a distinction, as the final state is identical the the initial state.
"No changes have been made" and "changes that have been made are identical to the initial situation" should not show the override message.
We need to be careful though with one changing some sub-node and make sure we are tracking changes in an operation level.

@baywet
Copy link
Member

baywet commented Jun 20, 2024

So we effectively need to store an initial state somewhere when loading. And diff it against the current state, to ensure whether any "true modification" was made. We should also consider #4870 in the process.
@ElinorW let me know whether it clarifies the design enough or whether you need more details here.

@petrhollayms
Copy link
Contributor

@maisarissi Are we not solving for a border case here? How often will it even happen that the user reaches exactly the same state?
When comparing to the IDE behavior, for example- only Undo brings me typically back to an "unmodified" state. Editing a file e.g. in VSCode and reverting the changes (without using undo) still keeps the file as "modified".

So, if we can simplify the implementation now by just checking the modified state without trying to compare the status with the last saved status (and handling the initial state well, and any errors during saving etc.) - I think this would be a pragmatic approach for the first iteration now, given the timelines. Thoughts?

@maisarissi
Copy link
Contributor Author

@petrhollayms I agree that in some cases, like editing files, the CTRL Z (undo) can bring you to the previous state. Does CTRL Z work in our case as well? AFAIK the only way going back in the extension is manually modifying to go back to the previous state.

My concern with this is that we might end up editing something wrong (by mistake) and decided to go back. The extension will then show you a message saying you will lose your changes. The problem with this scenario is that one might be concerned like "have I edited something I shouldn't?" "what's the difference from what I had before?" "Is this going to make my solution be wrong?" and then add the need from double checking everything just to make sure one is not making an undesired change. This is a bad experience in my opinion.

I do agree that this check with the initial state could be implemented later, after PP, but I would like to hear more from @ElinorW on where we are with this, as the comment from @baywet and @ElinorW in the PR was 3 weeks ago and part of this might be already implemented.

@ElinorW
Copy link
Contributor

ElinorW commented Aug 1, 2024

This is all good, I've implemented it as requested based on the hashing idea that @baywet proposed

@ElinorW ElinorW closed this as completed Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request type:enhancement Enhancement request targeting an existing experience vscode-extension Work related to the vscode-extension
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants