Nikhil's blog

Code reading and Bug fixing 101

Posted on Mar 23, 2011

Summer is here, and Google Summer of Code is on its way. The biggest hurdle new contributors often face (after compiling trunk ;)) is to get their head around the project they would like to work on, understanding how it works, where the parts fit in, and how to fix bugs or make improvements. Speaking from my experience, it took me the better part of a month to understand how KWin worked before I could actually hack on it for Season on KDE.

This is my attempt to explain how I approach new code and the tools I use. To demonstrate, I am going to try and fix this bug in Amarok. I am sorry for stealing a Junior Job.

The most important thing when working on existing code is:

DO NOT modify existing code that works, even if you absolutely have too1


This means, avoid changing function signatures, variable names, and especially surrounding code that does not affect you. You may inadvertently introduce bugs.
That said, remember that you are using a version control system, so code fearlessly. The best way to understand new code conceptually is to liberally insert some kind of debug/print statements all over relevant functions. With that in mind, let’s start.

Understand the problem

The bug report says:

When you filter all items in Amarok, a warning notice is shown that “tracks have been hidden in the playlist”. However, if you filter, and then delete all results of that filter from the playlist, this warning is not shown

Reproducible: Always

Steps to Reproduce:

Set a filter
Remove all matcheS
This is a fairly straight-forward bug with well written steps to reproduce. If not, its best to ask questions on the bug tracker or talk to someone more experienced to understand what the bug/feature actually requires. The next step is to reproduce the bug. If you cannot make the bug occur, you have no way to prove that your changes fix it. Again, try to reproduce bug in the bleeding edge version of the project. Otherwise it has already been fixed. So let’s add some tracks to the playlist, put something in the filter text. Observe that if you put a pattern that doesn’t match any track, you get the warning. Now change the pattern to match a few tracks. Remove those tracks from the playlist. No warning! This is what we have to fix.
Up to this point, the process has been just what a user would do, now its time to enter the code.

Where is the problem?

The Amarok source tree is pretty large. By convention, all source code is in the src/ directory. This is true for almost all open source projects. But what now?
The easiest way to find out the source of the problem is to find a nice clue in the interface that gives us a good idea of what the relevant file will be called or where we can make a change.
At this point, let me introduce a tool called ack, which is grep optimized for programmers. I won’t bother describing the features, but trust me, you should be using it!
Let’s enter the Amarok source tree, and try to find “Warning: tracks have been hidden in the playlist”. Why so, well, since it is being hidden and shown, it obviously has something to do with our task.

$ cs amarok
$ cd src # all code is here
$ ls
...
playlist
...
$ ack "Warning: tracks have been hidden" playlist/
playlist/ProgressiveSearchWidget.cpp
45:    m_warningLabel = new QLabel( i18n("Warning: tracks have been hidden in the playlist"), this );

Notice a few things. First, I ran the search only on the playlist directory. You could have run it on src and it would just have taken more time. But its good to do a ls on src because knowing the directory structure is a good way to get the highest-level overview of a project. playlist is a suspiciously strong pointer to our bug. So let’s run it on this.

This seems like a good start, there is a label being created that has the message. But we need more information. So fire up your editor/IDE and go to line 45 of src/playlist/ProgressiveSearchWidget.cpp. Knowing nifty utilities in your editor is a good investment, you absolutely must know the shortcut to jump to a specific line since line numbers are used all over programming, in compilers, editors, and humans talking to each other. With vim it’s:

$ vim playlist/ProgressiveSearchWidget.cpp +45

Now, let’s see where this label is being manipulated. For this we need another useful editor feature, to find instances of symbol under cursor. QtCreator or KDevelop allow you to just right click on the variable name and find all uses. With vim I hit the * key.

void ProgressiveSearchWidget::showHiddenTracksWarning()
{
   m_warningLabel->show();
}

void ProgressiveSearchWidget::hideHiddenTracksWarning()
{
   m_warningLabel->hide();
}
And there is our first break. A pair of functions which show or hide the label. Continue this technique of ‘follow the useful symbol’. Let’s see where showHiddenTracksWarning() is being called. In this case, its just above the definition.
void ProgressiveSearchWidget::noMatch()
{
...
   if( m_showOnlyMatches )
       showHiddenTracksWarning();
}

void ProgressiveSearchWidget::showHiddenTracksWarning()
But if you try to follow noMatch(), it won’t be in this file. Running ack again:

$ ack 'noMatch' playlist/
playlist/ProgressiveSearchWidget.h
130:    void noMatch();
playlist/ProgressiveSearchWidget.cpp
216:void ProgressiveSearchWidget::noMatch()
playlist/PlaylistDock.cpp
144:    connect( m_playlistView, SIGNAL( notFound() ), m_searchWidget, SLOT( noMatch() ) );

At this point, I assume you are smart enough to follow the code on your own, because pasting every sample here is annoying :) If you see playlist/PlaylistDock.cpp, then m_playlistView represents the playlist in some manner. m_searchWidget on the other hand is the place where the user types the filter. So when the playlist can’t find any matches, it tells the search widget, which then shows the label!

Except, something is going wrong in the exact circumstances of the bug. One possible explanation is that noMatch() never gets called. Your first idea might be – lets call notFound() when tracks are deleted and be done with it. But let’s dig deeper.

So what is m_playlistView? Simple, open the header file for PlaylistDock.

PrettyListView* m_playlistView;

Hmm, now where might PrettyListView be? At this point, you can use your fancy IDE, but I am going to introduce another ancient UNIX tool – find. When programming, it is helpful to generalize assumptions about how the code is organized and named, since it makes navigation a lot easier. If you’ve seen even the Amarok code that is just in this article, you can see that classes usually map one-to-one to file names.
$ find -iname 'prettylistview*'
./playlist/view/listview/PrettyListView.cpp
./playlist/view/listview/PrettyListView.h
find takes a lot of powerful options, but here we say

Find all files from the current directory (src) downwards, whose name (-name) matches the pattern ‘prettylistview*’, but ignore case (-iname)

and there you go, open PrettyListView.cpp and search for notFound. So notFound is emitted by the PrettyListView::find method, which itself is incidentally connected to ProgressiveSearchWidget::filterChanged (connected in Playlist::Dock::polish). Here is how our mental model goes till now:


Mental model

When the user types something, ProgressiveSearchWidget::filterChanged is being invoked, which is triggering PrettyListView::find. When find sees that no tracks are visible, it emits notFound. This triggers ProgressiveSearchWidget::noMatch which shows the warning label.
With some more effort, you will also find that PrettyListView::find is only called due to filterChanged. We can now use this knowledge to figure out why the bug happens.
Deleting a track does not change the filter in any manner. So following the call chain, noMatch never gets called when tracks are deleted to re-evaluate the situation!


The solution

The obvious solution is to somehow call PrettyListView::find manually when tracks are deleted in the playlist. But how will we know that? We will again use some GUI hints. Tracks are deleted by right-clicking them and selecting ‘Remove From Playlist’. If you ack this, you will find the action triggers a removeSelection() (playlist/view/PlaylistViewCommon.cpp). The type of the receiver is just QWidget, so it would be a hassle to try and find out the type of parent, but there is only one definition of a method called removeSelection() related to playlists. It is in PrettyListView. At this point, you suddenly feel empowered as the solution clicks before your eyes.

As soon as we remove the tracks, we can just call find() again and we will be done!

A fundamental constraint that inhibits us is the loose coupling made possible by signals and slots.
PrettyListView is unaware of ProgressiveSearchWidget and its properties.
All it knows is that it is expected to emit certain signals (found/notFound) and things happen. PrettyListView also does not have direct access to whether items are being filtered or not. These things are passed on to it from the filterChanged() signal’s arguments.
At this stage, I hope that you now have a good idea of how things are connected. For a little indepth understanding of how things are playing out see 2.

With this in mind, there are atleast 2 solutions that come to mind. Our task is to somehow force the filter action to be performed again on the view so that it emits the relevant signals. Unfortunately the view itself does not have access to the showOnlyMatches attribute present in the dock, and in the SortFilterProxies. We can,

1. Use PlaylistDock, which has access to the search widget

A roundabout method I did first, involving:
  1. adding a getter, ProgressiveSearchWidget::currentSearchFields().
  2. Connecting to the controller’s (The::playlistController()) changed() signal, a custom slot in Playlist::Dock called slotReapplyFilter().
  3. slotReapplyFilter() calls PrettyListView::find() again with relevant arguments.

2. Make the view store showOnlyMatches

A much simpler three line change.
  1. Add a bool m_showOnlyMatches to PrettyListView.
  2. When PrettyListView::showOnlyMatches() is called, along with passing along the value to the playlist, we also set m_showOnlyMatches.
  3. In PrettyListView::removeSelection(), call PrettyListView::find() since we now have complete knowledge.

Programmers exchange changes in code with something called a diff, or a file which only logs the changes made in the code.

$ git diff
diff --git a/src/playlist/view/listview/PrettyListView.cpp b/src/playlist/view/listview/PrettyListView.cpp
index cd650f6..e81d396 100644
--- a/src/playlist/view/listview/PrettyListView.cpp
+++ b/src/playlist/view/listview/PrettyListView.cpp
@@ -194,6 +194,8 @@ Playlist::PrettyListView::removeSelection()
        QModelIndex newSelectionIndex = model()->index( firstRow, 0 );
        setCurrentIndex( newSelectionIndex );
        selectionModel()->select( newSelectionIndex, QItemSelectionModel::Select );
+
+        find( The::playlist()->currentSearchTerm(), The::playlist()->currentSearchFields(), m_showOnlyMatches );
    }
}

@@ -912,6 +914,7 @@ void Playlist::PrettyListView::updateProxyTimeout()

void Playlist::PrettyListView::showOnlyMatches( bool onlyMatches )
{
+    m_showOnlyMatches = onlyMatches;
    The::playlist()->showOnlyMatches( onlyMatches );
}

diff --git a/src/playlist/view/listview/PrettyListView.h b/src/playlist/view/listview/PrettyListView.h
index f22a7c8..612be0b 100644
--- a/src/playlist/view/listview/PrettyListView.h
+++ b/src/playlist/view/listview/PrettyListView.h
@@ -139,6 +139,8 @@ private:

    QTimer *m_animationTimer;

+    bool m_showOnlyMatches;
+
public:
    QList<int> selectedRows() const;
};
Since this is a minor patch, I could just commit this change. But I’m not going to for two reasons:
  1. You don’t have commit access, so I will show you how its done in that case.
  2. This is not code that I have worked with before, so however small, it should go through review. The reason is that I may have inadvertently affected the system due to incomplete knowledge. This is were unit and regression tests can also come in handy.
So let’s first get our diff into a file

$ git diff > /tmp/amarok-bugfix-260352.patch
Now hop on to reviewboard, and submit it. (Don’t actually do it, I’ve already done it!). Here is how the final submission looks.
Now wait for somebody to reply or commit on behalf of you. That is it! Your first bug fix.

Code reading only gets easier as you go along. It does not involve complex equations, but instead mentally executing the program just as a computer would. The catch is to be able to go from the low level details to creating the software architecture in your head, and watching the messages flow through the program. You must know your tools really well since they are a big time-saver.

To summarise, the following usually help to get a good idea of the code and allow you to fix bugs or add features.


  1. Use UI hints
  2. Use debug statements where required.
  3. Sometimes purposely crashing a program (assert(0); anyone?) is a great way to see what code-path is being followed.
  4. Follow the code along, until you can build a mental model.
  5. Use the mental model to figure out multiple solutions.
  6. Implement a solution.
  7. Test it.
  8. Submit for review or commit.

I hope this post has been useful. If you do wish to continue participating in an open source project, it is a good idea to spend the first few days just glossing over various parts of the code to get a feel of the system. Then you can go into your little section and get comfortable.


  1. except when really required
    ↩

  2. If you don’t understand what I say in this paragraph, accept it at face value and continue. There is a powerful design pattern called Model-view-controller that allows separation of concern between the playlist contents, modifying them and drawing them. This is embedded into Qt using the various Item/View classes. Amarok uses this extensively. (probably one of the biggest uses of Model View within KDE?) The PrettyListView is just deciding how to show and draw the tracks, which are actually stored in a set of models. Similarly a Controller will often modify the models as required, and the changes will automatically show up in the PrettyListView.
    ↩