It Gets Weird for a Moment

To figure out what was happening, I decided to run the notepad in GUI mode. I instrumented the keypress and keydown handlers to show me the characters going by:

 public void XMLKeyDownHandler(object objSender, KeyEventArgs kea) { 
  Console.WriteLine("down  {0} {1} {2}", kea.KeyValue, kea.KeyCode,  
  kea.KeyData);  
   if (kea.KeyCode != Keys.ControlKey && 
  kea.KeyCode != Keys.Menu  && 
   kea.KeyCode != Keys.ShiftKey) { 
 GetText(); 
 model.Snapshot(); 
 } 
 if (kea.KeyCode == Keys.Enter && kea.Modifiers == Keys.None) { 
 CallModel(enterAction); 
 kea.Handled = true; 
 } 
 else if (kea.KeyCode == Keys.Enter && kea.Modifiers == Keys.Shift) { 
 CallModel(shiftEnterAction); 
 kea.Handled = true; 
 }   else if (kea.KeyCode == Keys.Y && kea.Modifiers == Keys.Control) { 
 Console.WriteLine("keydown Y"); 
 kea.Handled = true; 
 } 
 } 
 public void XMLKeyPressHandler(object objSender, KeyPressEventArgs kea) { 
  Console.WriteLine("press {0}", kea.KeyChar);  
 if ((int) kea.KeyChar == (int) Keys.Enter) { 
 kea.Handled = true; 
 // this code is here to avoid putting extra enters in the window. 
 // if removed, when you hit enter, the new <P> line breaks in two: 
 // <P> 
 // </P>  like that. 
 } 
 else if ((int) kea.KeyChar ==  26) { 
  Console.WriteLine("restoring");  
 model.Restore(); 
 model.Restore(); 
 PutText(textbox, model.LinesArray(), model.SelectionStart); 
 kea.Handled = true; 
 } 
 } 

And in TextModel:

 public void Snapshot() { 
  Console.WriteLine("snapping");  
 SavedModels.Push(new TextModel(this)); 
 } 

Note that there are a couple of changes here. The Down handler now says Keys.Menu instead of Keys.Alt. I learned that when I typed Alt, I didn t get a Snapshot, and the printout called the key Menu in some places, not Alt. Making the change shown got the snapshots to happen. And something else very weird is happening: the Notepad is now doing Undo exactly as we want it to! Typing the various commands ”Enter, Alt+S, and so on ” inserts tags just like always. We can type inside them as we should be able to. When we do Ctrl+Z, the characters disappear one at a time. When a tag is empty, one more Ctrl+Z removes the tag. The cursor moves around just as we would think it should.

This tells me a couple of things. First, the code we have is nearly right. Second, the way we are pushing characters to the TextBox for testing isn t working. In particular, it seems that adding the KeyPressHandler call for the characters didn t help at all. I ll remove that and see if it changes anything. Sure enough, the test still fails in the same way, so the KeyPress wasn t helping. And the notepad still works just fine. I exercised it pretty hard, even moving the caret with the arrow keys and with the mouse, and Ctrl+Z just goes around deleting just the characters you would expect. The backspace and Delete key work as you would expect. Typing over selections works. It all works, except the test.

There s one odd thing. Look at the code up there for Ctrl+Z, KeyChar == 26. Two restores are there, not one. The reason, of course, is that up where we do the KeyDown, we are snapshotting on every character, including the Ctrl+Z. We would like to fix that, but first let s think a bit about what to do.

Remember that the Form isn t really running when we do our customer tests. We have created a Form and connected it to the model, but the widgets aren t displayed on the screen. We have no event loop running, because we haven t done the Application.Run(). We are sending things to it by calling our handlers directly. Usually our handlers are on an event chain, from the Form to the textbox to us...and if our handler doesn t deal with something and set key.Handled = true, then the TextBox takes it. That s not happening here, and it won t happen without a lot of trouble.

In a different situation, we might want to run things all the way through, with SendKeys(), a function in Microsoft Windows that lets you type into a running application. I don t think that s appropriate here, because it won t add much information, because the feature is already working, and because we re writing a book here and there won t be much to learn about our topic. Instead, what I want to do is modify the tests and the XML Notepad so that we have a very simple testing interface that is as close as possible to the operation of the notepad in real life.

How about this as a possibility: we ll go back to using the model.InsertCharacter() function to poke the character in, but we won t allow the InsertCharacter() to do the Snapshot(). Instead, we ll also trigger the KeyDown and let it be the only place in the system that does the Snapshot(). We ll be perpetrating a minor hack around the fact that we can t type into the TextBox from our test, but the effect should be the same. Let s build that, make it work, and then look around again.

 private void TypeCharacters(String s) { 
 foreach (char c in s) { 
 form.XMLKeyDownHandler( 
 (object) this, 
 new KeyEventArgs((Keys) Enum.Parse(typeof(Keys), c.ToString(), true))); 
 model.InsertCharacter(c); 
 // the handler doesnt really update the TextBox 
 form.CustomerTestPutText(); 
 } 
 } 

Note that we had to do the form.CustomerTestPutText() method to force the form to update the TextBox in this case. The reason is, of course, that this time the model knows something that the text box does not. With this change, the tests are all running. Let s see if we can make things just a little bit better.

The way we have it now, functionality is in the KeyDown and in the KeyPress. The original purpose of the KeyPress was just to be sure that the TextBox didn t handle Enter. Let s retain that purpose, with these changes:

 public void XMLKeyPressHandler(object objSender, KeyPressEventArgs kea) { 
 if ((int) kea.KeyChar == (int) Keys.Enter) { 
 kea.Handled = true; 
 // this code is here to avoid putting extra enters in the window. 
 // if removed, when you hit enter, the new <P> line breaks in two: 
 // <P> 
 // </P>  like that. 
 } 
 } 
 public void XMLKeyDownHandler(object objSender, KeyEventArgs kea) { 
 if (kea.KeyCode != Keys.ControlKey && 
 kea.KeyCode != Keys.Menu && 
 kea.KeyCode != Keys.ShiftKey) { 
 GetText(); 
 model.Snapshot(); 
 } 
 if (kea.KeyCode == Keys.Enter && kea.Modifiers == Keys.None) { 
 CallModel(enterAction); 
 kea.Handled = true; 
 } 
 else if (kea.KeyCode == Keys.Enter && kea.Modifiers == Keys.Shift) { 
 CallModel(shiftEnterAction); 
 kea.Handled = true; 
 } 
  else if (kea.KeyCode == Keys.Z  &&  kea.Modifiers == Keys.Control) {  
  model.Restore();  
  model.Restore();  
  PutText(textbox, model.LinesArray(), model.SelectionStart);  
  kea.Handled = true;  
  }  
 } 

That s better. I just noticed that the handlers are public and that they can be internal. In fact, one of them can probably be private. I ll change that. We still have the need to override the Enter in KeyPress, but we don t need to process the Ctrl+Z in KeyPress at all. The tests are green!

Now there s that duplicate call to Restore() highlighted up there. We can make that go away if we check for Ctrl+Z in the code that does the Snapshot(). We do have a small dilemma: we don t have a broken test that tells us we must do this. What if we remove the extra Restore() and see whether any tests break...they do not. Why?? A quick look at the *undo command in the customer tests shows us:

 ... 
 if (line.StartsWith("*alt")) 
 ExecuteMenu("&"+line[4]); 
 else if (line.StartsWith("*type")) 
 TypeCharacters(line.Substring(6)); 
 else if (line == "*undo") 
 model.Restore(); 
 else if (line == "*enter") 
 form.XMLKeyDownHandler((object) this, new KeyEventArgs(Keys.Enter)); 
 ... 

We re restoring the model directly, not using a Ctrl+Z character. Let s put the double Restore() back and change that code to use our KeyDownHandler:

 ... 
 if (line.StartsWith("*alt")) 
 ExecuteMenu("&"+line[4]); 
 else if (line.StartsWith("*type")) 
 TypeCharacters(line.Substring(6)); 
 else if (line == "*undo") 
 form.XMLKeyDownHandler((object) this, 
 new KeyEventArgs(Keys.Z  Keys.Control)); 
 else if (line == "*enter") 
 form.XMLKeyDownHandler((object) this, new KeyEventArgs(Keys.Enter)); 
 ... 

This breaks a test, with an odd diagnostic: -1 is not a valid value for value . We weren t expecting anything to break yet. I m going to go out on a limb a bit here. I ll assume that this broke because of what I want to change, and I ll make the changes to the KeyDownHandler code. If that doesn t fix this test and keep all the others running, I ll have to dig deeper. This is a risk: I m on the edge of a debugging rathole. Here s my change:

 internal void XMLKeyDownHandler(object objSender, KeyEventArgs kea) { 
 if (kea.KeyCode != Keys.ControlKey && 
 kea.KeyCode != Keys.Menu && 
 kea.KeyCode != Keys.ShiftKey && 
 (kea.KeyCode == Keys.Z && kea.Modifiers == Keys.Control)) { 
 GetText(); 
 model.Snapshot(); 
 } 
 if (kea.KeyCode == Keys.Enter && kea.Modifiers == Keys.None) { 
 CallModel(enterAction); 
 kea.Handled = true; 
 } 
 else if (kea.KeyCode == Keys.Enter && kea.Modifiers == Keys.Shift) { 
 CallModel(shiftEnterAction); 
 kea.Handled = true; 
 } 
 else if (kea.KeyCode == Keys.Z && kea.Modifiers == Keys.Control) { 
 model.Restore(); 
 //        model.Restore(); 
 PutText(textbox, model.LinesArray(), model.SelectionStart); 
 kea.Handled = true; 
 } 
 } 

Arrgh! Tests still fail, but with a new message:

 c:\data\csharp\notepad\undo.test 
 *Expected 
 <P>ab</P> 
 *Result 
 <P>abc</P> 

It looks to me like it still needs the double Restore(). I ll check that quickly...no! Putting it back gives me a Stack Empty message. That s kind of good news, but I ve overstayed my welcome on this speculation. Fortunately, I took a second look at that complex if statement at the top. It needs a not (!) in front of the last clause. Let s see if that works better...that gets me back to the message about -1 not being a valid value for value .

I am very tempted to start debugging now. I m sure that I can find this problem and fix it, but if my pair were here with me, he would make me back out these changes and find that first bug. So that s what I ll do. The error is coming up with this stack trace:

 at System.Windows.Forms.TextBoxBase.set_SelectionStart(Int32 value) 
 at Notepad.TestableTextBox.Notepad.ITestTextBox.set_SelectionStart(Int32) 
 at Notepad.XMLNotepad.PutText(ITestTextBox textbox, String[] lines, 
 Int32 selectionStart) 
 in c:\data\csharp\notepad\xmlnotepad.cs:line 221 
 at Notepad.XMLNotepad.XMLKeyDownHandler(Object objSender, KeyEventArgs kea) 
 in c:\data\csharp\notepad\xmlnotepad.cs:line 213 
 at Notepad.CustomerTest.InterpretCommands(String commands, String message) 
 in c:\data\csharp\notepad\customertest.cs:line 69 
 at Notepad.CustomerTest.InterpretFileInput(String fileName) 
 in c:\data\csharp\notepad\customertest.cs:line 56 
 at Notepad.CustomerTest.TestAllFiles() 
 in c:\data\csharp\notepad\customertest.cs:line 48 

Line 69, we re not surprised to see, is our sending of the Ctrl+Z. Time for a little breakpointing. At the breakpoint, we ll see what s up. Stepping in, we see that the abc is in, we snap (redundantly), we restore twice, and the model has ab, as we expect. This isn t going to be the time through that causes the problem. Let s see...I ll let it proceed once to find the state at the error.

Debugging wasn t helpful. The error occurs, the system hurls, and I don t get a breakpoint in my code, because NUnit catches the exception. However, I learned that it wasn t the first *undo that failed, and I hypothesize that it is the last. So I ll do it again, waiting until the last *undo, which is the fourth one. Hold on...at the last *undo, after the Snapshot(), the model contains empty P tags. We expect it to remove them. I ll inspect the stack, expecting two entries. That s what I find, one with the P tags that we just pushed (redundantly) and then one with nothing in it. I m vaguely remembering that we put in special code to handle the result of an empty TextModel. Is that the bug???

After the two Restore() calls, we have no lines and a SelectionStart of -1. Ha! I see it. Look at this code:

 else if (kea.KeyCode == Keys.Z && kea.Modifiers == Keys.Control) { 
 model.Restore(); 
 model.Restore(); 
  PutText(textbox, model.LinesArray(), model.SelectionStart);  
 kea.Handled = true; 
 } 

We weren t executing this before in the CustomerTests; we were just calling model.Undo(). So we never tried to push an empty model back to the TextBox. What s happening is that we are telling the TextBox to set its selection to -1, and it doesn t like that. To make things work, I m going to fix the PutText method not to do that:

 internal void PutText(ITestTextBox textbox, string[] lines, 
 int selectionStart) { 
 // this is Feature Envy big time. 
 textbox.Lines = lines; 
 textbox.SelectionStart = selectionStart<0 ? 0 : selectionStart; 
 textbox.ScrollToCaret(); 
 } 

And this fixes the bug! Let s pause to think about that.

We had been testing *undo just against the model, and we are on a general path of pushing the tests to go more through the Form. In so doing, we did something we have never done before, at least not in this way: we set the TextBox SelectionStart to -1, because that was the saved value in the TextModel. We need to think about this, however, because we did not see that same problem when running the XML Notepad as an application. In that case, it apparently pushed a zero all on its own. Why is it different when testing?

We now have two problems on our plate. First, we are trying to get rid of the double Restore(), and second, we want to understand why we had to put that glitch in the code for the -1. First things first, let s see if we can now remove the double Restore(). This time the change works:

 internal void XMLKeyDownHandler(object objSender, KeyEventArgs kea) { 
 if (kea.KeyCode != Keys.ControlKey && 
 kea.KeyCode != Keys.Menu && 
 kea.KeyCode != Keys.ShiftKey && 
  !(kea.KeyCode == Keys.Z  &&  kea.Modifiers == Keys.Control)) {  
   GetText(); 
 model.Snapshot(); 
 } 
 if (kea.KeyCode == Keys.Enter && kea.Modifiers == Keys.None) { 
 CallModel(enterAction); 
 kea.Handled = true; 
 } 
 else if (kea.KeyCode == Keys.Enter && kea.Modifiers == Keys.Shift) { 
 CallModel(shiftEnterAction); 
 kea.Handled = true; 
 } 
 else if (kea.KeyCode == Keys.Z && kea.Modifiers == Keys.Control) { 
 model.Restore(); 
  //        model.Restore();  
   PutText(textbox, model.LinesArray(), model.SelectionStart); 
 kea.Handled = true; 
 } 
 } 

Lesson  

As I read the section just above, it feels to me that I was on the very edge of getting in trouble. The comments about confusion, the extra confusion when things don t work, problems piling on problems: these are all signs of trouble. I m sure you have felt them yourself. This time I got away with it with very little time spent in the rat hole. I was lucky.

What would be a good way for me to avoid this sort of thing? A great way, of course, is pairing . Our pair will not be very tolerant of this sort of thing, and if he isn t pretty sure that we re on the right track, he ll stop us. What could we do when we re working alone? Maybe set an oven timer? I m not sure: it s very easy for me to fall into this mode of just hacking around blindly, and it s not an effective use of my time. We ll see a big example of this in an upcoming chapter.

The tests run. However, my confidence is a bit lessened by this trouble. I m going to run the application manually. I did the same test as the customer one, and it works. We still need more information. We ll add some instrumentation and try it manually and with the tests:

 public void Snapshot() { 
  Console.WriteLine(  
  "pushing {0} lines, sel {1}",  
  this.Lines.Count,  
  this.selectionStart);  
   SavedModels.Push(new TextModel(this)); 
 } 
 public void Restore() { 
 TextModel savedModel = (TextModel) SavedModels.Pop(); 
  Console.WriteLine(  
  "popping {0} lines, sel {1}",  
  savedModel.Lines.Count,  
  savedModel.selectionStart);  
   lines = savedModel.Lines; 
 selectionStart = savedModel.SelectionStart; 
 } 

Running in this mode tells the tale. The first push in the manual mode pushes a selectionStart of zero, while the first push in the automatic mode pushes a selection start of -1. A little searching finds this code:

 private int LineContainingCursor() { 
 if (lines.Count == 0) 
 return -1; 
 int length = 0; 
 int lineNr = 0; 
 int cr = Environment.NewLine.Length; 
 foreach (String s in lines) { 
 if (length <= selectionStart 
 && selectionStart < length+s.Length + cr) 
 break; 
 length += s.Length + cr; 
 lineNr++; 
 } 
 return lineNr; 
 } 
 private Boolean InListItem() { 
 if (LineContainingCursor() < 0) return false; 
 return ((string) lines[LineContainingCursor()]).StartsWith("<LI>"); 
 } 

There s our hack about -1. We have felt nervous about it more than once. I don t see a really good way to improve it, but let s make it a bit better. Let s first fix InListItem():

 private Boolean InListItem() { 
  if (Lines.Count == 0)  return false; 
 return ((string) lines[LineContainingCursor()]).StartsWith("<LI>"); 
 } 

The tests still run. Now let s see who else is relying on that -1 in the LineContainingCursor(), by making it return zero. We get a bunch of errors, all with a negative index in InsertRange():

 public void InsertTags(InsertAction action) { 
 int cursorLine = LineContainingCursor(); 
 lines.InsertRange(cursorLine+1, action.TagsToInsert); 
 selectionStart = NewSelectionStart(cursorLine + 1, action.TagsToSkip); 
 } 

This looks familiar ”the -1 glitch was put in a long time ago to let us handle an insertion on an empty file. If the line count is zero, we want to insert at zero. Let s change that:

 public void InsertTags(InsertAction action) { 
 int cursorLine = LineContainingCursor() + 1; 
  if (Lines.Count == 0) cursorLine = 0;  
 lines.InsertRange(cursorLine, action.TagsToInsert); 
 selectionStart = NewSelectionStart(cursorLine, action.TagsToSkip); 
 } 

The tests run again. Now we should be able to remove that special code in the PutText():

 internal void PutText(ITestTextBox textbox, string[] lines, 
 int selectionStart) { 
 // this is Feature Envy big time. 
 textbox.Lines = lines; 
 textbox.SelectionStart =  selectionStart  <  0 ? 0 : selectionStart;  
 textbox.ScrollToCaret(); 
 } 

But when we do, a bunch of tests break! A little looking around and we find out why: the TextBox, when it is empty, will give us a SelectionStart of -1, but it won t take one back as input! Thanks, TextBox! If that s the case, what about the changes we just made? InListItem() looks about the same either way. The change to InsertTags() is a little awkward , but at least we re not passing an impossible value around from LineContiningCursor(). However, the result we send back, 0, is just as wrong as -1 if there are no lines. InsertRange handles an insertion at location zero, but if we were to try to subscript into Lines, the zero would be incorrect (as would -1).

There is definitely a code smell here, in our handling of the ArrayList of lines, but it s not really related to our main purpose, the implementation of Undo. Undo is passing all its tests so far.