KDE CI: Frameworks plasma-framework kf5-qt5 XenialQt5.7 - Build # 127 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20plasma-framework%20kf5-qt5%20XenialQt5.7/127/
 Project:
Frameworks plasma-framework kf5-qt5 XenialQt5.7
 Date of build:
Tue, 24 Oct 2017 03:09:10 +
 Build duration:
35 min and counting
   JUnit Tests
  Name: (root) Failed: 7 test(s), Passed: 8 test(s), Skipped: 0 test(s), Total: 15 test(s)Failed: TestSuite.dialognativetestFailed: TestSuite.plasma-configmodeltestFailed: TestSuite.plasma-dialogqmltestFailed: TestSuite.plasma-fallbackpackagetestFailed: TestSuite.plasma-iconitemtestFailed: TestSuite.plasma-packagestructuretestFailed: TestSuite.plasma-storagetest
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report86%
(6/7)62%
(57/92)62%
(57/92)38%
(3499/9230)26%
(1859/7156)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests100%
(22/22)100%
(22/22)76%
(605/795)38%
(390/1028)src.declarativeimports.core57%
(4/7)57%
(4/7)28%
(246/883)14%
(86/620)src.plasma62%
(13/21)62%
(13/21)40%
(1420/3581)29%
(773/2697)src.plasma.private46%
(11/24)46%
(11/24)39%
(649/1649)28%
(302/1080)src.plasma.scripting0%
(0/3)0%
(0/3)0%
(0/190)0%
(0/126)src.plasmaquick50%
(6/12)50%
(6/12)27%
(548/2019)19%
(302/1583)src.plasmaquick.private33%
(1/3)33%
(1/3)27%
(31/113)27%
(6/22)

KDE CI: Frameworks plasma-framework kf5-qt5 FreeBSDQt5.7 - Build # 131 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20plasma-framework%20kf5-qt5%20FreeBSDQt5.7/131/
 Project:
Frameworks plasma-framework kf5-qt5 FreeBSDQt5.7
 Date of build:
Tue, 24 Oct 2017 03:09:10 +
 Build duration:
34 min and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 13 test(s), Skipped: 0 test(s), Total: 14 test(s)Failed: TestSuite.plasma-packagestructuretest

KDE CI: Frameworks ktexteditor kf5-qt5 FreeBSDQt5.7 - Build # 87 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20ktexteditor%20kf5-qt5%20FreeBSDQt5.7/87/
 Project:
Frameworks ktexteditor kf5-qt5 FreeBSDQt5.7
 Date of build:
Tue, 24 Oct 2017 03:08:48 +
 Build duration:
29 min and counting
   JUnit Tests
  Name: (root) Failed: 3 test(s), Passed: 63 test(s), Skipped: 0 test(s), Total: 66 test(s)Failed: TestSuite.commands_testFailed: TestSuite.kateindenttest_testCstyleFailed: TestSuite.scripting_test

KDE CI: Frameworks kirigami kf5-qt5 FreeBSDQt5.7 - Build # 126 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20kirigami%20kf5-qt5%20FreeBSDQt5.7/126/
 Project:
Frameworks kirigami kf5-qt5 FreeBSDQt5.7
 Date of build:
Tue, 24 Oct 2017 03:08:12 +
 Build duration:
10 min and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 0 test(s), Skipped: 0 test(s), Total: 1 test(s)Failed: TestSuite.qmltests

KDE CI: Frameworks kirigami kf5-qt5 XenialQt5.7 - Build # 132 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20kirigami%20kf5-qt5%20XenialQt5.7/132/
 Project:
Frameworks kirigami kf5-qt5 XenialQt5.7
 Date of build:
Tue, 24 Oct 2017 03:08:12 +
 Build duration:
2 min 2 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 2 test(s)Failed: TestSuite.qmltests
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report100%
(2/2)67%
(6/9)67%
(6/9)64%
(409/640)43%
(169/393)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalssrc80%
(4/5)80%
(4/5)54%
(173/320)34%
(90/263)src.libkirigami50%
(2/4)50%
(2/4)74%
(236/320)61%
(79/130)

D7828: createKMessageBox tries to focus a default button when available

[Nathaniel Graham]

ngraham added a comment.


  @aacid sorry to be unclear.
  
  To most people, the default button is the one that's visually distinct. In 
Breeze, that generally is the one with a blue background.
  
  Without this patch, the *actual* default button is virtually 
indistinguishable from other buttons when it doesn't have focus. It looks like 
this, with only a slightly lighter background than the other buttons:
  
  F5446387: default wrong.png 
  
  With the patch, the default button gets focus, so it looks like a default 
button should (IMHO):
  
  F5446389: default correct.png 
  
  IMHO it's more important to put the focus where it will cause the button to 
look correct than on an unrelated UI element.

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

To: emateli, #frameworks, ngraham, aacid
Cc: ngraham, aacid, #frameworks

D7175: Redesign CMake syntax

[Alex Turbov]

turbov updated this revision to Diff 21208.
turbov added a comment.


  Include updates for CMake 3.10

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7175?vs=21207=21208

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

AFFECTED FILES
  data/generators/cmake-gen.sh
  data/generators/cmake.xml.tpl
  data/generators/cmake.yaml
  data/generators/generate-cmake-syntax.py
  data/syntax/cmake.xml

To: turbov, dhaumann, #kate, #framework_syntax_highlighting, vkrause
Cc: cullmann, #frameworks

D7175: Redesign CMake syntax

[Alex Turbov]

turbov updated this revision to Diff 21207.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7175?vs=18322=21207

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

AFFECTED FILES
  data/generators/cmake.yaml
  data/generators/generate-cmake-syntax.py
  data/syntax/cmake.xml

To: turbov, dhaumann, #kate, #framework_syntax_highlighting, vkrause
Cc: cullmann, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Albert Astals Cid]

aacid added a comment.


  In https://phabricator.kde.org/D7828#159054, @ngraham wrote:
  
  > There actually is a bug being fixed here in that currently many buttons 
marked default aren't actually getting made default buttons in that they turn 
blue and pressing enter will click them. This fixes that.
  
  
  I don't understand this sentence, can you rewrite it and/or give an example?

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

To: emateli, #frameworks, ngraham, aacid
Cc: ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Albert Astals Cid]

aacid requested changes to this revision.
aacid added a comment.
This revision now requires changes to proceed.


  Marking as request changes since i'd like someone with UI expertise to 
confirm this is what we want since from the code point of view default = focus 
is not what the default property for buttons means.

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

To: emateli, #frameworks, ngraham, aacid
Cc: ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Albert Astals Cid]

aacid added a comment.


  Sorry to be blunt, but your opinion doesn't matter.
  
  Here is the definition of the default property for a button.
  
  http://doc.qt.io/qt-5/qpushbutton.html#default-prop
  
  Nowhere in it says "this is the button that should have focus when the dialog 
pops up".

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

To: emateli, #frameworks, ngraham
Cc: ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Emirald Mateli]

emateli added a comment.


  In my opinion a "Default" widget, should also have focus. Makes little sense 
to have some widget focused and then pressing enter to trigger the action of 
another one. 
  By focusing, the default action is also made visible and allows to invoke the 
it action by pressing space bar. And no, I haven't spoken to the usability 
team. What is the team name so we can tag them here perhaps?
  
  After running some tests even when manually setting some other action to 
default, enter always invokes the same action. Eg: in the case of a Yes, No, 
Cancel dialog, the yes will always be called regardless.

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

To: emateli, #frameworks, ngraham
Cc: ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Nathaniel Graham]

ngraham added a comment.


  There actually is a bug being fixed here in that currently many buttons 
marked default aren't actually getting made default buttons in that they turn 
blue and pressing enter will click them. This fixes that.

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

To: emateli, #frameworks, ngraham
Cc: ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Albert Astals Cid]

aacid added a comment.


  Sorry but after reading what the patch does i'm not sure this makes sense.
  
  Default button just means "the button that is triggered when pressing Enter", 
it has nothing to do with "this is the button that should have focus".
  
  Have you talked with the usability people about this?

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

To: emateli, #frameworks, ngraham
Cc: ngraham, aacid, #frameworks

D8436: use a single QML engine

[Marco Martin]

mart added inline comments.

INLINE COMMENTS

> davidedmundson wrote in configmodule.cpp:106
> !!

right, since it keeps track of attached properties in this way, either gets 
changed or is not possible to use a single engine, damn :/

hmm, maybe using the rootcontext instead would work

REPOSITORY
  R296 KDeclarative

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

To: mart, #plasma
Cc: davidedmundson, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D8056: Improve usability of "Open With" dialog by adding option to filter the application tree

[Andres Betts]

abetts added a comment.


  Can you guys add a quick picture of what this looks like right now?

REPOSITORY
  R241 KIO

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

To: simgunz, dfaure, #frameworks, #vdg, ngraham
Cc: abetts, ngraham, alexeymin, #frameworks

D8056: Improve usability of "Open With" dialog by adding option to filter the application tree

[Nathaniel Graham]

ngraham added a comment.


  Sounds great, thanks!

REPOSITORY
  R241 KIO

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

To: simgunz, dfaure, #frameworks, #vdg, ngraham
Cc: ngraham, alexeymin, #frameworks

D8437: KWidgetsAddons : more compact password dialog

[René J . V . Bertin]

rjvbb added a comment.


  > Is this change intended?
  
  I doubt that (it's been a while). Probably something Designer added, and I 
can't imagine it would have an impact on packing. I'll check.

REPOSITORY
  R236 KWidgetsAddons

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

To: rjvbb, #frameworks
Cc: cfeck, ngraham

D1231: Add Remote Access interface to KWayland

[Roman Gilg]

subdiff added a task: T5653: [kwin] Screen recording in Wayland session.

REPOSITORY
  R127 KWayland

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

To: Kanedias, graesslin, davidedmundson
Cc: subdiff, ngraham, alexeymin, #frameworks, davidedmundson, plasma-devel, 
leezu, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
eliasp, sebas, apol, mart, hein

D8416: really raising window when shown from systray

[Martin Flöser]

graesslin added inline comments.

INLINE COMMENTS

> mkoller wrote in kstatusnotifieritem.cpp:653
> "without checking who sends it"
> 
> Who shall be allowed to do so ?
> 
> As said, the original problem is that a click in SysTray does not raise it,
> but as I understand it, the KStatusNotifierItem is owned by the application 
> which receives the "Activate" trigger sent via DBus, so it would be the 
> application which would raise itself. Am I right ?

Yes you got that right. And yes: if the systray would use the force active 
window request it would've fine and correct. From X11 perspective it is a tool 
which activated windows of other applications. Just like e.g. the Taskmanager. 
And it is the window which was interacted with. So from focus stealing 
prevention everything is fine.

If that StatusNotifier on the other hand activated itself it looks like focus 
stealing to the window manager.

REPOSITORY
  R289 KNotifications

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

To: mkoller, davidedmundson, graesslin
Cc: #frameworks

KDE CI: Frameworks kcoreaddons kf5-qt5 FreeBSDQt5.7 - Build # 53 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20kcoreaddons%20kf5-qt5%20FreeBSDQt5.7/53/
 Project:
Frameworks kcoreaddons kf5-qt5 FreeBSDQt5.7
 Date of build:
Mon, 23 Oct 2017 17:44:16 +
 Build duration:
3 min 9 sec and counting
   JUnit Tests
  Name: (root) Failed: 2 test(s), Passed: 21 test(s), Skipped: 0 test(s), Total: 23 test(s)Failed: TestSuite.kdirwatch_inotify_unittestFailed: TestSuite.kdirwatch_stat_unittest

D8401: fix URL detection with double urls like "http://www.foo.bar"

[Martin Koller]

This revision was automatically updated to reflect the committed changes.
Closed by commit R244:095a75eeb212: fix URL detection with double urls like 
http://www.foo.barhttp://foo.bar/; (authored by mkoller).

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8401?vs=21067=21193

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

AFFECTED FILES
  autotests/ktexttohtmltest.cpp
  src/lib/text/ktexttohtml.cpp

To: mkoller, mlaurent
Cc: #frameworks

D8416: really raising window when shown from systray

[Martin Koller]

mkoller added inline comments.

INLINE COMMENTS

> graesslin wrote in kstatusnotifieritem.cpp:653
> It only works on X11 because KWin has a bug. KWin allows the force (which is 
> in X11 Terms a "from tool" request) without checking who sends it. I will 
> look into hardening this in KWin. Then the existing code won't work anymore.

"without checking who sends it"

Who shall be allowed to do so ?

As said, the original problem is that a click in SysTray does not raise it,
but as I understand it, the KStatusNotifierItem is owned by the application 
which receives the "Activate" trigger sent via DBus, so it would be the 
application which would raise itself. Am I right ?

REPOSITORY
  R289 KNotifications

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

To: mkoller, davidedmundson, graesslin
Cc: #frameworks

KDE CI: Frameworks kirigami kf5-qt5 XenialQt5.7 - Build # 131 - Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20kirigami%20kf5-qt5%20XenialQt5.7/131/
 Project:
Frameworks kirigami kf5-qt5 XenialQt5.7
 Date of build:
Mon, 23 Oct 2017 17:09:46 +
 Build duration:
1 min 31 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 2 test(s)Failed: TestSuite.qmltests
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report100%
(2/2)67%
(6/9)67%
(6/9)64%
(409/640)43%
(169/393)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalssrc80%
(4/5)80%
(4/5)54%
(173/320)34%
(90/263)src.libkirigami50%
(2/4)50%
(2/4)74%
(236/320)61%
(79/130)

KDE CI: Frameworks kirigami kf5-qt5 FreeBSDQt5.7 - Build # 125 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20kirigami%20kf5-qt5%20FreeBSDQt5.7/125/
 Project:
Frameworks kirigami kf5-qt5 FreeBSDQt5.7
 Date of build:
Mon, 23 Oct 2017 17:09:46 +
 Build duration:
38 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 0 test(s), Skipped: 0 test(s), Total: 1 test(s)Failed: TestSuite.qmltests

D8437: KWidgetsAddons : more compact password dialog

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> kpassworddialog.ui:137
> +
> + true
> +

Is this change intended?

REPOSITORY
  R236 KWidgetsAddons

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

To: rjvbb, #frameworks
Cc: cfeck, ngraham

D8437: KWidgetsAddons : more compact password dialog

[Nathaniel Graham]

ngraham added a comment.


  Fantastic. Can you add the following onto its own line in the Summary section?
  
  "BUG: 381231"
  
  That will cause that bug to get automatically closed once this is merged.

REPOSITORY
  R236 KWidgetsAddons

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

To: rjvbb, #frameworks
Cc: ngraham

D8437: KWidgetsAddons : more compact password dialog

[René J . V . Bertin]

rjvbb added a comment.


  Screenshots attached to the bug report I filed about this 
(https://bugs.kde.org/show_bug.cgi?id=381231):
  
  Before:
  F5445829: kwallet-pw-dialog.png 
  
  After (packed):
  F5445832: kwallet-pw-dialog-packed.png 

REPOSITORY
  R236 KWidgetsAddons

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

To: rjvbb, #frameworks

D8437: KWidgetsAddons : more compact password dialog

[René J . V . Bertin]

rjvbb created this revision.
rjvbb added a reviewer: Frameworks.
rjvbb added a project: Frameworks.

REVISION SUMMARY
  When kwalletd4 was replaced by kwalletd5 I quickly got annoyed by the so much 
and unnecessarily larger password dialog. This patch addresses that issues, 
maing the dialog comparably compact as it was under KDE4.

TEST PLAN
  I've been using this patch since before summer, on Mac and Linux. I never 
encountered a password dialog that became unusable or looked wrong.

REPOSITORY
  R236 KWidgetsAddons

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

AFFECTED FILES
  src/kpassworddialog.ui

To: rjvbb, #frameworks

D8387: Workaround incorrectly returned EEXIST instead of EPERM regression introduced by libsmbclient 4.7

[Michal Malý]

madcatx added inline comments.

INLINE COMMENTS

> cfeck wrote in kio_smb_browse.cpp:521
> While it does no harm, you do not need 'const' for plain types, such as 'int'.

Don't worry, I'm well aware. I however like to use consts wherever possible to 
make it clear that changing a value of such variable makes no logical sense: 
It's a way of telling "You shouldn't ever have to change this and if you do, 
perhaps you should think again about what you're trying to do."

REPOSITORY
  R320 KIO Extras

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

To: madcatx, ngraham, davidedmundson, elvisangelaccio, #frameworks
Cc: cfeck, rdieter, graesslin, z3ntu

D8056: Improve usability of "Open With" dialog by adding option to filter the application tree

[Simone Gaiarin]

simgunz added a comment.


  In https://phabricator.kde.org/D8056#158939, @ngraham wrote:
  
  > @simgunz, have you had a chance to try implementing David's suggestion?
  
  
  Yes. Everything seems to work but I haven't pushed yesterday because I wanted 
to do an extra test.
  
  The current behavior is the following:
  
  - Down arrow pass the focus to the QTreeView and moves the selection down by 
one. (alternatively we can just pass the focus without changing the selection).
  - Up arrow is used to go back in the history of the combobox (as the current 
behavior)
  - Making down arrow change the focus, partially breaks it functionality of 
navigating the history of the combo box.
  - If the filter doesn't match anything, down arrow doesn't change the focus 
and works for navigating the history
  
  - The feature is not enabled if PopupAutocomplete is enabled. Theoretically 
it should only be disabled while the popup is shown, but I couldn't find a way 
to detect this state.
  
  Moreover the first match is selected by default and hitting enter calls it. 
To enable this feature I had to change the behaviour so that the text box is 
not filled anymore with the command corresponding to the selected desktop 
entry. For me this is even cleaner from the UI point of view.
  
  I'll push tomorrow evening.

REPOSITORY
  R241 KIO

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

To: simgunz, dfaure, #frameworks, #vdg, ngraham
Cc: ngraham, alexeymin, #frameworks

D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)

[René J . V . Bertin]

rjvbb set the repository for this revision to R245 Solid.

REPOSITORY
  R245 Solid

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

To: rjvbb, #frameworks, kfunk
Cc: kfunk, anthonyfieroni, cgilles, kde-mac

D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)

[René J . V . Bertin]

rjvbb updated this revision to Diff 21187.
rjvbb marked 7 inline comments as done.
rjvbb added a comment.


  Updated as requested/discussed.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7401?vs=18366=21187

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

AFFECTED FILES
  src/solid/devices/CMakeLists.txt
  src/solid/devices/backends/iokit/CMakeLists.txt
  src/solid/devices/backends/iokit/cfhelper.cpp
  src/solid/devices/backends/iokit/iokitbattery.cpp
  src/solid/devices/backends/iokit/iokitbattery.h
  src/solid/devices/backends/iokit/iokitblock.cpp
  src/solid/devices/backends/iokit/iokitblock.h
  src/solid/devices/backends/iokit/iokitdevice.cpp
  src/solid/devices/backends/iokit/iokitdevice.h
  src/solid/devices/backends/iokit/iokitdeviceinterface.cpp
  src/solid/devices/backends/iokit/iokitdeviceinterface.h
  src/solid/devices/backends/iokit/iokitgenericinterface.cpp
  src/solid/devices/backends/iokit/iokitmanager.cpp
  src/solid/devices/backends/iokit/iokitopticaldisc.cpp
  src/solid/devices/backends/iokit/iokitopticaldisc.h
  src/solid/devices/backends/iokit/iokitopticaldrive.cpp
  src/solid/devices/backends/iokit/iokitopticaldrive.h
  src/solid/devices/backends/iokit/iokitprocessor.cpp
  src/solid/devices/backends/iokit/iokitprocessor.h
  src/solid/devices/backends/iokit/iokitstorage.cpp
  src/solid/devices/backends/iokit/iokitstorage.h
  src/solid/devices/backends/iokit/iokitstorageaccess.cpp
  src/solid/devices/backends/iokit/iokitstorageaccess.h
  src/solid/devices/backends/iokit/iokitvolume.cpp
  src/solid/devices/backends/iokit/iokitvolume.h

To: rjvbb, #frameworks, kfunk
Cc: kfunk, anthonyfieroni, cgilles, kde-mac

D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)

[René J . V . Bertin]

rjvbb marked 27 inline comments as done.
rjvbb added inline comments.

INLINE COMMENTS

> kfunk wrote in iokitblock.h:43
> Here and below: Missing `Q_DECL_OVERRIDE`

I hope you meant everywhere, including in old code...

> kfunk wrote in iokitdevice.cpp:156
> `new`/`delete` mismatch. Use `delete[]`

(This keeps biting me. Even C doesn't have separate de/allocators for pointers 
to scalars and pointers to arrays ...)

> kfunk wrote in iokitdevice.cpp:171
> That's *very* odd style. Why does a private class delete its public 
> counterpart? I've never seen this.

Heh, that's because there is (was) a confusion in parenthood here. The member 
var didn't point to the private class parent, but holds a reference to the 
parent of the current IOKit device. Currently it's used and thus allocated only 
when getting the device icon.

> kfunk wrote in iokitdevice.cpp:197
> Don't leave commented code around. Either enable this code paths properly via 
> categorized logging or remove it altogether.

I'd love to, but Solid doesn't have any modern logging set up. Rather than 
introducing that through the back(end) door, wouldn't it be better if this were 
done for all of Solid? (Preferably by someone having a good overview of the 
entire framework...)

https://bugs.kde.org/show_bug.cgi?id=386107

> kfunk wrote in iokitopticaldrive.cpp:27
> Where's that defined via the build system?

Nowhere currently. It's somewhat experimental code which uses the 
DiskArbitration SDK for ejection, instead of invoking an external (system) 
command.

It works (and it would thus be a pity to throw everything away) but as 
documented in the comment, there are a few issues that I haven't been able to 
iron out.

Making this a build option would certainly increase its visibility and thus 
(hopefully) incite some other Apple users to test it. Should I put one under 
the WITH_NEW_* options in the toplevel CMake file?

> kfunk wrote in iokitstorage.cpp:36
> `nullptr`

Did you check that these are indeed pointers? ;)

> kfunk wrote in iokitstorage.cpp:75
> Please just remove the constructors taking a `const IOKitDevice *device` and 
> adapt uses (just use `IOKitDevice *device` everywhere). It's unusual for 
> pointer types to be `const` in such contexts. It just adds noise.

I don't get it, sorry, can you explain in more words how you'd want to see this 
changed? If I remove this extra ctor, I can no longer call 
`IOKitStorage(this).vendor()` in `IOKitDevice::vendor()` without extra code 
that's also going to add noise.

I get a warning when I remove the const attribute from `IOKitDevice::vendor()`, 
which suggests that I'd no longer be reimplementing a virtual method but adding 
a method instead.

All these "extra" ctors hand off the pointer to a "const this" to 
`DeviceInterface` which finally makes a deep copy. I find that cleaner than 
creating a deep copy of `this` everywhere needed and ensuring it gets 
deallocated (even via QPointers).
Unusual doesn't mean wrong (we're on Mac here, after all ^^)

> kfunk wrote in iokitstorageaccess.cpp:91
> Early-return would reduce the indentation level here [1].
> 
>   if (errorCase)
> // return early
> return {};
>   }
>   
>   // normal case
>   // ...
> 
> [1] 
> https://softwareengineering.stackexchange.com/questions/18454/should-i-return-from-a-function-early-or-use-an-if-statement

Compromise. Too many return points make functions hard to maintain (Apple code 
tends to be full of `goto bail;` instructions because of that; we're far here 
from the nesting you'd get without those in code using the QT SDK.)

> kfunk wrote in iokitvolume.cpp:177
> That's a lot of copy-pasted code amongst `fsType`, `label`, `vendor`, 
> `product, `description`. I'm sure that can be done better.

Somewhat.

REPOSITORY
  R245 Solid

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

To: rjvbb, #frameworks, kfunk
Cc: kfunk, anthonyfieroni, cgilles, kde-mac

D8387: Workaround incorrectly returned EEXIST instead of EPERM regression introduced by libsmbclient 4.7

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> kio_smb_browse.cpp:521
>  
> +bool SMBSlave::workaroundEEXIST(const int errNum) const
> +{

While it does no harm, you do not need 'const' for plain types, such as 'int'.

REPOSITORY
  R320 KIO Extras

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

To: madcatx, ngraham, davidedmundson, elvisangelaccio, #frameworks
Cc: cfeck, rdieter, graesslin, z3ntu

D8056: Improve usability of "Open With" dialog by adding option to filter the application tree

[Nathaniel Graham]

ngraham added a comment.


  @simgunz, have you had a chance to try implementing David's suggestion?

REPOSITORY
  R241 KIO

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

To: simgunz, dfaure, #frameworks, #vdg, ngraham
Cc: ngraham, alexeymin, #frameworks

D8056: Improve usability of "Open With" dialog by adding option to filter the application tree

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R241 KIO

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

To: simgunz, dfaure, #frameworks, #vdg, ngraham
Cc: ngraham, alexeymin, #frameworks

D8436: use a single QML engine

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> configmodule.cpp:106
>  {
>  ConfigModulePrivate::s_rootObjects.remove(d->_qmlObject->engine());
>  

!!

REPOSITORY
  R296 KDeclarative

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

To: mart, #plasma
Cc: davidedmundson, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D8436: use a single QML engine

[Marco Martin]

mart created this revision.
mart added a reviewer: Plasma.
Restricted Application added projects: Plasma, Frameworks.
Restricted Application added subscribers: Frameworks, plasma-devel.

REVISION SUMMARY
  use a shared qml engine for all ConfigModules in the same process
  this avoids some crashes as creating and deleting stuff from
  different engines is known to cause crashes in the v4 garbage collection
  and is recomended to use a single QQmlEngine per process

TEST PLAN
  normal SystemSettings seems unaffected, plasma mobile systemsettings
  which is qml-only now doesn't crash anymore after loading a module
  for the second time

REPOSITORY
  R296 KDeclarative

BRANCH
  phab/singleengine

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

AFFECTED FILES
  src/quickaddons/configmodule.cpp

To: mart, #plasma
Cc: plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D8433: Created 'shared' section

[Renato Oliveira Filho]

renatoo abandoned this revision.
renatoo added a comment.


  In https://phabricator.kde.org/D8433#158886, @ngraham wrote:
  
  > Looks like this was opened by accident; can you abandon it?
  
  
  Ok sorry did not notice that two merge reguest was creted.

REPOSITORY
  R241 KIO

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

To: renatoo
Cc: ngraham, #frameworks

D8433: Created 'shared' section

[Renato Oliveira Filho]

renatoo added a comment.


  In https://phabricator.kde.org/D8433#158886, @ngraham wrote:
  
  > Looks like this was opened by accident; can you abandon it?
  
  
  No this is a valid change? I would like to proposal that one. :D

REPOSITORY
  R241 KIO

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

To: renatoo
Cc: ngraham, #frameworks

D8433: Created 'shared' section

[Nathaniel Graham]

ngraham added a comment.


  Looks like this was opened by accident; can you abandon it?

REPOSITORY
  R241 KIO

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

To: renatoo
Cc: ngraham, #frameworks

D8243: Implement support for categories on KfilesPlacesView

[Nathaniel Graham]

ngraham added a comment.


  @dfaure and @ervin, is this acceptable now?

REPOSITORY
  R241 KIO

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

To: renatoo, #frameworks, dfaure, ervin, #vdg
Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks

D8434: Created 'shared' section

[Renato Oliveira Filho]

renatoo added reviewers: ngraham, Frameworks, Dolphin.

REPOSITORY
  R241 KIO

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

To: renatoo, ngraham, #frameworks, #dolphin
Cc: #frameworks

D8367: Hidding place groups implementation in KFilePlacesModel

[Franck Arrecot]

franckarrecot updated this revision to Diff 21177.
franckarrecot added a comment.


  Moving enum from item to model to avoid exposing private api

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8367?vs=21167=21177

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

AFFECTED FILES
  autotests/kfileplacesmodeltest.cpp
  src/filewidgets/kfileplacesitem.cpp
  src/filewidgets/kfileplacesitem_p.h
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesmodel.h

To: franckarrecot, renatoo, mlaurent, ngraham
Cc: ngraham, mlaurent, #frameworks

D8434: Created 'shared' section

[Renato Oliveira Filho]

renatoo edited the test plan for this revision.

REPOSITORY
  R241 KIO

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

To: renatoo
Cc: #frameworks

D8434: Created 'shared' section

[Renato Oliveira Filho]

renatoo created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  Group all network related urls into the 'shared' section, to make it
  clear to the user what is access through network or not

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  autotests/kfileplacesmodeltest.cpp
  src/filewidgets/kfileplacesitem.cpp
  src/filewidgets/kfileplacesitem_p.h

To: renatoo
Cc: #frameworks

D8433: Created 'shared' section

[Renato Oliveira Filho]

renatoo created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  Group all network related urls into the 'shared' section, to make it
  clear to the user what is access through network or not

TEST PLAN
  From any kde app try to open/save a file.
  It should show a new file dialog with network ulrs under the 'shared' section

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  autotests/kfileplacesmodeltest.cpp
  src/filewidgets/kfileplacesitem.cpp
  src/filewidgets/kfileplacesitem_p.h

To: renatoo
Cc: #frameworks

D1231: Add Remote Access interface to KWayland

[Roman Gilg]

subdiff added inline comments.

INLINE COMMENTS

> Kanedias wrote in remote_access_interface.cpp:206
> > Can a rogue client do it though? This would crash the server then?
> 
> Yes, I guess so... What would you propose? Should we send it only to first 
> bound? Or last one?
> 
> P.S. Even more: this interface has no authentication/authorization at all, so 
> any client can connect and steal our video buffers.
> Martin said that first version of protocol can go without it and we can readd 
> it later (as with fakeinput protocol).

Only first bound like you do it now. Just remove the Q_ASSERT (and make sure 
`boundScreens.size() >= 1`, otherwise continue).

> no authentication/authorization at all

That's a generic problem yet to be solved on Wayland / the Linux desktop. This 
also correlates with the push to containerized apps. I would just want 
something like the permission system in Android, but there might be better 
solutions. It's a bigger project for sure.

Also see here for some early thoughts on it, which to my knowledge until now 
did not lead to anything more: 
http://www.mupuf.org/blog/2014/02/19/wayland-compositors-why-and-how-to-handle/

REPOSITORY
  R127 KWayland

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

To: Kanedias, graesslin, davidedmundson
Cc: subdiff, ngraham, alexeymin, #frameworks, davidedmundson, plasma-devel, 
leezu, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Oleg Chernovskiy]

Kanedias added inline comments.

INLINE COMMENTS

> subdiff wrote in remote_access_interface.cpp:206
> Can a rogue client do it though? This would crash the server then?

> Can a rogue client do it though? This would crash the server then?

Yes, I guess so... What would you propose? Should we send it only to first 
bound? Or last one?

P.S. Even more: this interface has no authentication/authorization at all, so 
any client can connect and steal our video buffers.
Martin said that first version of protocol can go without it and we can readd 
it later (as with fakeinput protocol).

REPOSITORY
  R127 KWayland

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

To: Kanedias, graesslin, davidedmundson
Cc: subdiff, ngraham, alexeymin, #frameworks, davidedmundson, plasma-devel, 
leezu, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
eliasp, sebas, apol, mart, hein

D1231: Add Remote Access interface to KWayland

[Roman Gilg]

subdiff added inline comments.

INLINE COMMENTS

> remote_access_interface.cpp:206
> +// no reason for client to bind wl_output multiple times
> +Q_ASSERT(boundScreens.size() == 1);
> +org_kde_kwin_remote_access_manager_send_buffer_ready(res, buf->fd(), 
> boundScreens[0]);

Can a rogue client do it though? This would crash the server then?

REPOSITORY
  R127 KWayland

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

To: Kanedias, graesslin, davidedmundson
Cc: subdiff, ngraham, alexeymin, #frameworks, davidedmundson, plasma-devel, 
leezu, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
eliasp, sebas, apol, mart, hein

D8332: Added baloo urls into places model

[Renato Oliveira Filho]

renatoo added a comment.


  In https://phabricator.kde.org/D8332#158392, @ngraham wrote:
  
  > @dvratil, any remaining objections? This looks great to me.
  >
  > @renatoo, once this is in, can we remove the now-duplicated code from 
Dolphin?
  
  
  Yes the idea is to try to use at least the model on Dolphin.

REPOSITORY
  R241 KIO

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

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham
Cc: usta, mlaurent, dvratil, ngraham, #frameworks

D8243: Implement support for categories on KfilesPlacesView

[Renato Oliveira Filho]

renatoo marked 3 inline comments as done.

REPOSITORY
  R241 KIO

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

To: renatoo, #frameworks, dfaure, ervin, #vdg
Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks

D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)

[Kevin Funk]

kfunk added inline comments.

INLINE COMMENTS

> iokitopticaldrive.cpp:27
> +
> +#ifdef EJECT_USING_DISKARBITRATION
> +// for QCFType:

Where's that defined via the build system?

REPOSITORY
  R245 Solid

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

To: rjvbb, #frameworks, kfunk
Cc: kfunk, anthonyfieroni, cgilles, kde-mac

D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)

[Kevin Funk]

kfunk requested changes to this revision.
kfunk added a comment.
This revision now requires changes to proceed.


  This definitely needs another revision. Please fix the outstanding issues.

INLINE COMMENTS

> iokitblock.h:43
> +
> +virtual int deviceMajor() const;
> +virtual int deviceMinor() const;

Here and below: Missing `Q_DECL_OVERRIDE`

> iokitdevice.cpp:151
> +size_t size = 0;
> +if (sysctlbyname("hw.model", NULL, , NULL, 0) == 0 && size > 0) {
> +model = new char [size];

Use `nullptr` everywhere

> iokitdevice.cpp:154
> +if (sysctlbyname("hw.model", model, , NULL, 0) == 0) {
> +qModel= QLatin1String(model);
> +}

Style: Use `qModel = ...`

> iokitdevice.cpp:156
> +}
> +delete model;
> +}

`new`/`delete` mismatch. Use `delete[]`

> iokitdevice.cpp:167
> +{
> +type << Solid::DeviceInterface::Unknown;
> +}

Initialize at class member decl

> iokitdevice.cpp:171
> +{
> +if (parent) {
> +delete parent;

That's *very* odd style. Why does a private class delete its public 
counterpart? I've never seen this.

> iokitdevice.cpp:197
> +type = typesFromEntry(entry, properties, mainType);
> +// if (udi.contains(QStringLiteral("IOBD")) || 
> udi.contains(QStringLiteral("BD PX"))) {
> +// qWarning() << "Solid: BlueRay entry" << entry << "mainType=" << 
> mainType << "typeList:" << type

Don't leave commented code around. Either enable this code paths properly via 
categorized logging or remove it altogether.

> iokitopticaldrive.cpp:211
> +}
> +delete ioDVDServices;
> +d = new Private(device, devCharMap);

Create on `ioDVDServices` on the stack to begin with?

> iokitopticaldrive.cpp:333
> +{
> +QList speeds;
> +return speeds;

`return {}`

> iokitopticaldrive.cpp:354
> +return true;
> +} else {
> +emit ejectDone(Solid::ErrorType::OperationFailed, QVariant(), 
> m_device->udi());

Coding style: Would turn around this two branches (and remove the else part) to 
make the intended meaning more clear:

Pseudo code:

  if (errorCase) {
return ...;
  }
  
  // normal case, code flow continues
  return ...;

That's usually the common style

> iokitprocessor.cpp:84
> +
> +const QString Processor::vendor()
> +{

Remove `const`

> iokitprocessor.cpp:87
> +QString qVendor = QString();
> +char *vendor = NULL;
> +size_t size = 0;

Way too much error prone `new`/`delete` logic on naked char arrays. Every 
invocation here is wrong (causing undefined behavior) due to the 
`new[]`/delete` mismatch mentioned above.

Factor that out reading into a `QString` into a separate function, cf. 
https://github.com/trueos/lumina/blob/master/src-qt5/core/libLumina/LuminaOS-DragonFly.cpp#L29
 and use it everywhere instead.

> iokitprocessor.cpp:99
>  
> +const QString Processor::product()
> +{

Remove `const`

> iokitstorage.cpp:36
> +{
> +daRef = 0;
> +daDict = 0;

Here and one line below: Could be initialized at decl again.

> iokitstorage.cpp:36
> +{
> +daRef = 0;
> +daDict = 0;

`nullptr`

> iokitstorage.cpp:51
> +CFRelease(daDict);
> +daDict = 0;
> +}

`nullptr`, etc. pp.

> iokitstorage.cpp:75
> +
> +IOKitStorage::IOKitStorage(const IOKitDevice *device)
> +: Block(device)

Please just remove the constructors taking a `const IOKitDevice *device` and 
adapt uses (just use `IOKitDevice *device` everywhere). It's unusual for 
pointer types to be `const` in such contexts. It just adds noise.

> iokitstorageaccess.cpp:41
> +} else {
> +daRef = 0;
> +}

Here and below: `nullptr`

> iokitstorageaccess.cpp:91
> +{
> +// mount points can change (but can they between invocations of 
> filePath()?)
> +if (d->daRef) {

Early-return would reduce the indentation level here [1].

  if (errorCase)
// return early
return {};
  }
  
  // normal case
  // ...

[1] 
https://softwareengineering.stackexchange.com/questions/18454/should-i-return-from-a-function-early-or-use-an-if-statement

> iokitstorageaccess.cpp:132
> +return false;
> +} else {
> +// TODO?

Coding style: Eliminate `else` branch, just `return in function-level scope.

> iokitstorageaccess.cpp:142
> +return false;
> +} else {
> +// TODO?

Dito

> iokitvolume.cpp:42
> +} else {
> +daRef = 0;
> +}

Here and below: `nullptr`

> iokitvolume.cpp:121
> +if (dict) {
> +// qWarning() << Q_FUNC_INFO;
> +// CFShow(dict);

Remove commented code.

> iokitvolume.cpp:177
> +if (d->daRef) {
> +CFDictionaryRef dict = DADiskCopyDescription(d->daRef);
> +if (dict) {

That's a lot of copy-pasted code amongst `fsType`, `label`, `vendor`, `product, 
`description`. I'm sure that can be done better.

> iokitvolume.h:45
> +
> +virtual bool isIgnored() const;
> +virtual 

D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)

[René J . V . Bertin]

rjvbb marked 7 inline comments as done.
rjvbb added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in iokitdevice.cpp:293-294
> break after return is useless

I know, I do this as a matter of principle (and I'll leave it in since I'll 
undoubtedly be the principal maintainer of this code for  the foreseeable 
future).

> anthonyfieroni wrote in iokitmanager.cpp:95-98
> QStrinLiteral

No, not here. Check the return type and what the returned strings are used for.

> anthonyfieroni wrote in iokitopticaldrive.h:33
> I see that all classes have a virtual inheritance but i don't see they are 
> exported.

Did you see that this is also the case in the other backends, at least the ones 
I used for reference (hal and udisk)?

As I said, I'm not familiar enough with the construct to know what difference 
this would make at runtime. 
Shouldn't changing this be the focus of a different patch and review, tackling 
all backends at once?

REPOSITORY
  R245 Solid

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

To: rjvbb, #frameworks
Cc: anthonyfieroni, cgilles, kde-mac

D8367: Hidding place groups implementation in KFilePlacesModel

[Franck Arrecot]

franckarrecot updated this revision to Diff 21167.
franckarrecot edited the summary of this revision.
franckarrecot added a comment.


  sync summary

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8367?vs=21166=21167

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

AFFECTED FILES
  autotests/kfileplacesmodeltest.cpp
  src/filewidgets/kfileplacesitem.cpp
  src/filewidgets/kfileplacesitem_p.h
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesmodel.h

To: franckarrecot, renatoo, mlaurent, ngraham
Cc: ngraham, mlaurent, #frameworks

D8243: Implement support for categories on KfilesPlacesView

[Franck Arrecot]

franckarrecot added a dependent revision: D8367: Hidding place groups 
implementation in KFilePlacesModel.

REPOSITORY
  R241 KIO

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

To: renatoo, #frameworks, dfaure, ervin, #vdg
Cc: markg, ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks

D8367: Hidding place groups implementation in KFilePlacesModel

[Franck Arrecot]

franckarrecot retitled this revision from "WIP: Hidding Group from 
KFilePlacesModel" to "Hidding place groups implementation in KFilePlacesModel".
franckarrecot edited the summary of this revision.
franckarrecot edited the test plan for this revision.
franckarrecot added a reviewer: ngraham.
franckarrecot added a dependency: D8243: Implement support for categories on 
KfilesPlacesView.

REPOSITORY
  R241 KIO

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

To: franckarrecot, renatoo, mlaurent, ngraham
Cc: ngraham, mlaurent, #frameworks

D8367: WIP: Hidding Group from KFilePlacesModel

[Franck Arrecot]

franckarrecot updated this revision to Diff 21166.
franckarrecot added a comment.


  trying to make the summary changes update in phabricator

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8367?vs=21165=21166

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

AFFECTED FILES
  autotests/kfileplacesmodeltest.cpp
  src/filewidgets/kfileplacesitem.cpp
  src/filewidgets/kfileplacesitem_p.h
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesmodel.h

To: franckarrecot, renatoo, mlaurent
Cc: ngraham, mlaurent, #frameworks

D8367: WIP: Hidding Group from KFilePlacesModel

[Franck Arrecot]

franckarrecot updated this revision to Diff 21165.
franckarrecot added a comment.


  Changing the way to store group states from bookmark to root 
  propreties

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8367?vs=20974=21165

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

AFFECTED FILES
  autotests/kfileplacesmodeltest.cpp
  src/filewidgets/kfileplacesitem.cpp
  src/filewidgets/kfileplacesitem_p.h
  src/filewidgets/kfileplacesmodel.cpp
  src/filewidgets/kfileplacesmodel.h

To: franckarrecot, renatoo, mlaurent
Cc: ngraham, mlaurent, #frameworks

KDE CI: Frameworks kirigami kf5-qt5 FreeBSDQt5.7 - Build # 124 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20kirigami%20kf5-qt5%20FreeBSDQt5.7/124/
 Project:
Frameworks kirigami kf5-qt5 FreeBSDQt5.7
 Date of build:
Mon, 23 Oct 2017 09:02:10 +
 Build duration:
43 sec and counting
   JUnit Tests
  Name: (root) Failed: 1 test(s), Passed: 0 test(s), Skipped: 0 test(s), Total: 1 test(s)Failed: TestSuite.qmltests

KDE CI: Frameworks kirigami kf5-qt5 XenialQt5.7 - Build # 130 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks%20kirigami%20kf5-qt5%20XenialQt5.7/130/
 Project:
Frameworks kirigami kf5-qt5 XenialQt5.7
 Date of build:
Mon, 23 Oct 2017 09:02:10 +
 Build duration:
1 min 14 sec and counting
   JUnit Tests
  Name: (root) Failed: 0 test(s), Passed: 2 test(s), Skipped: 0 test(s), Total: 2 test(s)
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report100%
(2/2)67%
(6/9)67%
(6/9)64%
(409/640)43%
(169/393)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalssrc80%
(4/5)80%
(4/5)54%
(173/320)34%
(90/263)src.libkirigami50%
(2/4)50%
(2/4)74%
(236/320)61%
(79/130)

D7033: Port ki18n from QtScript to QtQml

[Allan Sandfeld Jensen]

carewolf added a comment.


  No, but I will see if I can find some. Otherwise feel free to take over an 
fix the remaining issues.

REPOSITORY
  R249 KI18n

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

To: carewolf, ilic
Cc: ngraham, dhaumann, ltoscano, kfunk, #frameworks

D8428: Fix cross-compiling with MinGW (MXE)

[Hannah von Reth]

vonreth accepted this revision.
vonreth added a comment.
This revision is now accepted and ready to land.


  looks good to me (and it compiles)

REPOSITORY
  R303 KInit

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

To: vpinon, vonreth, habacker
Cc: #frameworks, bartoloni, benjaminnelan, afarid, fernandoteles, mardelle

D8416: really raising window when shown from systray

[Martin Flöser]

graesslin added inline comments.

INLINE COMMENTS

> mkoller wrote in kstatusnotifieritem.cpp:653
> I'm a bit confused now.
> The patch solves my issue for X11 as it does some lines below already, but 
> you say that this is wrong for Wayland but a solution for wayland does not 
> exist right now.
> I don't use Wayland, I'm on X11 and on X11 the solution works.
> Why can't we simply add this 2 lines until there will be "the correct way" 
> for wayland as you suggest ?
> Since you say the current code is wrong anyway, it needs some adaption anyway.
> So until then, the current code plus my fix improves the situation for all 
> X11 users, and for Wayland you can change the code when all APIs are in place.

It only works on X11 because KWin has a bug. KWin allows the force (which is in 
X11 Terms a "from tool" request) without checking who sends it. I will look 
into hardening this in KWin. Then the existing code won't work anymore.

REPOSITORY
  R289 KNotifications

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

To: mkoller, davidedmundson, graesslin
Cc: #frameworks

D8402: Support "domain-suffix-match" in Security8021xSetting

[Jan Grulich]

jgrulich accepted this revision.
jgrulich added a comment.
This revision is now accepted and ready to land.


  Good to go. Thank you.

REPOSITORY
  R282 NetworkManagerQt

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

To: Martchus, #plasma, jgrulich
Cc: jgrulich, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D8402: Support "domain-suffix-match" in Security8021xSetting

[Marius Kittler]

Martchus updated this revision to Diff 21153.
Martchus added a comment.


  Add "domainSuffixMatch" to auto tests

REPOSITORY
  R282 NetworkManagerQt

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8402?vs=21149=21153

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

AFFECTED FILES
  autotests/settings/8021xsettingtest.cpp
  src/settings/security8021xsetting.cpp
  src/settings/security8021xsetting.h
  src/settings/security8021xsetting_p.h

To: Martchus, #plasma, jgrulich
Cc: jgrulich, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D8402: Support "domain-suffix-match" in Security8021xSetting

[Jan Grulich]

jgrulich requested changes to this revision.
jgrulich added a comment.
This revision now requires changes to proceed.


  Now it looks good. Just one thing I forgot is a unit test for this property. 
You can add it to autotests/settings/8021xsettingtest.cpp and build the tests 
with "-DBUILD_TESTING=true" cmake command and then running "make tests" to 
verify your test passes.

REPOSITORY
  R282 NetworkManagerQt

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

To: Martchus, #plasma, jgrulich
Cc: jgrulich, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D8402: Support "domain-suffix-match" in Security8021xSetting

[Marius Kittler]

Martchus updated this revision to Diff 21149.
Martchus added a comment.


  Changed the method names

REPOSITORY
  R282 NetworkManagerQt

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8402?vs=21069=21149

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

AFFECTED FILES
  src/settings/security8021xsetting.cpp
  src/settings/security8021xsetting.h
  src/settings/security8021xsetting_p.h

To: Martchus, #plasma, jgrulich
Cc: jgrulich, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart