Terminal.Gui icon indicating copy to clipboard operation
Terminal.Gui copied to clipboard

TextView: TextView.LoadFile keeps file open and provides no way of closing it

Open tig opened this issue 5 years ago • 5 comments

The code in TextModel uses File.OpenRead but there's no code that ever closes it.

As a result, clients can't save the file loaded this way.

tig avatar May 19 '20 20:05 tig

@BDisp , thanks for your PR #487 where you added a CloseFile method.

I probably should not have accepted the PR because while it meets the spirit of what I noted in this bug, I'm not sure it is actually correct design.

Instead, I think the reading/writing of the file for TextView should be left up to the caller. If it were up to me, I'd remove all file/stream logic from the class. It is trivial for a client to open/read/write the file and just assign it to TextView.Text as I did in the Editor Scenario (take a look).

Client code to Load:

			if (_fileName != null) {
				_textView.Text = System.IO.File.ReadAllText (_fileName);
				Win.Title = _fileName;
				_saved = true;
			}

Client code to Save:

			if (_fileName != null) {
				// BUGBUG: #279 TextView does not know how to deal with \r\n, only \r 
				// As a result files saved on Windows and then read back will show invalid chars.
				System.IO.File.WriteAllText (_fileName, _textView.Text.ToString());
				_saved = true;
			}

Why mess with re-implementing all of what .NET gives us here?

tig avatar May 22 '20 03:05 tig

I only made it because you mentioned somewhere that was missing.

BDisp avatar May 22 '20 12:05 BDisp

I only made it because you mentioned somewhere that was missing.

You are right, I did phrase it in a way that may have sounded like a request for a Close method.

Are you ok with us just closing #487 and then re-visiting how to fix this issue longer term here?

tig avatar May 22 '20 15:05 tig

But you already merged it.

BDisp avatar May 22 '20 15:05 BDisp

Right. Sorry, i have not had enough coffee this AM.

We can just leave it in for now since it does no harm. Ok?

tig avatar May 22 '20 15:05 tig