Friday, January 11, 2013

The Role of Good Software Design in wxMaxima

See: wxMaxima Issue #233
Patch: wxMaxima Pull Request #241

I posted previously about my first attempt to solve this issue. I'm going to briefly discuss why that approach was wrong, and explain a little something about how wxMaxima is architected. Hopefully future developers on wxMaxima will benefit from this discussion, and I hope that developers on other projects will also try to avoid similar mistakes.

I decided to make this post about good software design because the actual patch was rather simple, and the reason for that comes from the beauty of good software design.

Good Design Prevents Bad Errors


In the past, I have complained to my colleagues about the fact that sometimes changing the order of the #includes in a C++ source file can affect whether or not your code will compile. This was, of course, in my early days of C++, where most of the inner workings of the language seemed pretty mysterious. Unfortunately, knowing more about a technology can sometimes [read: often] result in ugly hacks, like the #ifdef hacks I tried at the end of my previous post, or StarTrek Voyager's deflector array (satired here). Or just ask anyone who has ever had to maintain a Perl script.

But I've never had a problem like that when I was making changes with good software design in mind.

By now, you would think that I would know to look for the source of an error caused by bad design, rather than trying to decrypt C++ compiler messages, but whenever I see an interesting compile error, my ego gets in the way and says, "I'm clever enough to fix this!" In reality, when you see a strange error you should take a step back instead of trying to fix it right away, and try to see where you might have gone wrong. Especially when you're working on a well-established and stable project, it's probably definitely* your fault when something breaks, and you can't discount the possibility that it was a bad design decision on your part that caused the problem.

[*Unless you've exhausted all other possibilities and you can prove it was an existing design flaw that somehow your patch revealed. And even if you're right, it will probably be more expensive to fix the right way, so it's still on you to make it work within your patch. (That's another problem in the software industry, but it's hard to make a practical argument against it.)]

Understanding the GUI Layout


With that discussion out of the way, let's take a moment to understand the component design of wxMaxima. I'll provide some pictures to make this a bit easier to digest, but first a quick description that I've learned from a few weeks of working with the code.

wxMaxima.cpp and wxMaximaFrame.cpp define the GUI layout of the main window, the top level containers, the menus, hotkeys, etc. All the basic high-level GUI design.

MathCtrl is the pane which contains all the content, and the content is composed of a series of GroupCells (which are a subtype of MathCell). Most of the GroupCells are statically typed as MathCells in the source, but that's probably an artifact of an earlier version and not terribly important here. The MathCtrl corresponds to the document itself, so it makes the most sense that this is where we want to decide whether the document is in an unsaved state. It helps that the MathCtrl itself deals with user interactions with the content displayed in the MathCtrl.

The output is composed of a series of subtypes of MathCells (for instance the SqrtCell in Fig. 1 below), but that's not important for this patch.

Fig. 1: GUI components annotated in color.

Understanding the Relationships between Objects


If you've ever really studied what it means to have an Object-Oriented design (not just the mere fact of using classes, but actually designing software in an Object-Oriented way), you probably know that the main strength that OO design offers us is the ability to abstract and assign responsibility. In order to make sure that we have good abstract and role definitions, we need to do a few things:

First, we want to make sure that the object/module dependency graph is a tree. That is, there are no cycles: if module A calls module B, then module B should not call module A either directly or indirectly. Basically, class A is class B's drill sergeant. A can give orders to B, give data to B, or ask for data from B. B just does its job with whatever A gives it. If B starts giving orders to A or speaking out of turn (even if B thinks he's being helpful), there's going to be trouble.

The problem with my first approach to my patch is that I failed to acknowledge the relationship between Objects in wxMaxima (shown in Fig. 2 below), and wanted to give GroupCell a reference to the MathCtrl which contained it. Based on the description above, you can see that that is a pretty scary situation.

Fig. 2: A partial graph of Object relationships in wxMaxima (generated by Doxygen). Relationships directly relevant to this discussion are highlighted in yellow.

Luckily, g++ was kind enough not to allow me to compile the code in that way. I can't say for sure, but if it is even possible to get a C++ program to compile with that kind of circular dependency, I would have been really unlucky if the code had compiled without giving me any trouble at all. Circular inclusions are a tricky problem in C++... you get a weird compiler error and fixing it is rather touch-and-go, but you should definitely check whether you really want to introduce that circular dependency.

Sidebar: There are some metrics that can be used to judge the quality and maintainability of Object-Oriented code. One is how tightly coupled your classes are to other classes. If you have a circular dependency between two classes, those classes are tightly coupled. This is bad for code maintainability because if you change something in either one of those classes, you have to update the other class to adapt to the change. In good OO design, we want changes made to any class not to require changes to another class. Often times it takes several rounds of refactoring and some knowledge of design patterns to get this to be completely true, but we can avoid the tight-coupling problem really easily by just being aware of it.
Tip: for those who use Visual Studio, you can do source code analysis (at least in a C# project) and it will show you maintainability metrics like the one I described above.

Analysis and Patch


So basically, even though GroupCell is responsible for actually handling the folding and unfolding actions, MathCtrl is the class which processes the user input which should cause the folding to occur. MathCtrl passes the message on to the relevant GroupCell(s) which in turn execute the folding algorithms. Since hotkeys and menu items are registered with wxMaxima, any hotkey actions which cause folding will cause the wxMaxima object to pass a message to MathCtrl which, again, will pass a message to the relevant GroupCell(s).

A quick search in the file yielded 5 functions which process folding. All I needed to do was, for every time a folding event occurred, I needed to mark the document as unsaved. Luckily the functionality already exists for this and happens to be conveniently packaged as the simple function call: MathCtrl::SetSaved(false), which happens to be a simple setter function. The magic happens in the wxMaxima class, which polls for a change in the saved status when the window is idle (wxMaxima::OnIdle()). Since in almost every case, UpdateMLast() was called, and it wouldn't hurt to call it in the final case, I refactored the code which handles folding events into the function MathCtrl::FoldOccurred().

The logic of the patch is contained in commit e27d891. (The other commits in the pull request are documentation and maintenance.)

If you don't count the refactoring (which was a good choice because it will help with maintainability later, if the folding functionality needs to be changed again), the patch added about 5 logical lines of code, and the line I added in each place was the same.

If the function FoldOccurred() had already existed and had been used as I have used it, the change would have been a single line of code.

That's the power of good design. Highly maintainable code, which requires only small changes to extend or correct functionality. And the lesson to be learned here is that if you take the time to understand the design of the code you're working on, you can save yourself a lot of time in maintaining the code later on.

Monday, January 7, 2013

Attempt on Issue #233: Fold actions should register as modifications to document

See: wxMaxima Issue #233

Edit: the approach I took in this post to try to fix the issue was in error. See the follow-up post for an explanation of why it was incorrect, and for my description of the actual fix.

Since wxMaxima's describes itself as a "document-based interface for...Maxima", it would stand to reason that any actions within wxMaxima that would affect how a document will appear should register as changes. wxMaxima supports a section hierarchy and allows folding of sections, and while the state of the fold is saved in .wxm and .wxmx documents saved from wxMaxima, changing the state of a fold alone does not register as a change which will allow the file to be saved again.

wxMaxima uses a familiar "Save" / "Save As..." scheme. You can "Save" your document by pressing Ctrl+S at any time the document has unsaved changes. You can "Save As..." at any time, but it's less convenient to do so, especially if you plan to overwrite the file. Luckily "Save As..." gives a workaround for the problem.

Since it seems like a relatively simple user-interface improvement which will improve the intuitiveness of the document composition system that wxMaxima offers, I decided to take on this issue.

Investigation

In order to fix a bug, first you need to find the code that is responsible for the incorrect behavior. It is often easier to locate a bug, however, than to find the code that should be responsible for a feature that doesn't exist yet, but it is a similar process. This issue appears at face value to be a bug, but depending on how the "unsaved document" functionality was implemented, the folding functionality might simply be missing rather than broken, so we might be looking for where and how to add a feature on this one.

In a graphical program it can be somewhat challenging to locate where a feature should be included, especially when that feature may touch non-graphical (backend) portions of the program.

In wxMaxima, cells that can be folded have a square button displayed on their left side that you click to toggle the fold. Since I knew that the functionality I want to add has to do with folding, I did a quick <code>git grep "Fold"</code> and found that there are some functions related to folding in the GroupCell class. At this point, how the folding button actually works doesn't matter, because we have located the code that is processed when a fold occurs.

I also needed to find the code that sets the asterisk next to the filename when a file has been modified. <code>git grep "*"</code> found me a lot of pointers but also found me a line which set the "*" in the window title. I saw that the function to set the title in wxWidgets is called SetTitle, so I searched for that function name to make sure that the window title isn't set anywhere else.

Looks good. Now I need to find out how the program decides whether to include the * in the title. Looking at the function wxMaxima::ResetTitle, I notice a few things that look concerning. First, there's a lot of conditional compilation in this method, relating to the platform on which the program is compiled. In this case, a special case is made for Mac, probably because setting window titles in OS X works considerably differently or follows a different paradigm than the other platforms.

At this point, I got a little discouraged, but I decided to consider the parts that I could understand right away and I realized it's not all that complicated for my purposes. What we know is that the title is only set from here, and the logic for doing so is already completed. So we need to look at the control logic. If a variable, m_fileSaved is false, then the asterisk is added to the title. That variable is set in this function from the variable passed into the function. So we need to look at the callers of this function to find out where that value is coming from. We need to set that value to false when we have a fold event.

This looks promising, because ResetTitle() is not called from many places. wxMaxima::OnIdle, which says in the comments that this is where we check if the document is saved, and otherwise it is called only when documents are loaded, which we're not interested in right now. So looking at OnIdle(), we can see that the value is coming from m_console, which is a MathCtrl, and the method IsSaved() returns the value of MathCtrl::m_saved. We can set this value with MathCtrl::SetSaved(), but other than that, this seems like a dead end. We will need access to the MathCtrl which represents the current document when a Fold event occurs so that we can call SetSaved(), which will result in the window title being set.

Solution

Back in GroupCell.h, I note that there is not a reference to a MathCtrl in GroupCell. Nor is there a reference to a MathCtrl in GroupCell's parent class, MathCell. I looked around for a bit but since any kind of fold action eventually comes down to GroupCell, the easiest way to make sure that all folds will update the saved status is to update from GroupCell. That means that we have to add a MathCtrl reference to GroupCell, which means changing the GroupCell constructor, and that's a little scary.

Remember that even if the maintainer of the project objects to your change, you can always revert and go about it a different way. To make merging easier, I make a branch called "issue233" (rather than working in "master") and get to work.

I added m_mathCtrl to GroupCell.h, and add a function called FoldEvent() which I will call from every Fold-related function to let the MathCtrl know that a fold has occurred. I update the constructor to take a MathCtrl* and update m_mathCtrl. I included "MathCtrl.h" in the files which now newly use the MathCtrl type.

I use the call hierarchy to find a few locations where GroupCells are constructed and update those constructors to take a reference to a MathCtrl, and then try to compile.

Debugging

And right away, I got this error message:

g++ -DHAVE_CONFIG_H -I.    -DPREFIX=\"/usr/local\" -g -O0 -I/usr/local/lib/wx/include/gtk2-ansi-release-2.8 -I/usr/local/include/wx-2.8 -D_FILE_OFFSET_BITS=64 -D_LARGE_FILES -D__WXGTK__ -pthread -MT main.o -MD -MP -MF .deps/main.Tpo -c -o main.o main.cpp
In file included from MathCtrl.h:29,
                 from wxMaximaFrame.h:32,
                 from wxMaxima.h:23,
                 from main.cpp:32:
GroupCell.h:44: error: ‘MathCtrl’ has not been declared
GroupCell.h:112: error: ISO C++ forbids declaration of ‘MathCtrl’ with no type
GroupCell.h:112: error: expected ‘;’ before ‘*’ token
make[1]: *** [main.o] Error 1
make[1]: Leaving directory `/home/doug/dev/wxmaxima/src'
make: *** [all] Error 2

This is frustrating because I know I included the file. What's actually going on here is circular inclusion. MathCtrl includes GroupCell in order to work properly, but now GroupCell needs MathCtrl in order to work. The solution is, unfortunately, to just declare "class MathCtrl;" and let the linker figure it out, but this is feeling more and more like I might be taking the wrong approach. Might as well follow through with it now.

After changing to the forward declaration, I have the following error:

g++ -DHAVE_CONFIG_H -I.    -DPREFIX=\"/usr/local\" -g -O0 -I/usr/local/lib/wx/include/gtk2-ansi-release-2.8 -I/usr/local/include/wx-2.8 -D_FILE_OFFSET_BITS=64 -D_LARGE_FILES -D__WXGTK__ -pthread -MT GroupCell.o -MD -MP -MF .deps/GroupCell.Tpo -c -o GroupCell.o GroupCell.cpp
GroupCell.cpp: In member function ‘void GroupCell::FoldEvent()’:
GroupCell.cpp:1169: error: invalid use of incomplete type ‘struct MathCtrl’
GroupCell.h:40: error: forward declaration of ‘struct MathCtrl’
make[1]: *** [GroupCell.o] Error 1
make[1]: Leaving directory `/home/doug/dev/wxmaxima/src'
make: *** [all] Error 2

Apparently the problem is that because of the include order, the class has actually been included already before the forward declaration, so I ended up with this terribly messy solution:

#ifndef _MATHCTRL_H_
#include "MathCtrl.h"
#else
class MathCtrl; // provide class declaration because of circular inclusion
#endif

So this way, if the class hasn't already been included, we will include it, and otherwise, we will have the forward declaration.

Unfortunately while compiling that file worked just fine after that point, I broke the compilation of other files. This is looking none too promising. Luckily, it's a learning experience, and we're seeing that sometimes the first solution that occurs to us is not always the best. Tomorrow, I'll try to approach the folding from the possibly messier but less invasive method of dealing with folding from the MathCtrl class.

Sunday, January 6, 2013

Starting Open Source Development with wxMaxima

wxMaxima is written in C++ using wxWidgets. My primary development environment is Linux (Ubuntu) with gcc, and I'm building wxMaxima against a version of wxGTK which I compiled from source.

Since I've made enough contributions to wxMaxima now to feel like I have a reasonable handle on the code base, I thought I would share my experiences developing for this project and what I've learned about open source in general.

Also my hope is that my keeping a blog about the work I'm doing, I'll force myself to justify the decisions I make, and I can learn from my mistakes, and hopefully other wxMaxima and Open Source developers can learn from my mistakes as well.

Open Source Development in General

I'll start by saying that most open source projects I've seen are pretty stuffy. You can write about issues in the issue tracker and they never seem to get any attention, and if they get any attention at all it is usually fairly delayed. If you try to make submissions you'll get buried in bureaucracy and they'll be unlikely to be looked at for their content, let alone integrated into the program.

Well, many people feel intimidated by open source and are discouraged by the obstacles that appear, but I happened to read some good advice that recommended working with a much smaller projects than those which get a lot of visibility. Everyone wants to work on the BIG projects, but there are a lot of smaller projects out there which are very useful and where you can make a difference.

My history with wxMaxima

I started visiting the forums, mailing list, and issue tracker about 3 years ago, shortly after I started using wxMaxima. From the beginning I understood that wxMaxima was a wrapper for Maxima, but spending some time with those resources helped me understand just where the line is. Even now, looking at the issue tracker, there are some issues in there where it could be argued either way, and occasionally, issues are posted to wxMaxima that are really Maxima issues, which shows how much visibility wxMaxima is getting that many users don't distinguish them.

Last winter break I tried to contribute, but because of some issues with the version of wxWidgets I had a hard time building the project, and I ended up making some very superficial changes to the code which helped clear up some of those issues. Unfortunately I never had a chance to work on any real issues from the issue tracker, and then I headed back to school for another year (with an internship in the Summer). All in all I was kept really busy, and since I hadn't made any real contributions to the project last winter, I wasn't too jazzed up about trying to contribute again.

Luckily, this winter break gave me a good opportunity to take some time to look at the project. I chose Linux because it generally is easier to develop cross-platform projects on Linux than on Windows, and I started with some simple issues from the issue tracker, and gradually built up to bigger issues, and features (some of my own request, and some from other users). Even the simple issues often require touching many parts of the code.

The owner of the project has been very patient with me and often reviews my pull requests within a few days, and then offers advice for improvement, or accepts my modifications right away. I am now listed as a developer on the project, and I hope to continue working on the project in my spare time.

Since the project is very open and relaxed about contributions from other developers, but the code can be somewhat confusing, I thought it would be a good idea to chronicle the process as I go, so that future developers on wxMaxima or other wxWidgets-based applications have something to refer to.


IDE/Tools

Whenever you're working on a project you're not familiar with, it's always good to have a good IDE at your disposal. I use Eclipse with CDT because I'm familiar with Eclipse from my days of Java development and I know that it includes great code analysis, navigation, and refactoring tools, which make navigating and understanding unfamiliar code SO much easier.

Some features that I use all the time in Eclipse to help me figure out the code:
  • Pressing F3 while the cursor is on a symbol will take you to its definition. You can also Ctrl-LeftClick on the symbol. This is great for navigating code in multiple files especially when you're not sure which file declared a class or method in Object-Oriented systems.
  • Ctrl+Alt+H: Call Hierarchy. This lets you see in one place all of the locations a symbol is used and for fields/members, makes it easy to see which of those uses are reads and which are writes. This is especially useful when you're adding a feature with similarities to another feature. You can see how parts of the code work together in the feature you're emulating so you can make sure you haven't missed any minor details about its implementation.
  • F4: Class Hierarchy. If you don't have documentation about the Object-Oriented structure of the source code, this is a valuable tool to help you see how classes fit together.
There are obviously many others, but learning to use them comes with learning to use the IDE a bit better.