amarok/amarok/HACKING

Hacking on Amarok
-----------------

Please respect these guidelines when coding for Amarok, thanks!

* Where this document isn't clear, refer to Amarok code.


This C++ FAQ is a life saver in many situations, so you want to keep it handy:

  http://www.parashift.com/c++-faq-lite/


Formatting
----------
* Spaces, not tabs
* Indentation is 4 spaces
* Lines should be limited to 90 characters
* Spaces between brackets and argument functions
* For pointer and reference variable declarations put a space between the type
  and the * or & and no space before the variable name.
* For if, else, while and similar statements put the brackets on the next line,
  although brackets are not needed for single statements.
* Function and class definitions have their brackets on separate lines
* A function implementation's return type is on its own line.
* amarok.h contains some helpful macros, foreach and foreachType.  Use them,
  they improve coding style and readability.

Example:

 | bool 
 | MyClass::myMethod( QPtrList<QListViewItem> items, const QString &name )
 | {
 |     if( items.isEmpty() )
 |         return false;
 |
 |     foreachType( QPtrList<QListViewItem>, items )
 |     {
 |         (*it)->setText( 0, name );
 |         debug() << "Setting item name: " << name << endl;
 |     }
 | }

Header includes should be listed in the following order:
    - Amarok includes
    - KDE includes
    - Qt includes

They should also be sorted alphabetically, for ease of locating them.  A small comment
if applicable is also helpful.

Includes in a header file should be kept to the absolute minimum, as to keep compile times
low. This can be achieved by using "forward declarations" instead of includes, like
"class QListView;" Forward declarations work for pointers and const references.

TIP:
Kate/KDevelop users can sort the headers automatically. Select the lines you want to sort,
then Tools -> Filter Selection Through Command -> "sort".


Example:

 | #include "amarok.h"
 | #include "debug.h"
 | #include "playlist.h"
 |
 | #include "kdialogbase.h"    //baseclass
 | #include "kpushbutton.h"    //see function...
 |
 | #include "qlistviewitem.h"
 | #include "qwidget.h"


Comments
--------
Comment your code. Don't comment what the code does, comment on the purpose of the code. It's
good for others reading your code, and ultimately it's good for you too.

Comments are essential when adding a strange hack, like the following example:

 | /** Due to xine-lib, we have to make TDEProcess close all fds, otherwise we get "device is busy" messages
 |   * Used by AmarokProcIO and AmarokProcess, exploiting commSetupDoneC(), a virtual method that
 |   * happens to be called in the forked process
 |   * See bug #103750 for more information.
 |   */
 | class AmarokProcIO : public KProcIO 
 | {
 |     public:
 |     virtual int commSetupDoneC() {
 |         const int i = KProcIO::commSetupDoneC();
 |         Amarok::closeOpenFiles(KProcIO::out[0],KProcIO::in[0],KProcIO::err[0]);
 |         return i;
 |     }
 | };


For headers, use the Doxygen syntax. See: http://www.stack.nl/~dimitri/doxygen/

Example:

 | /**
 |  * Start playback.
 |  * @param offset Start playing at @p msec position.
 |  * @return True for success.
 |  */
 | virtual bool play( uint offset = 0 ) = 0;


Header Formatting
-----------------
General rules apply here.  Please keep header function definitions aligned nicely,
if possible.  It helps greatly when looking through the code.  Sorted methods,
either by name or by their function (ie, group all related methods together) is
great too.


 | #ifndef AMAROK_QUEUEMANAGER_H
 | #define AMAROK_QUEUEMANAGER_H

 | class QueueList : public TDEListView
 | {
 |         Q_OBJECT
 |
 |     public:
 |         Queuelist( QWidget *parent, const char *name = 0 );
 |         ~QueueList() {};
 |
 |     public slots:
 |         void    moveSelectedUp();
 |         void    moveSelectedDown();
 | };

#endif /* AMAROK_QUEUEMANAGER_H */


0 vs NULL
---------
The use of 0 to express a null pointer is preferred over the use of NULL.
0 is not a magic value, it's the defined value of the null pointer in C++.
NULL, on the other hand, is a preprocessor directive (#define) and not only is
it more typing than '0' but preprocessor directives are less elegant.

 |     SomeClass *instance = 0;


Const Correctness
-----------------
Try to keep your code const correct. Declare methods const if they don't mutate the object,
and use const variables. It improves safety, and also makes it easier to understand the code.

See: http://www.parashift.com/c++-faq-lite/const-correctness.html


Example:

 | bool 
 | MyClass::isValidFile( const QString& path ) const
 | {
 |     const bool valid = QFile::exist( path );
 |
 |     return valid;
 | }


Debugging
---------
debug.h contains some handy functions for our debug console output.
Please use them instead of kdDebug().

Usage:

 | #include "debug.h"
 |
 | debug()   << "Something is happening" << endl;
 | warning() << "Something bad may happen" << endl;
 | error()   << "Something bad did happen!" << endl;

Additionally, there are some macros for debugging functions:

DEBUG_BLOCK
DEBUG_FUNC_INFO
DEBUG_LINE_INFO
DEBUG_INDENT
DEBUG_UNINDENT

AMAROK_NOTIMPLEMENTED
AMAROK_DEPRECATED

threadweaver.h has two additional macros:
DEBUG_THREAD_FUNC_INFO outputs the memory address of the current QThread or 'none'
    if its the original GUI thread.
SHOULD_BE_GUI outputs a warning message if it occurs in a thread that isn't in
    the original "GUI Thread", otherwise it is silent. Useful for documenting
    functions and to prevent problems in the future.


Usage of Amarok::config()
-------------------------
We provide this method for convenience, but it is important to use it properly. By
inspection, we can see that we may produce very obscure bugs in the wrong case:

 | TDEConfig 
 | *config( const QString &group )
 | {
 |    //Slightly more useful config() that allows setting the group simultaneously
 |    kapp->config()->setGroup( group );
 |    return kapp->config();
 | }

Take the following example:
 
 | void
 | f1()
 | {
 |    TDEConfig *config = Amarok::config( "Group 2" );
 |    config->writeEntry( "Group 2 Variable", true );
 | }
 |
 | void
 | doStuff()
 | {
 |   TDEConfig *config = Amarok::config( "Group 1" );
 |   f1();
 |   config->writeEntry( "Group 1 Variable", true );
 | }

We would expect the following results:

 | [Group 1]
 | Group 1 Variable = true
 |
 | [Group 2]
 | Group 2 Variable = true

However because the config group is changed before writing the entry:
 | [Group 1]
 |
 | [Group 2]
 | Group 1 Variable = true
 | Group 2 Variable = true

Which is clearly incorrect. And hard to see when your wondering why f1() is not
working. So do not store a value of Amarok::config, make it a habit to just 
always call writeEntry or readEntry directly.

Correct:
| amarok::config( "Group 1" )->writeEntry( "Group 1 Variable", true );


Errors & Asserts
----------------
*Never use assert() or fatal(). There must be a better option than crashing a user's
application (its not uncommon for end-users to have debugging enabled).

*KMessageBox is fine to use to prompt the user, but do not use it to display errors
or informational messages. Instead, KDE::StatusBar has a few handy methods. Refer to
amarok/src/statusbar/statusBarBase.h


Copyright
---------
To comply with the GPL, add your name, email address & the year to the top of any file
that you edit. If you bring in code or files from elsewhere, make sure its
GPL-compatible and to put the authors name, email & copyright year to the top of
those files.


Thanks, now have fun!
  -- the Amarok developers