Code reading and Bug fixing 101
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.
The most important thing when working on existing code is:
This means, avoid changing function signatures, variable names, and especially surrounding code that does not affect you. You may inadvertently introduce bugs.
DO NOT modify existing code that works, even if you absolutely have too1
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:
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.
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 shownReproducible: Always
Steps to Reproduce:Set a filter
Remove all matcheS
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
grepoptimized 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
$ ack "Warning: tracks have been hidden" playlist/
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
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
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.
But if you try to follow
if( m_showOnlyMatches )
noMatch(), it won’t be in this file. Running
$ ack 'noMatch' playlist/
130: void noMatch();
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.
Hmm, now where might
PrettyListViewbe? 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*'
findtakes 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 (
and there you go, open
PrettyListView.cpp and search for
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:
When the user types something,
ProgressiveSearchWidget::filterChangedis being invoked, which is triggering
findsees that no tracks are visible, it emits
notFound. This triggers
ProgressiveSearchWidget::noMatchwhich shows the warning label.
With some more effort, you will also find that
PrettyListView::findis 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,
noMatchnever gets called when tracks are deleted to re-evaluate the situation!
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.
A fundamental constraint that inhibits us is the loose coupling made possible by signals and slots.
As soon as we remove the tracks, we can just call
find()again and we will be done!
All it knows is that it is expected to emit certain signals (
PrettyListViewis unaware of
ProgressiveSearchWidgetand its properties.
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
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
showOnlyMatchesattribute present in the dock, and in the SortFilterProxies. We can,
1. Use PlaylistDock, which has access to the search widgetA roundabout method I did first, involving:
- adding a getter,
- Connecting to the controller’s (
changed()signal, a custom slot in
PrettyListView::find()again with relevant arguments.
2. Make the view store showOnlyMatchesA much simpler three line change.
- Add a
PrettyListView::showOnlyMatches()is called, along with passing along the value to the playlist, we also set
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.
Since this is a minor patch, I could just commit this change. But I’m not going to for two reasons:
$ git diff
diff --git a/src/playlist/view/listview/PrettyListView.cpp b/src/playlist/view/listview/PrettyListView.cpp
index cd650f6..e81d396 100644
@@ -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
@@ -139,6 +139,8 @@ private:
+ bool m_showOnlyMatches;
QList<int> selectedRows() const;
- You don’t have commit access, so I will show you how its done in that case.
- 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.
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.
$ git diff > /tmp/amarok-bugfix-260352.patch
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.
- Use UI hints
- Use debug statements where required.
- Sometimes purposely crashing a program (
assert(0);anyone?) is a great way to see what code-path is being followed.
- Follow the code along, until you can build a mental model.
- Use the mental model to figure out multiple solutions.
- Implement a solution.
- Test it.
- 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.
- except when really required
- 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.