What’s coming in KDE Telepathy 0.4

It’s been a long time since I’ve blogged about KDE Telepathy, we’ve got so much going on that we’ve had to delay the release so we can get everything perfect. Here’s a sneak preview of all the new stuff in this release.

Log Viewer

Assuming you have had an application called TpLogger installed all your messages have been logged, only without a way to view them. Now in 0.4 I wrote a log viewer to see them all.

Merging Kopete logs is a work in progress, but it is being thought about (but probably not for 0.4).

Chat Plasmoid

As Lasath Fernando blogged previously we have a plasmoid for handling chats.

Since that blog Lasath and I have changed the behaviour slightly so that all new incoming chats open directly in this plasmoid (if installed) then there’s a button to pop out to the full blown text interface if needed. It acts as a far more advanced notification interface, through which you can reply. It’s very much like the GMail web interface, but integrating with your desktop. It also contains several fixes since it was first announced.

(font colours need to be sorted out clearly)

Contact List Plasmoid

This is a new addition to our features, this was originally started as a small part of a GSOC project by Francesco Nwokeka last year. It’s since been tidied up to a proper Declarative Plugin and really simple QML.

What we’re doing with all our new plasmoids is buliding nice-reusable declarative plugins and keeping our plasmoids are very desktop oriented. Plasma Active can then redo this in a very touch oriented way only redoing the GUI layer.

We still really need some design inspiration on ours though.

KRunner integration

Dan Vratil has written a KRunner plugin for KDE Telepathy allowing you to quickly search for your contacts and start chats, it’s now passed our rather rigourous code reviews and heading into our repositories!

Fixes galore

Whilst we’ve been developing these new features, we’ve also been fixing little issues elsewhere; bugs and crashes to the small UI details. Daniele Domenichelli has made some massive improvements to the Adium Theme support in the main text view, amongst all sorts of other fixes.

Video support?

All the pieces towards working video are coming together thanks to the amazing work of George Kiagiadakis, so hopefully we can get video in this release. Don’t count on it, and it’s not quite ready for testing yet.

So when is it coming?

The release was originally scheduled for late March, however to get all these features perfect we are delaying the release slightly. You can see in the screenshots things are a bit rough round the edges so it’ll be ready when it’s ready, but we’re aiming for around April/May.

Can I help?

Absolutely! Attracting attention is the only reason I write these posts :). We have a load of tasks that need doing, and not just coding. The contact list applet above needs a lot of designing, a mockup in photoshop/equivalent would be amazing. We still need more testers who report and triage bugs, and there’s jobs for everyone. Plus we’re a cool group to hang out with, honest.

Head down to #kde-telepathy on Freenode to find us.

How else can I help?

Contribute to our beer fund! This will be spent at Akademy and future sprints and split across all the members contributing to the project to buy us all a well deserved beer.

We all work on this purely in our free time with little compensation so it’s things like this which keep us going.





A Review of Code Reviews

When I first started hacking on KDE-Telepathy the original manager was very insistent that all code should be reviewed before merging. Initially it was something that I considered a complete waste of time but has since grown on me as a really important step in development.

Recently, I’ve been getting involved with following another very prominent important KDE project and I’ve been a bit concerned by the “hack and slash” attitude that I see in that IRC channel with commits coming from all over the place, with very few of them looking like they’re actually reviewed.

What’s good about code reviews

Code reviews obviously result in “better code”, but I think there is a tendency to think it’s only for people making their first patches. This attitude is utter nonsense.

Many eyes make all bugs shallow

Also known as “Linus’s law”, the more people who look at a piece of code the more likely you are to find a bug. It’s a lot easier to fix a potential bug at review stage than find it after it’s deployed. Everyone makes mistakes, it’s the best time to find them.

Shared responsibility

This is tied into the above. Let’s say Martin submits a patch which I review which turns out to be crashy. Who’s fault is it? It’s his fault for the dodgy code but it’s just as much my fault for not spotting it. It’s also everyone else’s fault for being too lazy to review the patch. I think code reviews take away some of the “blame game” when it comes to bugs.

Code habits

This is more for the people starting out, but it’s a good time to spot the bad habits of missing “consts”, bad whitespace, not checking for null pointers etc. It’s better to point these out here and “block the commit” publicly so the submitter learns than to simply fix it for them.

Usability issues

One of the main things we’ve gained from reviewboard is fixing situations where the code is perfectly fine, but what it’s doing isn’t. At review is a good time to make sure that not only the code is correct but that the user experience is the best it can be. This means checking any strings presented to the user, we’ve had huge arguments over the tiniest of details “Alias” vs “Nickname” and checking all forms are visually well laid out and arguing over each pixel in a paintEvent. It can be frustrating at the time, but I reckon we get a better product for it.

Attaching screenshots help. Occasionally we’ve done UI testing just with a screenshot. I’ve sent someone else a link and said “explain to me what you think this button does”. If they’re unsure, the text/layout needs changing.

Keeping code simple

A good coder can write complex code, an expert coder can achieve the same complex tasks in a really simple way. It seems counter-intuitive but the better programmer will be writing simpler code as you learn how to design things better. I have rejected perfectly working patches saying “rewrite this part so it’s easier to read” or even a simple “comment this”. If it takes me several minutes to understand a patch at review time where I’m only looking a small section and have a description it’s going to be a real pain for the next coder who wants to edit some code around this or fix it.

I’ve heard rumours of code in KMail with a comment that says “//Don’t touch this unless you are ….”. That’s a horrific state to be in, and a lesson we should all learn from to avoid. It’s far easier to sort this at review time when the author can remember how the code works.

A second set of eyes may be able to find a cleaner way to do it, it’s often easier to find the best way to solve a problem once you’ve first drafted something.

More awareness into goings on in the project

If I have to read the code well enough to be able to review it properly I’ll have a better understanding of how everyone else’s code works, this is very useful when you end up having to add a feature somewhere that relates to their code.

What sucks, and how to avoid it

ReviewBoard is sooo slow to use

Use post-review, It does work. However the version shipped by your distro may be too old for the version of reviewboard we have. Follow the instructions to installing post-review on the reviewboard website.

It’s still slow

The post-review flags make things a lot easier, –guess-description is pretty good, it fills everything in based on your git commit logs.

Make an alias for reviews you type often. This is mine for all KDE Telepathy reviews.

alias tpreview=’post-review –parent=master –tracking-branch=anonupstream/master –guess-summary –guess-description –target-groups=telepathy -o

It’s still really slow

On #kde-telepathy you’ll constantly see links to pastebin being sent about. We use this for small patches, it’s not as formal as using review board but it gets the same job done.

Another speed-up trick we do is to point out changes but say “ship it!” anyway. If only trivial fixes are needed, there’s not always a lot of point making them submit a new patch with it fixed.

Pushing a git branch is easier than reviewboard

Use that then, the tool isn’t important, what’s important is making sure reviews happen.

It’s pointless for small patches

As with anything common sense is needed. I once got bugged on IRC “d_ed, d_ed can you review this” and showed me a change from “#include <QLabel>” to “#include <QtGui/QLabel>”. Be pragmatic.

It blocks development

No it doesn’t. As soon as you submit a review, create a new branch, carry on working. If you think reviews block development it probably means you need to improve your git-fu.

It’s disheartening to see your code ripped apart

Absolutely, I once had a 2000 line patch that I wrote on Christmas Day torn apart by Martin who proved part of it was pointless and cut it down to about 400 lines, removing this really clever feature I had. It’s better code for it though. Deal with it.

Reviews sit there for ages untouched

This is the biggest issue we face, some difficult patches no-one wants to review, and in such a small team like ours people are often simply away/busy. Having a patch stagnate is frustrating for everyone.

I also found a lot of the new people in the project felt “unworthy” to do reviews, which is silly. If I submit a difficult patch I may be waiting for a specific person to review it, but that doesn’t mean any other reviews aren’t highly valuable.

I’m not sure of other good ways to solve this.

It’s hard to review large amounts of pre-existing code

Yeah 🙁 I have no idea how to solve this. We got this even for the few small apps in KDE TP, as every new app starts off as “just prototype code” in some scratch repo somewhere, then end up getting merged.

Summary

  • Everyone should have their code reviewed. If you think you’re so good that you don’t need reviews, you’re mistaken.
  • Reviews are the perfect place to discuss UI isssues. Think about and challenge every user facing word, every UI decision.
  • Make that easier, attach screenshots
  • Use the appropriate tools for the job, for us this isn’t always reviewboard for small commits.
  • Everyone should be involved in reviewing, don’t leave it for someone else or people will be put off submitting things for review.