purebasic icon indicating copy to clipboard operation
purebasic copied to clipboard

Bugs related to "Save As"

Open kenmo-pb opened this issue 3 years ago • 17 comments

Here are some related bugs concerning the Save As feature. I am willing to submit fixes, if we agree on the proper desired behavior!

Bug 1 - Any Two Files

  1. Open two source files, for example A.pb and B.pb
  2. From file A.pb, do a Save As and agree to overwrite B.pb
  3. You now have two open tabs for the same file (this should not be allowed!)
  4. Furthermore, if you have Monitor open files for changes on disk unchecked, you can have two different contents for the same file!

Bug 2 - Project

  1. Open a source, A.pb
  2. Create a project and just add A.pb to it
  3. Do a Save As, for example to C.pb
  4. The file tab renames to C.pb and still looks and behaves like it's part of the project
  5. But only A.pb is still associated with the project, per the Project Files panel and the saved XML

Bug 3 - Combine 1 and 2!

  1. Open two sources A.pb and B.pb
  2. Create a project and just add A.pb to it
  3. Do a Save As over the other file B.pb
  4. It combines Bugs 1 & 2: You have two B.pb tabs, one appears to be part of the project, the other doesn't
  5. Switch to the tab which does not appear to be part of the project
  6. Press the menu item or shortcut Add File to Project several times (the tab won't change appearance)
  7. Close the project and look at the saved XML - B.pb is now written several duplicate times to the project file!

Bug 1 Solutions

  • Disallow Save As over a file open in the IDE
  • Or, allow it but close out the other tab automatically

Bug 2 Solutions

  • When doing Save As on a project file, the project should forget the old name and add the new name
  • Or, when doing Save As on a project file, the project keeps the old name and does not automatically add the new

Bug 3 should be fixed by fixing 1 and 2.

kenmo-pb avatar Mar 10 '21 04:03 kenmo-pb

Bug 1 Solutions

  • Disallow Save As over a file open in the IDE
  • Or, allow it but close out the other tab automatically

Maybe the safest solution is to disallow saving over a file open in the IDE. Consider that the open file being overwritten might also contain unsaved changes, which would then be lost (maybe the user didn't intend this to happen, he/she accidentally selected a file (or the wrong file) from the Save As file dialog). In any case, having two versions of a same file open in the IDE is going to be problematic and shouldn't happen as you said.

Bug 2 Solutions

  • When doing Save As on a project file, the project should forget the old name and add the new name
  • Or, when doing Save As on a project file, the project keeps the old name and does not automatically add the new

That's tricky. My bet is that it might make more sense to put the burden on the user in this case, for there is no implication that a Save As operation is intended as a replacement of the original project file (on the contrary, usually this is done to create an alternative version of the file, for backup, or to carry out tests).

But I do agree that the IDE should be more context aware and track meaningful changes to project files carried out within the IDE itself.

tajmone avatar Mar 10 '21 10:03 tajmone

Thanks @tajmone

For Bug 2, I agree. Save As does not necessarily mean "I'm continuing the old file as the new" so the user should be responsible for updating the associated project files. Then the bugfix is: update the file/tab status to disassociate from the project. Unless you're saving FileA over a file FileB which is also already a project file... then the newly renamed file/tab should stay attached to the project!

Bug 1 I am 50/50 on. The safer solution is disallow renaming over an open file. If we did allow it, the IDE would have to close the other tab. And if that tab is Modified... I guess it should prompt to "Save Changes" before closing? But that feels strange, because you're saving changes right before overwriting them with FileA?

kenmo-pb avatar Mar 10 '21 12:03 kenmo-pb

Bug 1 I am 50/50 on. The safer solution is disallow renaming over an open file. If we did allow it, the IDE would have to close the other tab. And if that tab is Modified... I guess it should prompt to "Save Changes" before closing? But that feels strange, because you're saving changes right before overwriting them with FileA?

I'm just worried that allowing the overwrite within the IDE might lead to a complex chain of repercussion which are hard to track. You'd also have to update the file history accordingly, make sure that UNDO/REDO operations in Scintilla would still work (what happens if I Undo the last operation on the overwritten file? Is the history buffer going to handle it correctly?), and many other subtle aspects that might be better handled by closing the old file and opening the new (whether manually or programmatically).

Sometimes "more is less", especially if the simpler solution wards off potential complications which might be hard to test and catch.

tajmone avatar Mar 10 '21 12:03 tajmone

Bug 1: I think nothing should happen automatically here to avoid the user losing any changes by accident. We should inform the user, by either:

  • Not allowing the operation and showing a MessageRequester with an error
  • Asking the user via MessageRequester whether the original should be closed and overriden or not (indicating that any changes would be lost). If the user chooses "Yes" then I think we should close the original without prompting to save changes.

I would tend towards the second option: If we have to show a Requester anyway, why not give the user the choice?

Bug 2: "Save As" should not do anything to the original file, so A.pb should remain in the project and the new C.pb should not be part of it. So the solution here is IMHO to fix the fact that C.pb looks like it is part of the project and show it as a new, independent file (which can then be added back to the project if desired).

t-harter avatar Mar 11 '21 21:03 t-harter

If we have to show a Requester anyway, why not give the user the choice?

Good point. What should the message string be?

Also, bug update: It doesn't have to be two files on disk. I just confirmed you can keep creating New tabs and saving to the same file name! For example, create 3 sources and save them all as A.pb.

A.pb should remain in the project and the new C.pb should not be part of it

Agreed. Except we must also handle the case that C.pb is currently another project file in which case the tab should remain part of the project! It gets slightly complicated :thinking:

kenmo-pb avatar Mar 12 '21 02:03 kenmo-pb

It gets slightly complicated

Also, make sure that differently-cased same-named files are handled properly under Windows (e.g. a.pb and A.pb) and the OSs which allow different-cased filenames (Linux and macOS). I've noticed that many cross-platform editors tend to consider differently cased files as independent files under Windows, and fail the checks for same-file handling. I'm not sure whether this could affect PB, since it depend on the way each editor interfaces to the native file system, but it's worth keeping an eye on it.

E.g. in Sublime Text there was a bug (for ages) where if you renamed a file to uppercase and open it in the editor while it was already open previously to the renaming, the editor will consider it a different file and keep both version open at once (each with its own file buffer an undo history) even though they'd be saving/flushing to the same file on disk (i.e. they had separate handles).

tajmone avatar Mar 12 '21 11:03 tajmone

Windows 10 Allows Case-Sensitive Filenames

It might also be worth mentioning here that with Windows 10 it's now possible to set a whole directory to handle filenames case-sensitively, in order to support Linux projects via WSL or Git.

So, besides checking that "Save As" is not saving just to a differently-cased same file (with default filename system), it would be good to check that under Win 10 the current and destination directories are not case-sensitive.

Whereas for the default case-insensitive behavior it's enough to compare filenames case-insensitively, when case-sensitivity is enabled it won't work. If my understanding is correct, Windows 10 introduced new API functions to handle case-sensitive directories, which might not be exposed to PB's native libraries (since they link to and old CRT).

Bear in mind that it's not infrequent that Win 10 users working with Git simply create a dedicated folder for their repositories and set it to case-sensitive file naming, so they don't have to worry about Linux repositories having file names that clash with Win filesystem.

This might affect not only file saving from the IDE, but also handling file changes that occurred outside of it (i.e. the user manually renaming files in File Explorer, or Git pull operations introducing file names changes).

tajmone avatar Mar 12 '21 11:03 tajmone

@tajmone Yes, filename case should be handled correctly per OS. The IDE code already has some functions to simplify this.

However I forgot about Windows 10 support for case-sensitive, AND it can be per-directory? So it's not as simple as checking for #CompileWindows or OSVersion().

Similar to PR #146 I wonder if we should implement the simple version first (assume Windows = insensitive) and we can tackle Windows 10 later on top of that.

kenmo-pb avatar Mar 12 '21 12:03 kenmo-pb

Similar to PR #146 I wonder if we should implement the simple version first (assume Windows = insensitive) and we can tackle Windows 10 later on top of that.

I guess with have no choice if the native PB filesystem library doesn't support this.

I'm not even sure this can be done from within PB, for it might rely on functionality exposed by newer VSCode versions. Doing it via manual checks might proof harder (and less reliable) than by querying the system. But it's worth to begin looking into it ahead of time, e.g. by creating a dedicated thread in Discussions, where to paste links to MS Docs, etc., and carry out some local testing.

I'm just afraid that might turn out to be one of those "postponed problems" that will eventually lead to a point where it will affect PB applications in general, not just the IDE. It's clear that in the past years MS has made a big change in its policies toward the Linux world and interoperability with other OSs — from the inclusion of a real Linux kernel in Win 10, to spreading .NET, PowerShell, and other of its technologies across other OSs too. I wouldn't be surprised at all if in the near future Windows will switch to case sensitive file naming as the default, and make case insensitivity a fallback option; especially since MS has acquired GitHub and is investing hugely in Git and cross platform projects in general.

Clearly, Win 10 is burning bridges behind, in terms of ruthlessly introducing new features which will make modern tools unusable on older Windows versions (which is why MS pushed so hard for the Win 10 free update "for all", even extending its offer with a grace period for those who hadn't done so).

tajmone avatar Mar 12 '21 12:03 tajmone

Even without windows 10. What about an FTP access mapped on a network drive ? What about extensions allowing to load Linux file systems on windows ? And for a samba server access from Linux ?

Naheulf avatar Mar 12 '21 18:03 Naheulf

Regarding the Windows 10 thing: You are way overthinking this. That is more an obscure feature for power-users. How many regular users do you actually think will know this feature exists and know how to turn it on via the commandline? And even if someone does, it still only becomes an issue if they then decide to create two PB sources in the same directory that differ only in case (which is not a good idea no matter what the filesystem says about it). I don't think it is worth complicating things or abandoning backward compatibility just to handle such an extreme edge case.

Regarding "Save As":

Agreed. Except we must also handle the case that C.pb is currently another project file in which case the tab should remain part of the project! It gets slightly complicated

This is going in the same direction IMHO: Using the "Save As" functionality to save a source code over an existing other source code is not a regular use case. Even less so if that other source code is currently open in the IDE. If that happens it is most likely a mistake more than anything else. So I don't think we have to provide a solution that works correctly in all corner cases of that edge case. How much time do you want to spend on functionality that no sane person would ever use on purpose? :-)

I am starting to think that maybe just forbidding to overwrite a file using "Save As" that is currently open or part of an open project is the simple way to deal with this without getting lost in the details. If somebody really wants to do this they can still close the file/project themselves and then save over it.

t-harter avatar Mar 12 '21 20:03 t-harter

Regarding the Windows 10 thing:

Yes, I think the minimum we should do here is handle case-insensitive file checking on Windows. It's already done in other parts of the IDE code, should be no problem here. That's how Windows users have expected programs to work for decades.

Support for Win 10 case-sensitive folders, FTP-mapped folders, WSL subsystems... those are far beyond the scope of this Bugfix. I welcome other people to tackle it. But for a small team of volunteer contributors, you really have to consider your time and effort vs the real-world use cases. (This Save As bug report is already pretty obscure!)

Regarding "Save As":

I will take a swing at this and submit a PR to try. Maybe I'm underestimating, but I don't think the logic is too complicated:

  • User selects new filename...
  • Before overwriting: Check if file is open in IDE. If no, just overwrite. If yes, prompt user to confirm or cancel. (Confirm will close the other existing tab.)
  • After overwriting, if a project is open: Check if the new filename is linked to the project. If yes, ensure tab is colored and file is linked correctly. If no, ensure tab is uncolored and unlinked correctly. (It might already be in the correct state.)

kenmo-pb avatar Mar 13 '21 05:03 kenmo-pb

Regarding the Windows 10 thing:

@t-harter:

You are way overthinking this. That is more an obscure feature for power-users.

We're speaking about an IDE here, targeting programmers. Even the most beginner of programmers is beyond the best power-user, because they fall in the developers category.

How many regular users do you actually think will know this feature exists and know how to turn it on via the commandline?

Any Windows user who is a programmer and subscribes to developers' newsletters, read PC magazines, participates to programming forums, has installed the latest MSVS or uses Git for Windows, WSL/WSL2.

In any case, it's not just about end users enabling this feature manually, themselves; some Win 10 applications could enable it on some directories at installation time, e.g. because their dependencies include some Linux modules. In fact, when you enable WSL you're automatically enabling this feature on your machine, and third party apps and VMs images might be enabling it in the background on various directories. If I remember correctly, MSYS2 also leverages this features.

it still only becomes an issue if they then decide to create two PB sources in the same directory that differ only in case (which is not a good idea no matter what the filesystem says about it).

Not at all, in cross platform projects like this the most common risk is having the code try to open a file using wrong letter casing, which will pass all tests on Windows and fail on other OSs with a "file not found" error — which, if I remember correctly, has already happened here during the fixes of typos in filenames.

It's really about best practices: if the project is cross-platform, Windows users should enforce case sensitive file names to maximize compatibility with other OSs and prevent case-insensitive file mismatches to go by unnoticed. So I would expect that many Git users would enable this feature for cross platform repositories.

I don't think it is worth complicating things or abandoning backward compatibility just to handle such an extreme edge case.

This fix shouldn't affect backward compatibility at all, since it's about being aware of the new directory flags for case sensitivity, and whether a particular Win machine supports them. On the contrary, ignoring the problem might result in backward incompatibility if MS in the future will switch to case sensitivity as the new default.

As I mentioned, Sublime Text suffered from a series of bugs similar to those of this PR (Save As, renamed files with different cases, etc.) and it took actually a long time to solve them, and when they did a new series of problem popped up in relation to the new case-sensitive folders functionality — which shouldn't really surprise that so many users was taking advantage of it, since ST users are often also Sublime Merge users, which is a Git GUI front-end by the same devs.

What I'm trying to say is that before trying to fix all the edge cases described in this PR, it's worth considering also the Win 10 case-sensitive feature and keep it in mind, to prevent having to unravel the fixes in the future just to fix new problems — BTW, I'm also suggesting to take the easiest path in fixing the problems, to avoid entanglement.

The point is that I simply don't know whether this new CS feature is something hard to exploit under Windows. It could be exposed by some new DLL only available via the latest VS runtimes, or it could be that the classical WinAPI interfaces were updated by adding an extra flag and error type to handle case sensitive folder.

Regarding "Save As":

@t-harter:

I am starting to think that maybe just forbidding to overwrite a file using "Save As" that is currently open or part of an open project is the simple way to deal with this without getting lost in the details. If somebody really wants to do this they can still close the file/project themselves and then save over it.

I also agree on this, "less is more" in this case, and trying to cover correctly all the "Save As" possible cases for all OSs might lead to code entanglement and unexpected problems.

tajmone avatar Mar 13 '21 09:03 tajmone

It's really about best practices: if the project is cross-platform, Windows users should enforce case sensitive file names to maximize compatibility with other OSs and prevent case-insensitive file mismatches to go by unnoticed.

Should we also ask @fantaisie-software to add a compiler warning:

  • on case sensitive file system if the the included file path can also match another path in case insensible mode.
  • on case insensible file system if the included file path does not match the include directive in case sensitive mode.

Naheulf avatar Mar 13 '21 11:03 Naheulf

Should we also ask @fantaisie-software to add a compiler warning:

If Fred was able to add the proposed compiler warnings it would mean that he's able to access this feature via the OS, so he would also be able to update the native PB file-system commands accordingly.

Maybe we need to first grasp if and how this feature is indeed accessible from PB at all, or if it depends on a different runtime, or other components not accessible to PB. The truth is that this is a fairly new (and optional) feature, so IMO the best thing we should do is open a dedicated threat in Discussions, and start to dig more into it (how to exploit it, its pros and cons, etc.).

In the meantime, any IDE changes relating to filenames changes should keep into consideration the existence of this feature (i.e. avoid writing lot's of hand-coded Win-specific workaround assuming all folders are case insensitive, since this might no longer be the case). Once the feature is better understood, and its potential problems identified, we could add a Wiki page describing it in more detail, and providing examples of the various DOs and DON'Ts when dealing file files under Windows.

We should start doing some local tests, to see how file operations cope with case-sensitive directories in Win 10 (e.g. if they catch errors correctly, how they are reported, etc.), and search what the MS documentation says about this new feature, it's interfaces, etc. From there, we might get a clear picture of how enabling this feature can affect PB apps. But surely this Issue is not the place to expand further on these details, which belong to Discussions, and we ought to have dedicated thread for the topic.

Right now, it's not an issue that should determine all coding choices and actions, but it's one that it's worth keeping in mind nonetheless, and worth mentioning when we see Issues and PRs that might involve it.

tajmone avatar Mar 13 '21 11:03 tajmone

If Fred was able to add the proposed compiler warnings it would mean that he's able to access this feature via the OS, so he would also be able to update the native PB file-system commands accordingly.

He can also scan folder content to see if the current name can create issue or not. And this does not need to find if this feature is enabled and how to use it.

Naheulf avatar Mar 13 '21 12:03 Naheulf

I think this discussion should split off into its own issue. Yes, it relates to "Save As" but really the IDE in general.

This OS-agnostic bug in my original post can be fixed first without tackling new Windows 10 specific logic. (Also I personally don't have Windows 10 at home, so I cannot write and test that!)

PS. I wonder, how much is "automatically" handled by the OS? When you call a PB function like FileSize(), in the end it compiles down to OS calls, and I wonder if those OS functions internally handle case-sensitive directories?

kenmo-pb avatar Mar 13 '21 21:03 kenmo-pb