D14666: Fixes memory leak in KUrlComboBox::setUrl

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kurlcombobox.cpp:279 > Q_ASSERT(!d->itemList.isEmpty()); > +delete d->itemList.last(); > d->itemList.removeLast(); This could be

D14662: KPropertiesDialog: switch to label in setFileNameReadOnly(true)

rkflx added a comment. Thanks for the help. Works great, just one inline question about an edge case (everything else LGTM). INLINE COMMENTS > kpropertiesdialog.cpp:853 > QGridLayout *grid = new QGridLayout(); // unknown rows > +d->m_grid = grid; > grid->setColumnStretch(0,

D14663: Give the "invalid directory name" dialog a cancel button

ngraham added reviewers: elvisangelaccio, Dolphin. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D14663 To: ngraham, #vdg, #frameworks, elvisangelaccio, #dolphin Cc: kde-frameworks-devel, michaelh, ngraham, bruns

KDE CI: Frameworks kio kf5-qt5 SUSEQt5.9 - Build # 200 - Still Unstable!

BUILD UNSTABLE Build URL https://build.kde.org/job/Frameworks%20kio%20kf5-qt5%20SUSEQt5.9/200/ Project: Frameworks kio kf5-qt5 SUSEQt5.9 Date of build: Tue, 07 Aug 2018 21:05:56 + Build duration: 15 min and counting JUnit Tests Name: (root)

D14666: Fixes memory leak in KUrlComboBox::setUrl

hallas created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: kde-frameworks-devel. hallas requested review of this revision. REVISION SUMMARY Subject: Fixes memory leak in KUrlComboBox::setUrl Details: If setUrl is called

KDE CI: Frameworks kio kf5-qt5 SUSEQt5.10 - Build # 355 - Still Unstable!

BUILD UNSTABLE Build URL https://build.kde.org/job/Frameworks%20kio%20kf5-qt5%20SUSEQt5.10/355/ Project: Frameworks kio kf5-qt5 SUSEQt5.10 Date of build: Tue, 07 Aug 2018 21:05:56 + Build duration: 5 min 41 sec and counting JUnit Tests Name:

Re: KDE CI: Dependency Build Extragear kf5-qt5 SUSEQt5.9 - Build # 121 - Still Failing!

On Thu, Aug 2, 2018 at 6:10 AM, Ben Cooksley wrote: > Hi folks, > Hi all, > > Looks like there has been a regression in libkscreen, as can be seen below. > This now seems to have been fixed, but the failure point has now moved into KWin. Please see

KDE CI: Frameworks kio kf5-qt5 FreeBSDQt5.10 - Build # 110 - Still Unstable!

BUILD UNSTABLE Build URL https://build.kde.org/job/Frameworks%20kio%20kf5-qt5%20FreeBSDQt5.10/110/ Project: Frameworks kio kf5-qt5 FreeBSDQt5.10 Date of build: Tue, 07 Aug 2018 21:05:56 + Build duration: 1 hr 59 min and counting JUnit Tests

D14663: Give the "invalid directory name" dialog a cancel button

ngraham updated this revision to Diff 39275. ngraham added a comment. Rebase on latest master REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14663?vs=39234=39275 BRANCH arcpatch-D14663 REVISION DETAIL https://phabricator.kde.org/D14663 AFFECTED FILES

D14666: Fixes memory leak in KUrlComboBox::setUrl

hallas added inline comments. INLINE COMMENTS > dfaure wrote in kurlcombobox.cpp:358 > problem: `i` shouldn't be in increased after this... > > Maybe this should use iterators instead? > > A unitttest is missing for this code path, in any case. In general this class is not covered very well

D14666: Fixes memory leak in KUrlComboBox::setUrl

hallas updated this revision to Diff 39265. hallas marked 2 inline comments as done. hallas added a comment. Use remove_if for removing entries from itemList and defaultList. Added unit test of KUrlComboBox::removeUrl. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

D14641: Refine wording when a folder with an invalid name could not be created

ngraham closed this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D14641 To: ngraham, #dolphin, #frameworks, #vdg, tmarshall, elvisangelaccio Cc: markuss, kde-frameworks-devel, michaelh, ngraham, bruns

D13584: KFormat: Replace byte specific implementation with generic one

astippich added a comment. I've been running with it for a while and found no issues, but only giving +1 this time because of the fallout from the other patch (which I didn't catch, so others should have a look) REPOSITORY R244 KCoreAddons REVISION DETAIL

D14641: Refine wording when a folder with an invalid name could not be created

elvisangelaccio accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO BRANCH refine-wording (branched from master) REVISION DETAIL https://phabricator.kde.org/D14641 To: ngraham, #dolphin, #frameworks, #vdg, tmarshall, elvisangelaccio Cc: markuss,

D13583: KFormat: Allow usage of quantities beyond bytes and seconds

safaalfulaij added a comment. > Yes. As you said, these are symbols. For the names there may be transliterations, the symbols stay the same, to avoid ambiguities. This also matches my experience with datasheets, the text may be chinese, but measurements are given in SI units. I just

D14579: api for multi level kcms

broulik added inline comments. INLINE COMMENTS > kcmodule.cpp:59 > QString m_ExportText; > +QList _levelTitles; > +uint _currentLevel = 0; `QStringList`? > kcmodule.h:273 > + */ > +uint depth() const; > + `uint` is quite unusual for our APIs, isn't it? > kcmodule.h:286 >

D14666: Fixes memory leak in KUrlComboBox::setUrl

hallas updated this revision to Diff 39249. hallas added a comment. Refactored the code to use std::shared pointers instead. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14666?vs=39240=39249 BRANCH master REVISION DETAIL

D14666: Fixes memory leak in KUrlComboBox::setUrl

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. Why shared? I don't see any ownership sharing happening. The list owns the pointers. I'm always wary of shared_ptr because it's often overused as "we don't really know who's

D14162: Figure out the escaped path list on kconfig

apol added a comment. In D14162#304826 , @dfaure wrote: > I don't really understand why we can't just skip the escapes as we go along, as most parsers do, for the sake of efficiency. This is already a state-machine based parser so it should be

D14668: [KCM GridDelegate] Use layer effect only on OpenGL backend

broulik closed this revision. broulik added a comment. https://cgit.kde.org/kdeclarative.git/commit/?id=94c6f3152188d15e84019c4aafcc7835b7f42b12 REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D14668 To: broulik, #plasma, mart, davidedmundson Cc:

D14666: Fixes memory leak in KUrlComboBox::setUrl

hallas added a comment. In D14666#304848 , @dfaure wrote: > Why shared? I don't see any ownership sharing happening. The list owns the pointers. I'm always wary of shared_ptr because it's often overused as "we don't really know who's

D14666: Fixes memory leak in KUrlComboBox::setUrl

dfaure resigned from this revision. dfaure added a comment. The ownership was clear in the original code: itemList and defaultList own the items, the rest (like itemMapper) doesn't. So unique_ptrs in itemList and defaultList, and raw pointers elsewhere, would be perfectly fine and much

D14668: [KCM GridDelegate] Use layer effect only on OpenGL backend

davidedmundson accepted this revision. This revision is now accepted and ready to land. REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D14668 To: broulik, #plasma, mart, davidedmundson Cc: kde-frameworks-devel, michaelh, ngraham, bruns

Re: ktextwidgets on FreeBSD

I can read speech-dispatcher, which had the hanging processes, if you want. mfg Tobias On Sun, 5 Aug 2018 at 01:41, Ben Cooksley wrote: > On Sat, Aug 4, 2018 at 11:06 PM, David Faure wrote: > > Anyone running FreeBSD, who could to try and debug this hanging unittest > from the ktextwidgets

D14162: Figure out the escaped path list on kconfig

dfaure added a comment. I don't really understand why we can't just skip the escapes as we go along, as most parsers do, for the sake of efficiency. This is already a state-machine based parser so it should be easy to integrate that in, no? REPOSITORY R237 KConfig REVISION DETAIL

D14666: Fixes memory leak in KUrlComboBox::setUrl

dfaure added a comment. std::unique_ptr sounds fine indeed. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D14666 To: hallas, dfaure Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14668: [KCM GridDelegate] Use layer effect only on OpenGL backend

broulik edited the test plan for this revision. REPOSITORY R296 KDeclarative REVISION DETAIL https://phabricator.kde.org/D14668 To: broulik, #plasma, mart Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14668: [KCM GridDelegate] Use layer effect only on OpenGL backend

broulik created this revision. broulik added reviewers: Plasma, mart. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: kde-frameworks-devel. broulik requested review of this revision. REVISION SUMMARY `QtGraphicalEffects` only works then, otherwise

D14666: Fixes memory leak in KUrlComboBox::setUrl

hallas added inline comments. INLINE COMMENTS > dfaure wrote in kurlcombobox.cpp:363 > Doesn't this have the same issue? delete missing Sure has :) I was thinking if a nicer solution would be to use a smart pointer in the itemList instead? That way we wouldn't need to go hunting for this and

D14674: handle non-ASCII encodings of file names in tar archives

broulik added reviewers: dfaure, kossebau. REPOSITORY R243 KArchive REVISION DETAIL https://phabricator.kde.org/D14674 To: ibragimovrinat, dfaure, kossebau Cc: ibragimovrinat, kde-frameworks-devel, michaelh, ngraham, bruns

D14674: handle non-ASCII encodings of file names in tar archives

ibragimovrinat created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: kde-frameworks-devel. ibragimovrinat requested review of this revision. REVISION SUMMARY BUG: 266141 As UTF-8 may use multiple bytes per character, it's

D14674: handle non-ASCII encodings of file names in tar archives

cfeck added inline comments. INLINE COMMENTS > ktar.cpp:777 > +// If more than 100 bytes, we need to use the LongLink trick > +if (encodedFileName.length() > 99) { > d->writeLonglink(buffer, encodedFileName, 'L', uname.constData(), > gname.constData()); Does the spec say `>

D14666: Fixes memory leak in KUrlComboBox::setUrl

hallas updated this revision to Diff 39252. hallas added a comment. Refactored to use std::vector and std::unique_ptr REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14666?vs=39249=39252 BRANCH master REVISION DETAIL https://phabricator.kde.org/D14666

D14579: api for multi level kcms

abetts added a comment. Awesome! REPOSITORY R265 KConfigWidgets REVISION DETAIL https://phabricator.kde.org/D14579 To: mart, #plasma, #frameworks Cc: abetts, broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. Thanks ;) INLINE COMMENTS > kurlcombobox.cpp:358 > +if (d->itemList.at(i).get() == mit.value()) { > +d->itemList.erase(d->itemList.begin() +

D14674: handle non-ASCII encodings of file names in tar archives

broulik added a comment. Sorry, I just added you as I looked at who committed to that repo most, to ensure this review doesn't go unnoticed for the lack of reviewers :) REPOSITORY R243 KArchive REVISION DETAIL https://phabricator.kde.org/D14674 To: ibragimovrinat, dfaure, kossebau Cc:

D14674: handle non-ASCII encodings of file names in tar archives

kossebau added a subscriber: broulik. kossebau added a comment. Thanks for proposing a patch. for the bug @ibragimovrinat Myself I only contributed once in former times to karchive, all memories lost, not sure if I have time to dive into that codebase again, despite @broulik here poking