ionide / ionide-vscode-fsharp

VS Code plugin for F# development
http://ionide.io
MIT License
865 stars 280 forks source link

Adding the same file twice to a project will add the compile tag 2 times #1771

Closed MangelMaxime closed 2 years ago

MangelMaxime commented 2 years ago

Describe the bug

Steps to reproduce

  1. Use the solution explorer to "Add file" > new.fs
  2. Use the solution explorer to "Add file" > new.fs
  3. See that the fsproj has 2 lines <Compile Include="new.fs" />

Expected behaviour

The <Compile Include="new.fs" /> should only be added if the file not already present.

Screenshots

https://user-images.githubusercontent.com/4760796/185758975-c20072f2-1581-472c-8a1d-61daf2b273af.mp4

Machine info

Additional context

Add any other context about the problem here.

MangelMaxime commented 2 years ago

Would this behaviour be ok:

https://user-images.githubusercontent.com/4760796/185793117-bffc3e2a-2617-4af0-a9ad-37a633e793d7.mp4

If the file already exist in the fsproj an error is reported saying "File already exist".

baronfel commented 2 years ago

Could you try adding validation in the text box where the user can put the file name? I think it would be easier to just stop the bad behavior before the user can even submit the bad value.

MangelMaxime commented 2 years ago

This is indeed possible to do, however when loading the prompt even if the file new.fs exist and we return the error VSCode doesn't show it in the textbox. This is probably a bug in VSCode when they are loading the textbox.

image

I see 2 possibilities:

  1. We don't put new.fs in the textbox just keep it as a placeholder because user will most probably not use new.fs as the file name
  2. Don't care about this small edge case and perhaps it will be fixed in the future in a VSCode release.

Demo:

https://user-images.githubusercontent.com/4760796/185796091-2261a02e-c2dd-4f19-8da7-53c58f8ec753.mp4

Krzysztof-Cieslak commented 2 years ago

Yeah, I don't think we need to worry about new.fs case, I guess most people won't name their files like that

MangelMaxime commented 2 years ago

@baronfel @Krzysztof-Cieslak

I made a PR preventing the user from submitting the input if the file already exist in the project.

This indeed provide a better UX experience.

However, because FsAutoComplete is not tied specifically to Ionide I think we should still handle the case on FsAutoComplete too. For example, if people using Sublime Text or Emacs implements the fsproj/addFile commands they would still have the correct behaviour without duplication. And if they want to provide nice UX from the client they can implement it like done with Ionide.

What do you think?

baronfel commented 2 years ago

Yeah, that's a completely reasonable thing. Let's do it.

MangelMaxime commented 2 years ago

I created an issue to keep track of it.

I should work on it this weekend or next week.