[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: Review Request: Fix description handling in Places entry editor
From:       "Kevin Ottens" <ervin () kde ! org>
Date:       2009-08-15 10:27:03
Message-ID: 20090815102703.9095.74634 () localhost
[Download RAW message or body]


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1312/#review2030
-----------------------------------------------------------


General approach sounds good. I tested the patch and it works great.

Some nitpicking though:
 * please also rename the "description" variables into "label"
 * please comply to the following style for the contructs: "if (foo) {"
   (no space in between parenthesis).

Reason for the second point is that the file has spaces between parenthesis
because it's a straight port of what we had in KDE3, but since KDE4 I'm slowly
migrating it to the same style than kdelibs. The kdelibs coding style doesn't specify
it but the "no whitespace between parenthesis" seems dominant (and the examples on
techbase use that).

- Kevin


On 2009-08-12 23:10:37, Christoph Feck wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1312/
> -----------------------------------------------------------
> 
> (Updated 2009-08-12 23:10:37)
> 
> 
> Review request for Dolphin and kdelibs.
> 
> 
> Summary
> -------
> 
> This fixes a few problems with the "Description" of file places:
> 
> * it says "Description", but you are not going to enter a description, but only a \
>                 (descriptive) label
> * the "Enter description here" is not a clickMessage, but the actual default text
> * due to setting this, automatic derivation of label from URL does not work as \
>                 intended
> * label derivation from URL fails for URLs without path or host (e.g. \
> "ftp://ftp.test.org/", "music://") 
> Please also review string changes.
> 
> 
> This addresses bug 159971.
> https://bugs.kde.org/show_bug.cgi?id=159971
> 
> 
> Diffs
> -----
> 
> /trunk/KDE/kdelibs/kfile/kfileplaceeditdialog.cpp 1010310 
> /trunk/KDE/kdelibs/kfile/kfileplacesview.cpp 1010310 
> 
> Diff: http://reviewboard.kde.org/r/1312/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Christoph
> 
> 


[prev in list] [next in list] [prev in thread] [next in thread]