From kfm-devel  Fri Apr 04 13:44:01 2014
From: Marco <marco () maniatek ! de>
Date: Fri, 04 Apr 2014 13:44:01 +0000
To: kfm-devel
Subject: Re: [kde-baseapps] dolphin/src/panels/places: Translate bookmark text on update.
Message-Id: <69cff4d6fa55fd11d7a0061fe8ec6c04 () marco ! maniatek ! de>
X-MARC-Message: https://marc.info/?l=kfm-devel&m=139662002316140

Hi,


thanks for your feedback!

> Please note that code
> contributions to Dolphin are usually peer-reviewed at
> https://git.reviewboard.kde.org/

Okay, I'll respect this in future of course.

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

My fail. This patch, if this is the right way to fix the problem, could
also be pushed to other branches.

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

Environment: KDE with german locales (I suppose all other locales are 
also affected).

Problem: If you start many instances of dolphin the same time and end 
one of them,
the list of bookmarks in the places panel will become english in all 
other running
dolphin instances.

> 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

Yes, you're right! This was a working and fast solution satisfied
my customer.

So, fixing setText() in placesitem.cpp is the correct approach to fix 
the problem?

I'll have a deeper look at the code within the next few days and going 
to create
a new patch with respect to git.reviewboard.kde.org.

Please revert my commit and sorry for any mistakes and circumstances. 
I'm not familiar
with the KDE contribution and qs processes. So again, many thanks for 
your feedback.


Best regards,

Marco


Am 2014-04-04 14:55, schrieb Frank Reininghaus:
> 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