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

[Ben Cooksley]

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
https://build.kde.org/job/Dependency%20Build%20Extragear%20kf5-qt5%20SUSEQt5.9/lastFailedBuild/console


If someone could fix that it would be appreciated.

Please note that as parts of Extragear (which depend on Qt 5.9 as their
minimum) depend on parts of Plasma it's still a requirement that Plasma is
buildable with Qt 5.9.
The components in Extragear which create these dependencies are Mycroft,
Apper, KDevelop and Plasma's own SDK.


> Can someone please take a look?
>
> Thanks,
> Ben
>

Cheers,
Ben


>
> -- Forwarded message --
> From: CI System 
> Date: Thu, Aug 2, 2018 at 1:00 AM
> Subject: KDE CI: Dependency Build Extragear kf5-qt5 SUSEQt5.9 - Build #
> 121 - Still Failing!
> To: bcooks...@kde.org
>
>
> *BUILD FAILURE*
> Build URL https://build.kde.org/job/Dependency%20Build%20Extragear%
> 20kf5-qt5%20SUSEQt5.9/121/
> Project: Dependency Build Extragear kf5-qt5 SUSEQt5.9
> Date of build: Wed, 01 Aug 2018 10:01:53 +
> Build duration: 2 hr 58 min and counting
> * CONSOLE OUTPUT *
> [...truncated 62.14 MB...]
> [ 37%] Linking CXX shared module ../../bin/kf5/kscreen/KSC_QScreen.so
> [ 37%] Built target KSC_QScreen
> Scanning dependencies of target KSC_KWayland
> [ 37%] Building CXX object backends/kwayland/CMakeFiles/K
> SC_KWayland.dir/waylandbackend.cpp.o
> [ 38%] Building CXX object backends/kwayland/CMakeFiles/K
> SC_KWayland.dir/waylandconfig.cpp.o
> [ 38%] Building CXX object backends/kwayland/CMakeFiles/K
> SC_KWayland.dir/waylandoutput.cpp.o
> [ 39%] Building CXX object backends/kwayland/CMakeFiles/K
> SC_KWayland.dir/waylandscreen.cpp.o
> [ 40%] Building CXX object backends/kwayland/CMakeFiles/K
> SC_KWayland.dir/__/utils.cpp.o
> [ 40%] Building CXX object backends/kwayland/CMakeFiles/K
> SC_KWayland.dir/KSC_KWayland_autogen/mocs_compilation.cpp.o
> [ 41%] Linking CXX shared module ../../bin/kf5/kscreen/KSC_KWayland.so
> [ 41%] Built target KSC_KWayland
> Scanning dependencies of target KSC_XRandR
> [ 42%] Building CXX object backends/xrandr/CMakeFiles/KSC
> _XRandR.dir/xrandr.cpp.o
> [ 42%] Building CXX object backends/xrandr/CMakeFiles/KSC
> _XRandR.dir/xrandrconfig.cpp.o
> [ 43%] Building CXX object backends/xrandr/CMakeFiles/KSC
> _XRandR.dir/xrandrcrtc.cpp.o
> [ 43%] Building CXX object backends/xrandr/CMakeFiles/KSC
> _XRandR.dir/xrandroutput.cpp.o
> [ 44%] Building CXX object backends/xrandr/CMakeFiles/KSC
> _XRandR.dir/xrandrmode.cpp.o
> [ 44%] Building CXX object backends/xrandr/CMakeFiles/KSC
> _XRandR.dir/xrandrscreen.cpp.o
> [ 45%] Building CXX object backends/xrandr/CMakeFiles/KSC
> _XRandR.dir/__/xcbwrapper.cpp.o
> [ 45%] Building CXX object backends/xrandr/CMakeFiles/KSC
> _XRandR.dir/__/xcbeventlistener.cpp.o
> [ 46%] Building CXX object backends/xrandr/CMakeFiles/KSC
> _XRandR.dir/__/utils.cpp.o
> [ 46%] Building CXX object backends/xrandr/CMakeFiles/KSC
> _XRandR.dir/KSC_XRandR_autogen/mocs_compilation.cpp.o
> [ 47%] Linking CXX shared module ../../bin/kf5/kscreen/KSC_XRandR.so
> [ 47%] Built target KSC_XRandR
> Scanning dependencies of target KSC_XRandR11
> [ 48%] Building CXX object backends/xrandr1.1/CMakeFiles/
> KSC_XRandR11.dir/xrandr11.cpp.o
> [ 48%] Building CXX object backends/xrandr1.1/CMakeFiles/
> KSC_XRandR11.dir/__/xcbeventlistener.cpp.o
> [ 49%] Building CXX object backends/xrandr1.1/CMakeFiles/
> KSC_XRandR11.dir/__/xcbwrapper.cpp.o
> [ 49%] Building CXX object backends/xrandr1.1/CMakeFiles/
> KSC_XRandR11.dir/KSC_XRandR11_autogen/mocs_compilation.cpp.o
> [ 50%] Linking CXX shared module ../../bin/kf5/kscreen/KSC_XRandR11.so
> [ 50%] Built target KSC_XRandR11
> [ 50%] Generating fakebackendinterface.cpp, fakebackendinterface.h
> [ 51%] Generating fakebackendinterface.moc
> Scanning dependencies of target testbackendloader
> [ 52%] Building CXX object autotests/CMakeFiles/testbacke
> ndloader.dir/testbackendloader.cpp.o
> [ 52%] Building CXX object autotests/CMakeFiles/testbacke
> ndloader.dir/fakebackendinterface.cpp.o
> [ 53%] Building CXX object autotests/CMakeFiles/testbacke
> ndloader.dir/testbackendloader_autogen/mocs_compilation.cpp.o
> [ 53%] Linking CXX executable ../bin/testbackendloader
> [ 53%] Built target testbackendloader
> Scanning dependencies of target testscreenconfig
> [ 54%] Building CXX object autotests/CMakeFiles/testscree
> nconfig.dir/testscreenconfig.cpp.o
> [ 55%] Building CXX object autotests/CMakeFiles/testscree
> nconfig.dir/fakebackendinterface.cpp.o
> [ 55%] Building CXX object autotests/CMakeFiles/testscree
> nconfig.dir/testscreenconfig_autogen/mocs_compilation.cpp.o
> [ 56%] Linking CXX executable ../bin/testscreenconfig
> [ 57%] Built target testscreenconfig
> Scanning dependencies of target testconfigserializer
> [ 57%] Building CXX object 

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

[CI System]

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
  Name: (root) Failed: 5 test(s), Passed: 53 test(s), Skipped: 0 test(s), Total: 58 test(s)Failed: TestSuite.kiocore-jobtestFailed: TestSuite.kiocore-kmountpointtestFailed: TestSuite.kiofilewidgets-kfileplacesviewtestFailed: TestSuite.kiowidgets-kdirlistertestFailed: TestSuite.kiowidgets-kdirmodeltest

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

[CI System]

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) Failed: 1 test(s), Passed: 58 test(s), Skipped: 0 test(s), Total: 59 test(s)Failed: TestSuite.kiofilewidgets-kfileplacesviewtest
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report64%
(23/36)65%
(258/396)65%
(258/396)53%
(31948/59913)38%
(16142/42676)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests100%
(55/55)100%
(55/55)95%
(9025/9455)51%
(3929/7716)autotests.http100%
(5/5)100%
(5/5)99%
(581/582)68%
(113/166)autotests.kcookiejar100%
(1/1)100%
(1/1)91%
(179/197)72%
(49/68)src100%
(1/1)100%
(1/1)86%
(6/7)67%
(4/6)src.core84%
(98/116)84%
(98/116)58%
(8332/14357)50%
(4653/9289)src.core.kssl100%
(1/1)100%
(1/1)40%
(35/88)50%
(3/6)src.filewidgets76%
(28/37)76%
(28/37)49%
(3893/7926)34%
(1585/4669)src.gui100%
(2/2)100%
(2/2)94%
(103/109)74%
(49/66)src.ioslaves.file100%
(5/5)100%
(5/5)52%
(527/1015)39%
(315/814)src.ioslaves.file.kauth0%
(0/2)0%
(0/2)0%
(0/106)0%
(0/65)src.ioslaves.ftp0%
(0/1)0%
(0/1)0%
(0/1364)0%
(0/1414)src.ioslaves.help0%
(0/5)0%
(0/5)0%
(0/245)0%
(0/144)src.ioslaves.http88%
(7/8)88%
(7/8)41%
(1770/4320)35%
(1306/3700)src.ioslaves.http.kcookiejar33%
(2/6)33%
(2/6)47%
(629/1331)55%
(619/1123)src.ioslaves.remote100%
(2/2)100%
(2/2)28%
(72/257)7%
(14/212)src.ioslaves.remote.kdedmodule0%
(0/2)0%
(0/2)0%
(0/12)100%
(0/0)src.ioslaves.telnet0%
(0/1)0%
(0/1)0%
(0/43)0%
(0/30)src.ioslaves.trash56%
(5/9)56%
(5/9)52%
  

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

[Nathaniel Graham]

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
  src/filewidgets/knewfilemenu.cpp

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

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

[Safa Alfulaij]

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 saw¹ this PR and I'd like to say something.
  Yes, in scientific papers we use English terms/symbols so that they are 
understood by others, and to have a system for communication (same goes for 
IUPAC naming.)
  But for users this is wrong, especially with different scripts, and even with 
right-to-left languages. We are here so that they understand that, not others.
  These are the Chinese 
, Russian 
, Arabic 
, Hebrew 
 and Persian 
 Unicode pages 
showing different units.
  
  Maybe in Chinese you don't use the localized version of the symbols, but I 
think you use English full-width characters, right? How these will be localized 
then?
  Same case goes for Russian that uses thier cyrillic script.
  
  Arabic, Hebrew and Persian are a different case. It is always considered bad 
to mix between Arabic and English text, and if so then in extreme hard cases 
(where you can't translate MySQL or a command-line application that no one 
really use except for developers, or maybe Qt method names or classes.).
  But here we are mixing just few characters with few numbers and things are in 
a mess. If this was used in QML (where text direction is detected 
automaticlly), things will go wrong for RTL languages, and you'll see the unit 
name on the right and the quantity on the left.
  
  F6180270: Screenshot_٢٠١٨٠٨٠٧_٢٣٥٨٠٩.png 

  //Fixed by prepending a RLM to the sentence, as Kate detects text direction 
automatically as well//
  
  Please, if your language uses English symbols, it doesn't mean that every 
other must follow. KI18n previous (and maybe current, not sure) developers 
created scripting-enabled translation system so that each language team can do 
whatever suits thier language.
  
  Thank you.
  
  1: After I saw the new PR  using this.
  
  P.S: Check these files in KDE l10n repositories:
  
https://websvn.kde.org/trunk/l10n-kf5/fr/messages/frameworks/kcoreaddons5_qt.po?view=markup
 (starting from line 225)
  
https://websvn.kde.org/trunk/l10n-kf5/ru/messages/frameworks/kcoreaddons5_qt.po?view=markup
 (starting from line 230)
  
https://websvn.kde.org/trunk/l10n-kf5/fa/messages/frameworks/kcoreaddons5_qt.po?view=markup
 (starting from line 325)
  
https://websvn.kde.org/trunk/l10n-kf5/he/messages/frameworks/kcoreaddons5_qt.po?view=markup
 (starting from line 247)
  
https://websvn.kde.org/trunk/l10n-kf5/ar/messages/frameworks/kcoreaddons5_qt.po?view=markup
 (starting from line 204)

REPOSITORY
  R244 KCoreAddons

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

To: bruns, #frameworks, astippich
Cc: safaalfulaij, siddharthasahu, bcooksley, kossebau, kde-frameworks-devel, 
astippich, michaelh, ngraham, bruns

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

[CI System]

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: (root) Failed: 1 test(s), Passed: 58 test(s), Skipped: 0 test(s), Total: 59 test(s)Failed: TestSuite.kiofilewidgets-kfileplacesviewtest
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report64%
(23/36)65%
(258/396)65%
(258/396)53%
(31886/59913)38%
(16114/42678)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests100%
(55/55)100%
(55/55)95%
(9025/9455)51%
(3929/7716)autotests.http100%
(5/5)100%
(5/5)99%
(581/582)68%
(113/166)autotests.kcookiejar100%
(1/1)100%
(1/1)91%
(179/197)72%
(49/68)src100%
(1/1)100%
(1/1)86%
(6/7)67%
(4/6)src.core84%
(98/116)84%
(98/116)58%
(8284/14357)50%
(4638/9285)src.core.kssl100%
(1/1)100%
(1/1)40%
(35/88)50%
(3/6)src.filewidgets76%
(28/37)76%
(28/37)49%
(3894/7926)34%
(1586/4669)src.gui100%
(2/2)100%
(2/2)94%
(103/109)74%
(49/66)src.ioslaves.file100%
(5/5)100%
(5/5)52%
(527/1015)39%
(315/814)src.ioslaves.file.kauth0%
(0/2)0%
(0/2)0%
(0/106)0%
(0/65)src.ioslaves.ftp0%
(0/1)0%
(0/1)0%
(0/1364)0%
(0/1414)src.ioslaves.help0%
(0/5)0%
(0/5)0%
(0/245)0%
(0/144)src.ioslaves.http88%
(7/8)88%
(7/8)41%
(1775/4320)35%
(1304/3700)src.ioslaves.http.kcookiejar33%
(2/6)33%
(2/6)47%
(630/1331)55%
(620/1123)src.ioslaves.remote100%
(2/2)100%
(2/2)28%
(72/257)7%
(14/212)src.ioslaves.remote.kdedmodule0%
(0/2)0%
(0/2)0%
(0/12)100%
(0/0)src.ioslaves.telnet0%
(0/1)0%
(0/1)0%
(0/43)0%
(0/30)src.ioslaves.trash56%
(5/9)56%

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

[Nathaniel Graham]

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

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

[Nathaniel Graham]

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

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

[Elvis Angelaccio]

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, kde-frameworks-devel, michaelh, ngraham, bruns

D13584: KFormat: Replace byte specific implementation with generic one

[Alexander Stippich]

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
  https://phabricator.kde.org/D13584

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

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Hallas]

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 by unit test, I have added a 
test case that covers most of the removeUrl functionality, but I think I will 
do a different patch with more unit test.

REPOSITORY
  R241 KIO

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

To: hallas, dfaure
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Hallas]

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
  https://phabricator.kde.org/D14666?vs=39252=39265

BRANCH
  master

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

AFFECTED FILES
  autotests/kurlcomboboxtest.cpp
  autotests/kurlcomboboxtest.h
  src/widgets/kurlcombobox.cpp

To: hallas, dfaure
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

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

[Henrik Fehlauer]

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, 0);

Just out of interest (don't change anything): I assume you don't fully 
transition everything to `d->m_grid` to keep the diff small, or because `grid` 
is more readable?

> kpropertiesdialog.cpp:1211-1214
> +d->m_fileNameLabel = new QLabel(d->m_frame);
> +
> d->m_fileNameLabel->setTextInteractionFlags(Qt::TextSelectableByMouse | 
> Qt::TextSelectableByKeyboard);
> +d->m_fileNameLabel->setText(d->oldName); // will get overwritten if 
> d->bMultiple
> +d->m_grid->addWidget(d->m_fileNameLabel, 0, 2);

Wouldn't this mean that calling `setFileNameReadOnly(true)` multiple times will 
also create multiple labels on top of each other, breaking idempotence and 
resulting in some kind of bold font effect?

REPOSITORY
  R241 KIO

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

To: dfaure, rkflx, shubham, ngraham
Cc: bruns, michaelh, kde-frameworks-devel, ngraham

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

[Kai Uwe Broulik]

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: broulik, cfeck, ibragimovrinat, kde-frameworks-devel, michaelh, ngraham, 
bruns

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

[Friedrich W. H. Kossebau]

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 me :)
  
  One thing though for sure which would be wanted here is hardening the code by 
having some unit tests covering ascii-only and non-ascii file names. So whoever 
might review this patch surely would like to see some samples tested somewhere 
below autotests/ :)

REPOSITORY
  R243 KArchive

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

To: ibragimovrinat, dfaure, kossebau
Cc: broulik, cfeck, ibragimovrinat, kde-frameworks-devel, michaelh, ngraham, 
bruns

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

[Christoph Feck]

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 `> 99` or `> 100`? The comment and the code should match.

REPOSITORY
  R243 KArchive

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

To: ibragimovrinat, dfaure, kossebau
Cc: cfeck, ibragimovrinat, kde-frameworks-devel, michaelh, ngraham, bruns

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

[Kai Uwe Broulik]

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

[Rinat Ibragimov]

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 required to measure 
encoded file name length in bytes, not just in characters. It's possible to 
accidentally truncate file name if its name in characters is shorter that 100, 
but in bytes — longer than 100. Also, checksum calculation must be performed on 
unsigned bytes. Otherwise bytes in range 0x80-0xff when promoted to int become 
negative.

REPOSITORY
  R243 KArchive

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

AFFECTED FILES
  src/ktar.cpp

To: ibragimovrinat
Cc: ibragimovrinat, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Faure]

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() + i);
> +break;

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.

> kurlcombobox.cpp:365
> +if (d->defaultList.at(i).get() == mit.value()) {
> +d->defaultList.erase(d->defaultList.begin() + i);
> +break;

same here.

Maybe erase(remove_if()) would be best.

REPOSITORY
  R241 KIO

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

To: hallas, dfaure
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14579: api for multi level kcms

[Andres Betts]

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

[David Hallas]

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

AFFECTED FILES
  autotests/kurlcomboboxtest.cpp
  autotests/kurlcomboboxtest.h
  src/widgets/kurlcombobox.cpp

To: hallas
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

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

[Kai Uwe Broulik]

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: kde-frameworks-devel, michaelh, ngraham, bruns

D14579: api for multi level kcms

[Kai Uwe Broulik]

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
> + */
> +int currentLevel() const;
> +

`setCurrentLevel` is `uint` but this returns `int`

REPOSITORY
  R265 KConfigWidgets

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

To: mart, #plasma, #frameworks
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

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

[David Edmundson]

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

D14162: Figure out the escaped path list on kconfig

[Aleix Pol Gonzalez]

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 easy to integrate that in, no?
  
  
  You mean we could integrate it in KConfigIni itself?
  Currently values are always stored as QString, it would need a refactoring to 
allow to store values in different types.

REPOSITORY
  R237 KConfig

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

To: apol, #frameworks, dfaure
Cc: dfaure, anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Faure]

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 clearer.
  Don't fall into the "no raw pointers" trap. The correct recommendation is "no 
raw pointers that own the object".

REPOSITORY
  R241 KIO

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

To: hallas
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Hallas]

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 responsible for deleting this, so let's just refcount 
it". But there's no refcounting needed here. The ownership is very clear here.
  
  
  My first approach was to use unique_ptr, but then I realized that pointers 
are shared between the defaultList and the itemMapper (see for example line 
164) and also between the itemList and the itemMapper (see line 278, 291 and 
others), but I can't see any sharing between the defaultList and itemList. I 
thought of using a std::weak_ptr in the itemMapper, but I am not sure that 
entries are always removed from itemMapper when the are removed from the 
defaultList or itemMapper, also if the itemMapper should be able to hold 
objects from both lists I think it is more sane to use a shared_ptr.
  
  Let me know what you think

REPOSITORY
  R241 KIO

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

To: hallas, dfaure
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Faure]

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 responsible for deleting this, so let's just refcount 
it". But there's no refcounting needed here. The ownership is very clear here.

REPOSITORY
  R241 KIO

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

To: hallas, dfaure
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Hallas]

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
  https://phabricator.kde.org/D14666

AFFECTED FILES
  autotests/kurlcomboboxtest.cpp
  autotests/kurlcomboboxtest.h
  src/widgets/kurlcombobox.cpp

To: hallas, dfaure
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14162: Figure out the escaped path list on kconfig

[David Faure]

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
  https://phabricator.kde.org/D14162

To: apol, #frameworks, dfaure
Cc: dfaure, anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns

Re: ktextwidgets on FreeBSD

[Tobias C. Berner]

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 framework?
> >
> >
> https://build.kde.org/view/Frameworks/job/Frameworks%20ktextwidgets%20kf5-qt5%20FreeBSDQt5.10/9/testReport/junit/(root)/TestSuite/ktextwidgets_krichtextedittest/
> >
> > There's no waiting of any kind in the code, it's straight QTextDocument
> usage, this shouldn't deadlock or anything.
> > Yet it times out (after 30 seconds!!) quite often in CI.
>
> This is a rather unusual failure, as the CI system is giving that test
> 5 minutes to timeout, after the test has finished.
>
> Usually this happens when tests start background daemons and don't
> kill those processes off before the test exits (which CTest doesn't
> like).
> However I don't think it's a background process sticking around in
> this case as those type of hangs are usually
> manual-intervention-required ones, which this isn't.
>
> Unfortunately it seems that the issue has gone away though so it's not
> possible to dig into this much further...
>
> https://build.kde.org/view/Frameworks/job/Frameworks%20ktextwidgets%20kf5-qt5%20FreeBSDQt5.10/22/
>
> >
> > Just a backtrace would be very useful already.
> >
> >
> > void KRichTextEditTest::testUpdateLinkAdd()
> > {
> > KRichTextEdit edit;
> > edit.enableRichTextMode();
> >
> > // Add text, apply initial formatting, and add a link
> > QTextCursor cursor = edit.textCursor();
> > cursor.insertText(QStringLiteral("Test"));
> > QTextCharFormat charFormat = cursor.charFormat();
> > // Note that QTextEdit doesn't use the palette. Black is black.
> > QCOMPARE(charFormat.foreground().color().name(),
> QColor(Qt::black).name());
> >
> > cursor.select(QTextCursor::BlockUnderCursor);
> > edit.setTextCursor(cursor);
> > edit.setTextBold(true);
> > edit.setTextItalic(true);
> > edit.updateLink(QStringLiteral("http://www.kde.org;),
> QStringLiteral("KDE"));
> >
> > // Validate text and formatting
> > cursor.movePosition(QTextCursor::Start);
> > cursor.select(QTextCursor::WordUnderCursor);
> > edit.setTextCursor(cursor);
> > QCOMPARE(edit.toPlainText(), QStringLiteral("KDE "));
> > QCOMPARE(edit.fontItalic(), true);
> > QCOMPARE(edit.fontWeight(), static_cast(QFont::Bold));
> > QCOMPARE(edit.fontUnderline(), true);
> > charFormat = cursor.charFormat();
> > QCOMPARE(charFormat.foreground(),
> QBrush(KColorScheme(QPalette::Active,
> KColorScheme::View).foreground(KColorScheme::LinkText).color()));
> > QCOMPARE(charFormat.underlineColor(), KColorScheme(QPalette::Active,
> KColorScheme::View).foreground(KColorScheme::LinkText).color());
> > QCOMPARE(charFormat.underlineStyle(),
> QTextCharFormat::SingleUnderline);
> > }
> >
> > --
> > David Faure, fa...@kde.org, http://www.davidfaure.fr
> > Working on KDE Frameworks 5
> >
> >
> >
>
> Cheers,
> Ben
>

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Faure]

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

[Kai Uwe Broulik]

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

[Kai Uwe Broulik]

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 we render no thumbnail 
whatsoever
  
  CCBUG: 397220

TEST PLAN
  Ran `QT_QUICK_BACKEND=software systemsettings5` I can now see the grid 
delegates again, albeit with no shadow or border around it

REPOSITORY
  R296 KDeclarative

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

AFFECTED FILES
  src/qmlcontrols/kcmcontrols/qml/GridDelegate.qml

To: broulik, #plasma, mart
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Hallas]

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 
risk introducing leaks again? Do we have any favorite smart pointer for this? 
Or would a std::unique_ptr be fine?

REPOSITORY
  R241 KIO

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

To: hallas, dfaure
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Faure]

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 merged with the next line, by doing

  delete d->itemList.takeLast();

> kurlcombobox.cpp:363
>  if (url.toString(QUrl::StripTrailingSlash) == 
> mit.value()->url.toString(QUrl::StripTrailingSlash)) {
>  if (!d->itemList.removeAll(mit.value()) && checkDefaultURLs) {
>  d->defaultList.removeAll(mit.value());

Doesn't this have the same issue? delete missing

> kurlcombobox.cpp:364
>  if (!d->itemList.removeAll(mit.value()) && checkDefaultURLs) {
>  d->defaultList.removeAll(mit.value());
>  }

same here, right?

REPOSITORY
  R241 KIO

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

To: hallas, dfaure
Cc: dfaure, kde-frameworks-devel, michaelh, ngraham, bruns

D14666: Fixes memory leak in KUrlComboBox::setUrl

[David Hallas]

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 multiple times it first removes the previous item
  but fails to delete the item before removing it causing a memory leak.

TEST PLAN
  Unit test

REPOSITORY
  R241 KIO

BRANCH
  master

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

AFFECTED FILES
  autotests/kurlcomboboxtest.cpp
  autotests/kurlcomboboxtest.h
  src/widgets/kurlcombobox.cpp

To: hallas
Cc: kde-frameworks-devel, michaelh, ngraham, bruns