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

List:       kfm-devel
Subject:    Re: [kde-baseapps] dolphin/src/panels/places: Translate bookmark text on update.
From:       Frank Reininghaus <frank78ac () googlemail ! com>
Date:       2014-04-04 12:55:21
Message-ID: CAFoZWWig8_fAp7+iFW+-t_Fr+fa2xVfg6EFteCmTBuK0StSWLQ () mail ! gmail ! com
[Download RAW message or body]

Hi Marco,

2014-04-04 11:44 GMT+02:00 Marco Nelles <marco@maniatek.de>:
> Git commit d22f3b16ab84710f3af8d49b8efded13d18a8af0 by Marco Nelles.
> Committed on 04/04/2014 at 09:41.
> Pushed by marcon into branch 'master'.
> 
> Translate bookmark text on update.
> 
> M  +1    -0    dolphin/src/panels/places/placesitemmodel.cpp
> 
> http://commits.kde.org/kde-baseapps/d22f3b16ab84710f3af8d49b8efded13d18a8af0
> 
> diff --git a/dolphin/src/panels/places/placesitemmodel.cpp \
> b/dolphin/src/panels/places/placesitemmodel.cpp index baa770d..1f05e07 100644
> --- a/dolphin/src/panels/places/placesitemmodel.cpp
> +++ b/dolphin/src/panels/places/placesitemmodel.cpp
> @@ -650,6 +650,7 @@ void PlacesItemModel::updateBookmarks()
> found = true;
> if (newBookmark.metaDataItem("UDI").isEmpty()) {
> item->setBookmark(newBookmark);
> +                        item->setText(i18nc("KFile System Bookmarks", \
> newBookmark.text().toUtf8().data())); }
> break;
> }
> 

first of all, thanks for your code contribution. Please note that code
contributions to Dolphin are usually peer-reviewed at
https://git.reviewboard.kde.org/ . If there is a trivial change, i.e.,
a very small patch which is self-explanatory, such that everyone who
sees the commit immediately sees that it is the right way to do it,
then it may be OK to push it immediately. However, in this case I have
a few questions, and I would appreciate if you could answer them.

1. If this is supposed to fix a problem, why did you push it to master only?

2. Could you describe the problem which this commit is supposed to fix?

3. I do not understand why you added a call to
"item->setText(i18nc(...))" after the setBookmark call, instead of
fixing the setText call inside the function

PlacesItem::setBookmark(const KBookmark& bookmark)?

If I understand correctly, setText is now called twice, first with the
"wrong" value, and then with the "right" value. This looks like the
wrong approach to me, and it might cause trouble in the future, but I
assume that you had a reason for it? Please explain.

Thanks and best regards,
Frank


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