D17617: Display error instead of silently failing when asked to create folder that already exists

[Hazem Salem]

Codezela added a comment.


  can we change enter different name to just rename
  for simplicity

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17617

To: ngraham, #dolphin
Cc: Codezela, kde-frameworks-devel, michaelh, ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[Nathaniel Graham]

ngraham edited the test plan for this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17617

To: ngraham, #dolphin
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17617: Display error instead of silently failing when asked to create folder that already exists

[Nathaniel Graham]

ngraham created this revision.
ngraham added a reviewer: Dolphin.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
ngraham requested review of this revision.

REVISION SUMMARY
  BUG: 400423
  FIXED-IN: 5.54

TEST PLAN
  1. `mkdir ~/test`
  2. In Dolphin, try to create `~/test`: folder not created
  3. In Dolphin, try to create `~/test/foo`: folder created
  4. In Dolphin, try to create `~/testo`: folder created

REPOSITORY
  R241 KIO

BRANCH
  dont-try-to-create-existing-dir (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D17617

AFFECTED FILES
  src/filewidgets/knewfilemenu.cpp

To: ngraham, #dolphin
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17596: [KDirOperator] Allow renaming files from the context menu

[Markus Slopianka]

markuss added a comment.


  In D17596#377668 , @ngraham wrote:
  
  > That's currently an option in Dolphin. Maybe we should just read that 
setting to see if we should do it here too?
  
  
  Sounds like a good idea.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17596

To: ngraham, #frameworks, #dolphin
Cc: emateli, elvisangelaccio, markuss, dhaumann, kde-frameworks-devel, 
michaelh, ngraham, bruns

D17595: Upstream Dolphin's file rename dialog

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> ngraham wrote in renamefiledialog.h:5
> The KIO repo's COPYING.LIB file says LGPL 2.1. Is that a mismatch?

> The KIO repo's COPYING.LIB file says LGPL 2.1. Is that a mismatch?

Considering this new code is GPL: yes, it's a mismatch, and it would make the 
whole kiowidgets as GPL.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17595

To: ngraham, #frameworks, #dolphin, broulik
Cc: dhaumann, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D17595: Upstream Dolphin's file rename dialog

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> dhaumann wrote in renamefiledialog.h:5
> Is the rest of KIO LGPL or GPL? If we have a license mismatch, we'd have to 
> ask the authors for an OK to relicense.

The KIO repo's COPYING.LIB file says LGPL 2.1. Is that a mismatch?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17595

To: ngraham, #frameworks, #dolphin, broulik
Cc: dhaumann, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D17596: [KDirOperator] Allow renaming files from the context menu

[Nathaniel Graham]

ngraham added a comment.


  That's currently an option in Dolphin. Maybe we should just read that setting 
to see if we should do it here too?
  
  Either way I'd prefer to do that in a separate patch.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17596

To: ngraham, #frameworks, #dolphin
Cc: emateli, elvisangelaccio, markuss, dhaumann, kde-frameworks-devel, 
michaelh, ngraham, bruns

D17596: [KDirOperator] Allow renaming files from the context menu

[Emirald Mateli]

emateli added a comment.


  IMO it would be great if this would just rename inline rather than opening a 
dialog.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17596

To: ngraham, #frameworks, #dolphin
Cc: emateli, elvisangelaccio, markuss, dhaumann, kde-frameworks-devel, 
michaelh, ngraham, bruns

D17609: KTextEditor: Add action for static word wrap

[Andres Betts]

abetts added a comment.


  +1

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17609

To: loh.tar, #ktexteditor, #vdg
Cc: abetts, kwrite-devel, kde-frameworks-devel, #ktexteditor, hase, michaelh, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D17609: KTextEditor: Add action for static word wrap

[loh tar]

loh.tar created this revision.
loh.tar added reviewers: KTextEditor, VDG.
Herald added projects: Kate, Frameworks.
Herald added subscribers: kde-frameworks-devel, kwrite-devel.
loh.tar requested review of this revision.

REVISION SUMMARY
  - Move "Show Static Word Wrap Marker" code some lines up to group related 
actions
  
  BUG:141946

TEST PLAN
  F6478656: 1544896348.png 

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17609

AFFECTED FILES
  src/data/katepart5ui.rc
  src/view/kateview.cpp

To: loh.tar, #ktexteditor, #vdg
Cc: kwrite-devel, kde-frameworks-devel, #ktexteditor, hase, michaelh, ngraham, 
bruns, demsking, cullmann, sars, dhaumann

D17609: KTextEditor: Add action for static word wrap

[loh tar]

loh.tar added a comment.


  - Not well tested
  - Slot code may not fit coding style but looks not bad to me
  - No Ampersand & short cut set as done by other actions

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D17609

To: loh.tar, #ktexteditor, #vdg
Cc: kwrite-devel, kde-frameworks-devel, #ktexteditor, hase, michaelh, ngraham, 
bruns, demsking, cullmann, sars, dhaumann

D17596: [KDirOperator] Allow renaming files from the context menu

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> dhaumann wrote in kdiroperator.cpp:907-909
> Does dialog->open() block? If so, do we have to new the dialog at all? (Only 
> reason I can think of is some DBus crash, see 
> https://blogs.kde.org/2009/03/26/how-crash-almost-every-qtkde-application-and-how-fix-it-0
>  but that would require a better fix).

Nope, `dialog->open()` does not block.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17596

To: ngraham, #frameworks, #dolphin
Cc: elvisangelaccio, markuss, dhaumann, kde-frameworks-devel, michaelh, 
ngraham, bruns

D17596: [KDirOperator] Allow renaming files from the context menu

[Dominik Haumann]

dhaumann added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in kdiroperator.cpp:907-909
> I think we should just use `dialog->open()` here. I see you've copied these 3 
> lines from Dolphin, but we should also fix it there (the rename dialog should 
> really be modal).

Does dialog->open() block? If so, do we have to new the dialog at all? (Only 
reason I can think of is some DBus crash, see 
https://blogs.kde.org/2009/03/26/how-crash-almost-every-qtkde-application-and-how-fix-it-0
 but that would require a better fix).

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17596

To: ngraham, #frameworks, #dolphin
Cc: elvisangelaccio, markuss, dhaumann, kde-frameworks-devel, michaelh, 
ngraham, bruns

D17596: [KDirOperator] Allow renaming files from the context menu

[Nathaniel Graham]

ngraham added a comment.


  In D17596#377423 , @markuss wrote:
  
  > Before the feature was removed, the window had all file operations, incl. 
move to trash and IIRC even the ability to cut/copy/paste and move (via D'n'D) 
files.
  
  
  Move to Trash is already there. Cut/Copy/Paste shouldn't be too hard to 
add--in another patch of course. :)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17596

To: ngraham, #frameworks, #dolphin
Cc: elvisangelaccio, markuss, dhaumann, kde-frameworks-devel, michaelh, 
ngraham, bruns

D17596: [KDirOperator] Allow renaming files from the context menu

[Nathaniel Graham]

ngraham updated this revision to Diff 47624.
ngraham marked 2 inline comments as done.
ngraham added a comment.


  Address review comments

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17596?vs=47607&id=47624

BRANCH
  arcpatch-D17596

REVISION DETAIL
  https://phabricator.kde.org/D17596

AFFECTED FILES
  src/filewidgets/kdiroperator.cpp
  src/filewidgets/kdiroperator.h

To: ngraham, #frameworks, #dolphin
Cc: elvisangelaccio, markuss, dhaumann, kde-frameworks-devel, michaelh, 
ngraham, bruns

D17596: [KDirOperator] Allow renaming files from the context menu

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> kdiroperator.cpp:907-909
> +dialog->show();
> +dialog->raise();
> +dialog->activateWindow();

I think we should just use `dialog->open()` here. I see you've copied these 3 
lines from Dolphin, but we should also fix it there (the rename dialog should 
really be modal).

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17596

To: ngraham, #frameworks, #dolphin
Cc: elvisangelaccio, markuss, dhaumann, kde-frameworks-devel, michaelh, 
ngraham, bruns

D10446: Add KLanguageName

[Harald Sitter]

sitter added a comment.


  In D10446#377355 , @aacid wrote:
  
  > But you end up repeating that in lots of places (which we should there's 
lots of places that suffer from trying to guess a language name at this point, 
and all of them went the bad way one way or another). If you want to give some 
random potential user more flexibility i'm fine with that, add some flags, but 
i want the "give me the best you can do" possibility to still work. Maybe we 
should even never return an empty string and worst case scenario return code 
back.
  
  
  I am not too invested in the use case. For all I care we can leave it as it 
is and should the no-fallback usecase actually get requested we can simply add 
a three-argument variant of the function without default value for KF5 and 
reshuffle the functions for KF6.
  
  The diff as it is right now is good IMO

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D10446

To: aacid
Cc: hein, kde-frameworks-devel, sitter, markg, apol, michaelh, ngraham, bruns

D10446: Add KLanguageName

[Harald Sitter]

sitter updated this revision to Diff 47623.
sitter added a subscriber: hein.
sitter added a comment.


  iterating the diff a bit as mentioned in a comment.
  
  - new unit test which covers all likely scenarios (I think). this uses 
fixtures and doesn't require anything to be installed etc.
  - the specific func now calls the generic to resolve into current-language. 
this requires some string mangling as qlocale apparently has no way to get the 
ISO639-1 string directly
  - some refinement on the english-check resolution: now returns the kconfig 
resolved name iff it is different from english and it is not english 
(previously this was a bit bugged for english itself)
  - some refinement on the qlocale fallback: now returns immediately when a 
final result is found (simplifies code a bit)
  - a failed kconfig resolution (for whatever reason) now always continues 
onwards into the qlocale fallback. this effectively separates the kconfig 
portion entirely from the qlocale portion for ease of reading
  
  as mentioned in an earlier comment I am not too sure about giving out 
languageToString names and would rather have it always give out 
nativeLanguageNames. I am somewhat indifferent on the issue though, it's just a 
gut feeling is all.
  
  I should also note that I had a quick look at systemsettings and it also does 
exhaustive poking of QLocale to come up with reasonable prettynames for locales 
(and then fails in part). Perhaps @hein could take a look at this diff and see 
if it could be useful for the language KCM and/or give some thoughts on what 
may be needed to make that happen. For the record though: since KLanguageName 
exclusively deals with languages and the KCM AFAIK deals with locales there is 
no 100% overlap; perhaps there should be though.

REPOSITORY
  R265 KConfigWidgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10446?vs=26943&id=47623

BRANCH
  arcpatch-D10446

REVISION DETAIL
  https://phabricator.kde.org/D10446

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kf5_entry_data/locale/ca/kf5_entry.desktop
  autotests/kf5_entry_data/locale/de/kf5_entry.desktop
  autotests/kf5_entry_data/locale/en_US/kf5_entry.desktop
  autotests/kf5_entry_data/locale/fr/kf5_entry.desktop
  autotests/kf5_entry_data/locale/pt/kf5_entry.desktop
  autotests/klanguagenametest.cpp
  src/CMakeLists.txt
  src/klanguagename.cpp
  src/klanguagename.h

To: aacid
Cc: hein, kde-frameworks-devel, sitter, markg, apol, michaelh, ngraham, bruns

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Elvis Angelaccio]

elvisangelaccio added a comment.


  In D10742#377356 , @jtamate wrote:
  
  > Let's see if this assert/crash can be avoided without reverting all the 
patch. Any help is welcome.
  
  
  Does this valgrind log help?
  
Invalid read of size 8
   at 0x56B5628: 
QSharedDataPointer::QSharedDataPointer(QSharedDataPointer
 const&) (qshareddata.h:92)
   by 0x56AF5D6: KFileItem::KFileItem(KFileItem const&) (kfileitem.h:47)
   by 0x575FDFB: KCoreDirListerCache::renameDir(QUrl const&, QUrl const&) 
(kcoredirlister.cpp:1576)
   by 0x575B041: KCoreDirListerCache::slotFileRenamed(QString const&, 
QString const&, QString const&) (kcoredirlister.cpp:969)
   by 0x577BF39: QtPrivate::FunctorCall, 
QtPrivate::List, void, void 
(KCoreDirListerCache::*)(QString const&, QString const&, QString 
const&)>::call(void (KCoreDirListerCache::*)(QString const&, QString const&, 
QString const&), KCoreDirListerCache*, void**) (qobjectdefs_impl.h:152)
   by 0x577A630: void QtPrivate::FunctionPointer::call, 
void>(void (KCoreDirListerCache::*)(QString const&, QString const&, QString 
const&), KCoreDirListerCache*, void**) (qobjectdefs_impl.h:185)
   by 0x5777250: QtPrivate::QSlotObject, 
void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) 
(qobjectdefs_impl.h:414)
   by 0x75C93BF: QMetaObject::activate(QObject*, int, int, void**) (in 
/usr/lib/libQt5Core.so.5.12.0)
   by 0x5798ED3: 
OrgKdeKDirNotifyInterface::FileRenamedWithLocalPath(QString const&, QString 
const&, QString const&) (moc_kdirnotify.cpp:224)
   by 0x579889D: OrgKdeKDirNotifyInterface::qt_static_metacall(QObject*, 
QMetaObject::Call, int, void**) (moc_kdirnotify.cpp:103)
   by 0x5798DAB: OrgKdeKDirNotifyInterface::qt_metacall(QMetaObject::Call, 
int, void**) (moc_kdirnotify.cpp:203)
   by 0x72B19EE: ??? (in /usr/lib/libQt5DBus.so.5.12.0)
 Address 0x11350758 is 72 bytes inside a block of size 128 free'd
   at 0x4839D7B: realloc (vg_replace_malloc.c:826)
   by 0x7439992: QListData::realloc_grow(int) (in 
/usr/lib/libQt5Core.so.5.12.0)
   by 0x7439A3B: QListData::append(int) (in /usr/lib/libQt5Core.so.5.12.0)
   by 0x576BE43: QList::insert(QList::iterator, 
KFileItem const&) (qlist.h:521)
   by 0x576B1EC: KCoreDirListerCache::DirItem::insert(KFileItem const&) 
(kcoredirlister_p.h:422)
   by 0x576AC8A: KCoreDirListerCache::reinsert(KFileItem const&, QUrl 
const&) (kcoredirlister_p.h:310)
   by 0x5760053: KCoreDirListerCache::renameDir(QUrl const&, QUrl const&) 
(kcoredirlister.cpp:1584)
   by 0x575B041: KCoreDirListerCache::slotFileRenamed(QString const&, 
QString const&, QString const&) (kcoredirlister.cpp:969)
   by 0x577BF39: QtPrivate::FunctorCall, 
QtPrivate::List, void, void 
(KCoreDirListerCache::*)(QString const&, QString const&, QString 
const&)>::call(void (KCoreDirListerCache::*)(QString const&, QString const&, 
QString const&), KCoreDirListerCache*, void**) (qobjectdefs_impl.h:152)
   by 0x577A630: void QtPrivate::FunctionPointer::call, 
void>(void (KCoreDirListerCache::*)(QString const&, QString const&, QString 
const&), KCoreDirListerCache*, void**) (qobjectdefs_impl.h:185)
   by 0x5777250: QtPrivate::QSlotObject, 
void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) 
(qobjectdefs_impl.h:414)
   by 0x75C93BF: QMetaObject::activate(QObject*, int, int, void**) (in 
/usr/lib/libQt5Core.so.5.12.0)
 Block was alloc'd at
   at 0x4839D7B: realloc (vg_replace_malloc.c:826)
   by 0x7439992: QListData::realloc_grow(int) (in 
/usr/lib/libQt5Core.so.5.12.0)
   by 0x7439C1A: QListData::insert(int) (in /usr/lib/libQt5Core.so.5.12.0)
   by 0x576BE43: QList::insert(QList::iterator, 
KFileItem const&) (qlist.h:521)
   by 0x576B1EC: KCoreDirListerCache::DirItem::insert(KFileItem const&) 
(kcoredirlister_p.h:422)
   by 0x575D78C: KCoreDirListerCache::slotEntries(KIO::Job*, 
QList const&) (kcoredirlister.cpp:1252)
   by 0x577C268: QtPrivate::FunctorCall, 
QtPrivate::List const&>, void, void 
(KCoreDirListerCache::*)(KIO::Job*, QList const&)>::call(void 
(KCoreDirListerCache::*)(KIO::Job*, QList const&), 
KCoreDirListerCache*, void**) (qobjectdefs_impl.h:152)
   by 0x577AAC8: void QtPrivate::FunctionPointer 
const&)>::call const&>, 
void>(void (KCoreDirListerCache::*)(KIO::Job*, QList const&), 
KCoreDirListerCache*, void**) (qobjectdefs_impl.h:185)
   by 0x5777D4A: QtPrivate::QSlotObject const&), 
QtPrivate::List const&>, void>::impl(int, 
QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (qobjectdefs_impl.h:414)
   by 0x75C93BF: QMetaObject::activate(QObject*, int, int, void**) (in 
/usr/lib/libQt5Core.so.5.12.0)
   by 0x5703C23: KIO::ListJob::entries(KIO::Job*, QList 
const&) (moc_listjob.cpp:236)
   by 0x57027CE: KIO::ListJobPrivate::slotListEntries(QList 
const&) (listjob.cpp:154)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kd

D17596: [KDirOperator] Allow renaming files from the context menu

[Markus Slopianka]

markuss added a comment.


  Before the feature was removed, the window had all file operations, incl. 
move to trash and IIRC even the ability to cut/copy/paste and move (via D'n'D) 
files.
  
  What's that "ShowDeleteCommand", false? Can it be true and file deletion will 
be possible again as well?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17596

To: ngraham, #frameworks, #dolphin
Cc: markuss, dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Jaime Torres Amate]

jtamate added a comment.


  I'm able to reproduce the bug with the following steps:
  
  - Create a folder structure X/X1/X2/X3/X4 and add a new empty text file into 
each folder.
  - Within Dolphin, open a tab for each folder.
  - In the tab showing X contents, rename X1 to X1_ and the crash is there.
  
  Next step, transform this into a unit test that always crashes.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10742

To: jtamate, #frameworks, dfaure
Cc: elvisangelaccio, bruns, kde-frameworks-devel, mwolff, markg, michaelh, 
ngraham

Google Summer of Code Ideas

[Valorie Zimmerman]

Hi folks, a few teams have added their ideas to
https://community.kde.org/GSoC/2019/Ideas but many have not yet done so.
Those of you reading this plea have participated in the past but do not
have ideas posted yet.

PLEASE get this done before the end of the year! We need to submit our
application  15 January 2019 which is about one month away.

So I beg of you, please get your ideas up soon.

Valorie

-- 
http://about.me/valoriez

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[Jaime Torres Amate]

jtamate added a comment.


  I've been able to reproduce the bug:
  
#10 0x7f44d257ae1d in qt_assert 
(assertion=assertion@entry=0x7f44d456fc43 "it != dirItem->lstItems.end()", 
file=file@entry=0x7f44d456fc10 
"/g/5kde/frameworks/kio/src/core/kcoredirlister_p.h", line=line@entry=308) at 
../../include/QtCore/../../src/corelib/global/qlogging.h:91
#11 0x7f44d4535c7a in KCoreDirListerCache::reinsert 
(this=this@entry=0x7f44d45a5440 <(anonymous 
namespace)::Q_QGS_kDirListerCache::innerFunction()::holder>, item=..., 
oldUrl=...) at /g/5kde/frameworks/kio/src/core/kcoredirlister_p.h:310
#12 0x7f44d452825c in KCoreDirListerCache::renameDir 
(this=this@entry=0x7f44d45a5440 <(anonymous 
namespace)::Q_QGS_kDirListerCache::innerFunction()::holder>, oldUrl=..., 
newUrl=...) at /g/5kde/frameworks/kio/src/core/kcoredirlister.cpp:1584
#13 0x7f44d452aa29 in KCoreDirListerCache::slotFileRenamed 
(this=0x7f44d45a5440 <(anonymous 
namespace)::Q_QGS_kDirListerCache::innerFunction()::holder>, _src=..., 
_dst=..., dstPath=...) at /g/5kde/frameworks/kio/src/core/kcoredirlister.cpp:969
  
  Let's see if this assert/crash can be avoided without reverting all the 
patch. Any help is welcome.

INLINE COMMENTS

> dfaure wrote in kcoredirlister.cpp:963
> Where did the old code call refresh() (which you now call inside findByUrl)? 
> I don't see it, I only see more specific calls in more specific cases. So 
> this looks slower and possibly incorrect (for non-local-files).

Changed to use the reinsert semantic.

> dfaure wrote in kcoredirlister.cpp:1004
> This used to modify the fileitem in dirItem->lstItems, now it's modifying a 
> copy.
> Same for all other fileitem.setFoo calls below.
> 
> Is there a call to reinsert missing?

Thanks, reinsert has a better semantics than refresh.

> markg wrote in kcoredirlister.cpp:825-829
> Hmm, this looks weird to me.
> Sure, it works. "KFileItem retKFileItem = *it;" makes a copy.
> 
> A more efficient way (but requires you to change the lists this is backed by 
> to a std::vector) is to:
> 
> 1. Take the element out of the vector. Something like "KFileItem item = 
> std::move(*it);
> 2. Now the vector would be in a valid state but with one invalid object (will 
> post a problem if you iterate over it later on) so you have to remove that 
> element from the vector like you did.
> 
> I'm not sure if the benefits of this justify the path of changing the list 
> (dirItem->lstItems) to a std::vector, if possible at all.
> That's up to you :)

Changed, using the reinsert semantic.

> bruns wrote in kcoredirlister_p.h:302
> This can be better implemented with std::move and std::move_backward from 
> 
> http://www.cplusplus.com/reference/algorithm/move/
> 
> 1. calculate start and end iterators from the old and new URL
> 2. move old item to tmp
> 3.
> 
>   if (old_it < new_it) {
>  std::move(old_it + 1, new_it, old_it); // move downwards
>   } else {
>  std::move_backward(new_it , old_it - 1, old_it);
>   }
>   *new_it = tmp;

I've looked at those methods, they create a new "undefined" status in the list. 
I don't want to worry about this new status.

> bruns wrote in kfileitem.cpp:180
> You can probably leave this out if you use the following for ordering:
> 
>   bool operator< (const KFileItem& other) {
> if m_url.size() != other.m_url.size() {
>   return m_url.size() < other.m_url.size()
> }
> return m_url < other.m_url;
>   }
> 
> You don't need lexicographical sorting, but just a total ordering for the 
> lookup.

I've tried something similar. As QUrl doesn't have a size method, I've tried 
with path().size(). With findByUrl it is no problem, it gives me 28 msecs vs. 
1795 msecs, but inserting in the list gives me 37 msecs vs. the original 19 
msecs. 
Only checking m_url < other.m_url (without m_hash) gives me 21 msecs inserting 
and 15 msecs in findByUrl, faster than using m_hash with the right checks for 
collisions (22-23 msecs inserting).

> bruns wrote in kfileitem.h:490
> The description does not match the implementation ...

Changed, thanks. Now they match.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10742

To: jtamate, #frameworks, dfaure
Cc: elvisangelaccio, bruns, kde-frameworks-devel, mwolff, markg, michaelh, 
ngraham

D10446: Add KLanguageName

[Albert Astals Cid]

aacid added a comment.


  In D10446#376825 , @sitter wrote:
  
  > Stacking the functions seems to work fine
  >
  >   QString KLanguageName::nameForCode(const QString &code)
  >   {
  >   const QStringList parts = QLocale().name().split(QChar('_'));
  >   return nameForCodeInLocale(code, parts.at(0));
  >   }
  >
  >
  > I do have various refinements and fixes for the logic plus a unit test I 
can update the diff if you are ok with that.
  
  
  Sure, tests are nice i remember this was kind of a bit of a hit and miss 
sometimes. But are you going to depend on having all of l10n installed (which 
is where kf5_entry.desktop files come from or will you have some fake 
kf5_entry.destkops for the test? (second would be awesome)
  
  > As for kcoreaddons, I don't think we could go there with kconfig could we? 
And I am not sure QSettings is up to the task?
  
  QSettings doesn't support language entries AFAIK
  
  > I also have some behavior concerns. The auto-fallback to QLocale is super 
handy, but isn't very flexible: As an application author I might have another 
way to map a language to a pretty string, if KLanguageName automatically falls 
back to QLocale, I won't be able to easily determine if I should use another 
(possibly better) fallback.
  >  At the same time falling back to `QLocale::languageToString` is not 
necessarily in the interest of the user either. Who's to say the user will know 
what a language means in English.
  > 
  > So, I haven't give this a great deal of thought, but it seems that always 
returning QString(), if we cannot resolve a string internally, is possibly more 
flexible. Worst case the consumer has to do:
  > 
  >   str = KLanguageName::nameForCode("fr")
  >   if (str.isEmpty()) { str = QLocale("fr").nativeLanguageName() }
  > 
  > 
  > Which isn't that much code TBH.
  
  But you end up repeating that in lots of places (which we should there's lots 
of places that suffer from trying to guess a language name at this point, and 
all of them went the bad way one way or another). If you want to give some 
random potential user more flexibility i'm fine with that, add some flags, but 
i want the "give me the best you can do" possibility to still work. Maybe we 
should even never return an empty string and worst case scenario return code 
back.

REPOSITORY
  R265 KConfigWidgets

REVISION DETAIL
  https://phabricator.kde.org/D10446

To: aacid
Cc: kde-frameworks-devel, sitter, markg, apol, michaelh, ngraham, bruns

D17595: Upstream Dolphin's file rename dialog

[Dominik Haumann]

dhaumann added inline comments.

INLINE COMMENTS

> renamefiledialog.h:5
> + *   This program is free software; you can redistribute it and/or modify  *
> + *   it under the terms of the GNU General Public License as published by  *
> + *   the Free Software Foundation; either version 2 of the License, or *

Is the rest of KIO LGPL or GPL? If we have a license mismatch, we'd have to ask 
the authors for an OK to relicense.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17595

To: ngraham, #frameworks, #dolphin, broulik
Cc: dhaumann, pino, kde-frameworks-devel, michaelh, ngraham, bruns

D17596: [KDirOperator] Allow renaming files from the context menu

[Dominik Haumann]

dhaumann added a comment.


  Not sure about binary compatibility...

INLINE COMMENTS

> kdiroperator.h:774
> + */
> +virtual void renameSelected();
> +

Adding a virtual function changes the vtable, so this is BIC (binary 
incompatible). Does the function need to be virtual?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17596

To: ngraham, #frameworks, #dolphin
Cc: dhaumann, kde-frameworks-devel, michaelh, ngraham, bruns

D17595: Upstream Dolphin's file rename dialog

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

> renamefiledialog.cpp:50-51
> +{
> +const QSize minSize = minimumSize();
> +setMinimumSize(QSize(320, minSize.height()));
> +

why a minimum size is enforced?

> renamefiledialog.cpp:54
> +const int itemCount = items.count();
> +Q_ASSERT(itemCount >= 1);
> +m_renameOneItem = (itemCount == 1);

assert in a library, especially when checking user input, is never a good 
idea...

> renamefiledialog.h:65-73
> +private:
> +bool m_renameOneItem;
> +QList m_renamedItems;
> +QString m_newName;
> +QLineEdit* m_lineEdit;
> +KFileItemList m_items;
> +bool m_allExtensionsDifferent;

an exported class must use a d-pointer for all the private variables & members

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17595

To: ngraham, #frameworks, #dolphin, broulik
Cc: pino, kde-frameworks-devel, michaelh, ngraham, bruns