Code review comment for lp:~manchicken/kubuntu-debug-installer/kubuntu-debug-installer

Yeah I don't think that decorator thing makes sense at all, but that's a library thing and libqapts fault really. Why would every application using qapt have to carry a verbatim code copy with strings around?

As for style... here comes the style PITA police :P

Please note that we use 4 space indent in the source code. Also please review your added includes, just from looking at the diff it seems like they are not actually all necessary.

> m_txn = NULL;

By convention we use 0 in Qt software. No clue what a txn is, perhaps a more descriptive name would be good.

> QString headerText = i18n("<b>Adding debug symbol sources</b>");

please have a look at i18nc [1] and in particular you'd want to get rid of the <b> and use <title> for example [2]. this applies to the entire diff.

also, why not simply merge the i18nc() into the next line
m_progress.setLabelText(i18nc(...));

and on that note I would make m_progress a pointer to get it out of the header for compile time's sake and to reduce overall startup workload

> hcRepoList

same as with m_txn, no clue what hc stands for....

> QString(DDEB_REPO_FILE)

constructing using QString::fromLatin1(..) is faster.

> int sourceCount = sourceList.entries().count();

I reckon this value is const, yet the code doesn't say so.

> foreach (QApt::SourceEntry repo, hcRepoList)

Not sure whether that is possible, but generally speaking that should be const QApt::SourceEntry &repo to avoid pointless copying.

By convention this should be foreach (...) { ... i.e. opening angle braces on the same line [3]

> m_srcUpdated

variable name again, I guess this should be something like m_srcsListUpdated

> this->find();

convention again, no this unless necessary.

> int m_srcCount;

unused it seems

> add_definitions(-DKDE_DEFAULT_DEBUG_AREA=8675309)

I don't think we'd want a set debug area (seems unnecessary and is only advised for libs/plugins), at any rate we'd not want a static debug area but register one dynamically [4]

> headerText = i18n("<b>Downloading Packages</b>");
like above and indeed it seems like not using headertext to begin with would be a good idea

[1] http://techbase.kde.org/Development/Tutorials/Localization/i18n#Adding_Context_with_i18nc.28.29
[2] http://techbase.kde.org/Development/Tutorials/Localization/i18n_Semantics
[3] https://wiki.kubuntu.org/Kubuntu/Specs/MaverickCodestylePolicy
[4] http://api.kde.org/4.x-api/kdelibs-apidocs/kdecore/html/classKDebug.html#a40c55b7ae17e62f18fa5e44349496e33