#646 Problems with handling of read-only files in TCoolEdit

[Ognyan Chernokozhev]

TCoolEdit can open read-only text files and by default does not allow editing and saving them.

But the read-only flag can be turned off by the method EnableReadOnlyMode(false), which is available in the Edit->Modes->Read-Only Mode.

If after that the user tries to save the document, the application crashes.

This case should be handled gracefully, by either showing an error message to the user or by clearing the read-only flag of the file and then saving the changes. I think the first option is preferable, as the file may be opened from a read-only location.

Another use case is when the user opens a read-only file and then uses Save As to save it under a different name - the document stays read-only and the user has to manually turn off the read-only flag before editing and saving the changes. The read-only flag should be automatically cleared in this case.

Lastly, it would be nice to have some visual indication when the file that is opened is read-only. For example Notepad++ grays the save icon to the left of the file name, which Visual Studio shows a small lock icon to the right of the file name. Even a simple text "(read-only)" in the title bar may be good to have.

[Vidar Hasfjord]

Good ticket!

See also "TCoolEdit: Display ReadOnly in the document title" [feature-requests:#195].

[Ognyan Chernokozhev]

Possible fixes for the first two issues pushed in [r8820].

[Erwin Lotter]

Another use case is when the user opens a read-only file and then uses Save As to save it .... . The read-only flag should be automatically cleared in this case.

This was easy to fix in "branches/elotter-cooledit" ([r8825]).

However, I think a clean solution should distinguish between a user option and a state flag.
That would mean to introduce a separate read-only state and not to change a user setting.

But I wonder if it wouldn't be simpler not to set the read-only flag when opening a read-only file, but instead to suggest using "Save As" when an attempt is made to save the file.

[Vidar Hasfjord]

@jogybl committed:

CHG: CoolEdit window title now displays a (read-only) or a * dirty indicator [r8943].

This resolves the third and final issue.

But note that OWLMaker will still replace a read-only file, e.g. if you open a read-only file, turn off read-only mode, change the text and save. Then OWLMaker will make a backup by renaming the old read-only file (prepending a tilde to the extension) and then write a new file with the original name, no longer read-only and with the changed content.

I guess a simple fix is to redirect Save to SaveAs when the file is read-only.

[Ognyan Chernokozhev]

Good point. I thinkit will be best not to rely on the flag in the app, but when saving, to first check if the file that is being saved to is read-only and if so, to redirect. This should handle cases like when the file was not read-only initially, but has become read-only in the meantime.

[Vidar Hasfjord]

@jogybl committed:

CHG: When TCoolEdit saves a file, if the file on disk is read-only, perform a SaveAs instead [r8944].

This seems to work well in my tests with RichEditor and OWLMaker.

However, I don't like having UI code (SaveAs) in TSource.

Note that TDocument::CanClose calls TDocManager::FlushDoc to handle the UI:

return DocManager->FlushDoc(*this);  // do the UI in the doc manager

So a better solution seems to be to extend FlushDoc to handle the read-only case in general:

 // Prompt the user for filename in save-as situation
 //
-if (!doc.IsEmbedded() && doc.GetDocPath() == 0)
+if (!doc.IsEmbedded() && (!doc.GetDocPath() || doc.IsReadOnly()))

Similarly extend TDocManager::FileSave:

-if (!doc->GetDocPath()) {
+if (!doc->GetDocPath() || doc.IsReadOnly()) {
   CmFileSaveAs();
   return;
 }

This presumes that TDocument::IsReadOnly is introduced. It could be a virtual function with the base implementation checking the open mode:

virtual auto IsReadOnly() const -> bool
{ return (OpenMode & TDocMode::ofWrite) == 0; }

It can then be overridden by TFileDocument to check the read-only attribute of the actual underlying file as well.

auto IsReadOnly() const -> bool override
{
  const auto isPathReadOnly = [](LPCTSTR) {...};
  return TDocument::IsReadOnly() || isPathReadOnly(GetDocPath());
}

This gives us a general solution that respects the open mode as well as the actual file attributes for TFileDocument and subclasses.

(By the way, the low-level TTextBuffer::Save should also refuse to save if the destination file is read-only. Currently, it renames the read-only file as a backup, then replaces it by a new file. Instead, it should just fail and return false.)

first check if the file that is being saved to is read-only and if so, to redirect [to Save As]. This should handle cases like when the file was not read-only initially, but has become read-only in the meantime.

Exactly; I fully agree. The editor flag isn't a mirror of the file attribute. It is its own thing, even though it is initially set for a read-only file. Setting the flag for a writeable file doesn't mean you want the file to become read-only. It just means you do not intend to edit and want to protect against it. This is how LibreOffice works by the way. And vice versa, clearing the flag doesn't necessarily mean you intend to overwrite the read-only file. You may want to create a modified copy.

[Ognyan Chernokozhev]

Agreed, it is a generic flaw with the Doc/View implementation, not one specific to TCoolEdit. I guess Claude assumed it was constrained to making changes only in the CoolPrj project, and that is what it did.

Still in the mood for experimentation, I simply pointed Claude to your comment and asked to implement the suggesstions.

Here is the reply:

---

Thanks for the review, Vidar. I agree that the UI belongs in the doc manager, so I have reworked the solution along the lines you suggested in [r8945]:

• Added virtual auto TDocument::IsReadOnly() const -> bool, overridden by TFileDocument to check the read-only attribute of the file.
• TDocManager::FileSave and TDocManager::FlushDoc now redirect to "Save As" if !doc.GetDocPath() || doc.IsReadOnly().
• Removed TSource::CanClose, TSource::SaveAs and TCoolEdit::CmSave again. TSource now just overrides IsReadOnly to check the file attribute.
• TTextBuffer::Save now refuses to replace a read-only file, instead of renaming it as a backup and writing a new file.

One deviation: the base implementation of TDocument::IsReadOnly returns false, rather than checking the open mode. TDocManager::InitDoc opens documents with ofRead unless the template has dtNoReadOnly, which none of our templates set (e.g. CoolDemo and the tutorials use plain dtAutoOpen). OWLMaker calls Open(0, ...), so the mode is 0. Writing is done through a separate OutStream(ofWrite), so these documents are writable in practice. Checking ofWrite would therefore redirect every Save to "Save As" in those applications. I have documented this in the TDocument::IsReadOnly comment.

Also, to keep the editor flag separate from the file attribute, as you described, I renamed the editor mode accessor to IsReadOnlyModeEnabled (in both TSource and TCoolEdit), matching EnableReadOnlyMode and the other mode functions. This was necessary anyway, since the old TSource::IsReadOnly() const would otherwise have overridden the new virtual function with the wrong meaning.

Note that this is a breaking change: code calling TCoolEdit::IsReadOnly must be updated, and code calling TSource::IsReadOnly will still compile but now gets the file status. Also, since TDocument has a new virtual function, dependent code needs to be rebuilt.

[Ognyan Chernokozhev]

Note that previously RichEdit crashed when opening a read-only text file and then trying to save it. With the new changes, it now opens the SaveAs dialog.

[Vidar Hasfjord]

@jogybl wrote:

Agreed, it is a generic flaw with the Doc/View implementation, not one specific to TCoolEdit. I guess Claude assumed it was constrained to making changes only in the CoolPrj project, and that is what it did. Still in the mood for experimentation, I simply pointed Claude to your comment and asked to implement the suggesstions.

Super! The revision [r8945] changed the newline convention for "source.cpp" (reverted in [r8948]), but other than that it looks flawless. I've now merged it into Owlet [r8946].

However, seeing the name clash, I'm now uneasy about the name. While renaming the old TSource::IsReadOnly to IsReadOnlyModeEnabled is a great move, thereby aligning it with the other modes, there is still a high risk of programmer confusion.

Looking at the state of the document in memory, the name "IsReadOnly" doesn't reflect whether the document is mutable or not. Hence it is misleading in that sense as well.

And note that there may be other reasons, other than a read-only file attribute, why a file cannot be overwritten. In particular, it may have restrictive access rights.

Looking at what the function is for, it basically needs to tell FlushDoc and Save whether committing to the current path is possible. So a brute name would be "CanCommitToDocPath". The function can then subsume the tests for null and empty path.

-if (!doc.IsEmbedded() && (!doc.GetDocPath() || doc.IsReadOnly())) {
+if (!doc.IsEmbedded() && !doc.CanCommitToDocPath()) {

Just "CanCommit" is tempting, but is too broad, because FlushDoc and Save's query and remedy are only concerned with the path. "CanCommit" also implies, if it returns true, that Commit is guaranteed to succeed, which is not the case.

An alternative is "IsDocPathOK", but the path may be fine for reading and not for writing. We can add a parameter to deal with that, e.g. IsDocPathOK(TDocPathFlag::Committable), but at first glance, it feels like over-engineering. On the upside, it would allow us to replace all those ugly direct checks on the path, e.g. if (GetDocPath() && *GetDocPath()). But these empty-checks are perhaps better dealt with by changing GetDocPath to return a tstring, and then just use tstring::empty.

Perhaps "IsDocPathCommittable" or "HasCommittableDocPath"?

I prefer "CanCommitToDocPath", though.

What do you think?

(Edit: I've now changed the name to CanCommitToDocPath in Owlet [r8955] and merged the change into the trunk [r8960], until or unless we find a better name. Note that TSource::IsReadOnlyModeEnabled has been reverted back to IsReadOnly, matching TTextBuffer::IsReadOnly, but TCoolEdit::IsReadOnlyModeEnabled has not, since it is consistent with the accessors for other modes in the editor.)

PS. Good call by Claude on not conflating the open mode with the read-only state of the path.

Note that previously RichEdit crashed when opening a read-only text file and then trying to save it. With the new changes, it now opens the SaveAs dialog.

I saw similar improvement in Lima VVA. It does its own thing for CmSave (perhaps due to this very issue), so in that case just reports a commit error as before, hinting that the file may be read-only. But in the CanClose scenario, before if you selected "Yes" to save and the commit failed, the message box just flickered and asked again. Now you are redirected to SaveAs before the commit is attempted. Brilliant!