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

Doh. I'll be making many of the changes you mentioned. I'll make them
Saturday. Do we have the ability to test now so we can see if it works like
we want it to? I want to get as much lined up for my hackathon Saturday as
I can.
On Jul 24, 2013 4:33 PM, "Harald Sitter" <email address hidden> wrote:

> 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
> --
>
> https://code.launchpad.net/~manchicken/kubuntu-debug-installer/kubuntu-debug-installer/+merge/176318
> You are the owner of
> lp:~manchicken/kubuntu-debug-installer/kubuntu-debug-installer.
>