Please use ReviewBoard

Milian Wolff mail at milianw.de
Sat Jul 27 20:26:49 UTC 2013


On Saturday 27 July 2013 23:13:01 Vlas Puhov wrote:
<snip>

> >Furthermore, please add more information to your commit messages.  This one
> >for example should state that the big hunk you removed is still "available"
> >to the user from within the dialog by using the synchronize button (at
> >least, I hope so)
> 
> Nope, it's not avaliable, not anymore. This as you call it "the big hunk"
> was wrong from the start. If you take a close look you'll notice that this
> hunk didn't let you search in all opened projects, instead it opened just a
> first one. Futhermore there was the stupid check: first
> !m_directory.isEmpty() was checked and then if it's empty:
> !m_directory.startsWith(proj->folder().toLocalFile(). What a hell is
> that??? How can it start with something if it's empty?

First of all, this is just to preselect some directory. The "synchronize" 
button still offers a way to do this as far as I can see, so the feature is 
still there if desired (which is good).

Secondly, you are right that the conditional was a no-op, but it worked 
nontheless. The whole conditional was only entered when m_directory.isEmpty(), 
thus !m_directory.startsWith(anything) always returns true. So the directory 
was correctly preselected.

> >This commit could also be cleaned up:
> >commit 722457052892e3a79f40f82f3f2e005abcd3f91d
> >Author: Vlas Puhov < vlas.puhov at mail.ru >
> >Date:   Sat Jul 20 16:15:00 2013 +0400
> >
> >    BreakpointWidget: Show only file's name, the tooltip shows the full
> >path.
> >
> >BreakpointDelegate::displayText's implementation should not use the ternary
> >operator in my opinion and also needs a comment to clarify what it actually
> >does. Furthermore, please put opening braces of function bodies on their
> >own line, similar to how the other functions are doing it.
> 
>  I don't know if it really needs a comment. I mean, there is only only two
> lines of code. I've seen hunderds lines of code in kdevelop's code base
> without even a single word of explanation about what it does. So, I think
> it's ok. And what is wrong with ternary operator?

Yes, but a bad example is not worth following. I want to establish better 
guidelines to the codebase, to improve that situation. New code should be 
clean and well documented. The fact that old code does not follow this, does 
not mean we should forfeit and continue writing uncommented ugly code.

And the ternary operator is fine, as long as the operation it does is simple 
enough. But from the top of my had e.g. I can't remember the operator 
precendence when I see something like this:

return ( i == -1 ) ? value.toString() : "..." + 
value.toString().right(value.toString().size() - i);

Thus, I suggest using a "proper" if/else branch here. Since it's nicely 
encapsulated in a function, you can rewrite it e.g. as such:

QString BreakpointDelegate::displayText ( const QVariant& value, const 
QLocale& ) const
{
    const QString& path = value.toString();
    int slashIdx = value.toString().lastIndexOf('/');
    if (slashIdx == -1) {
        return path;
    }
    // only return the filename
    return QLatin1String("...") + path.right(path.size() - slashIdx);
}

This is much more readable, is documented "enough" and requires less QVariant 
<-> QString conversions.

> >Even better though would be to remove the whole code there and put that
> >logic into the model, instead of inside the delegate. The code for that
> >resides in BreakPoint::data, where you need to special-case the
> >DisplayRole then to only return the filename. There it's also much
> >simpler, as you have the url at hand and can just use m_url.fileName()
> >instead of string parsing
> 
> At first I thought about it too. But there is a little problem:
> BreakpointWidget is not the only one who retrieves data by DisplayRole.
> Actually it's the right thing for breakpoint model to return the full path
> instead of a file's name(it's more useful, for example breakpoint insertion
> uses the full path).

Can you elaborate on this please? Where else is this model being used, where 
one wants to show the full path instead of just the ellided filename part?

Imo, showing it everywhere in the same way also leads to increased 
consistency, which I think is a good thing - esp. in a complicated program 
such as KDevelop.

Note that if there is code which actually _requires_ the full path, then it 
should not use DisplayRole to query for it, but rather the custom 
LocationRole.

> >PS: I hope you see this as constructive criticism. I really don't want to
> >annoy you or drive you away from the project - quite the contrary. I just
> >hope to teach you some more tricks to improve the quality of your
> >contributions even more. Using something like reviewboard is also how one
> >learns a lot, by taking in the suggestions from others.
> >
> >Cheers! Rock on!
> 
> PS: I'd love to use the reviewboard. But for me it looks like there is no
> one who is proficient enough in code base and at the same time interested
> in reviewing patches. For example,  I posted a couple of patches : 1. Fix
> crash in disassembleWidged. It was on the reviewboard for 2 weeks - not a
> single comment. 2. Fix incorect debug session ending - 2 weeks too. Yeah
> Aleix wrote something,  but he didn't tell(no one did!) that I was patching
> in a wrong place. 3. Fix debug/execute actions avaliable while none of
> projets are opened - was for review about 2 weeks too. Yes I know I was
> completely wrong about it, but afterwards I wasn't disabling those actions
> anymore, I just speeded a project's loading up, but still no comments. So,
> the question is why? What is the meaning in posting patches on reviewboard
> if there is no one to review it?

Yes, these are very valid points. And let me apologize for not attending to 
reviewrequests in a timely manner, but I was simply overworked with 
university. That being said, going through review initially is always a good 
thing. If you see that noone responds after, say, a week or so just go ahead 
and commit the stuff. Then it's not my place to complain and I can still do 
what I did here, i.e. post-review.

But since I finally have more time again, I do plan to attend to our review 
requests more again. I hope that others do the same. And I rather have a 
"clean" history with patches that don't require much work after being 
committed initially. This is what you gain from reviewboard as a project. You 
as a contributor get more feedback on code which is very educative and leads 
to better code.

Cheers
-- 
Milian Wolff
mail at milianw.de
http://milianw.de


More information about the KDevelop-devel mailing list