D23205: [KProcessList] Optimize KProcessList::processInfo

[David Hallas]

hallas edited the test plan for this revision.

REPOSITORY
  R244 KCoreAddons

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

To: hallas, davidedmundson, broulik
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23205: [KProcessList] Optimize KProcessList::processInfo

[David Hallas]

hallas updated this revision to Diff 64914.
hallas added a comment.


  Add bug reference

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23205?vs=64913=64914

BRANCH
  optimize_kprocesslist_processinfo

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

AFFECTED FILES
  src/lib/util/kprocesslist.cpp
  src/lib/util/kprocesslist_unix.cpp
  src/lib/util/kprocesslist_win.cpp

To: hallas, davidedmundson, broulik
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23205: [KProcessList] Optimize KProcessList::processInfo

[David Hallas]

hallas marked an inline comment as done.

REPOSITORY
  R244 KCoreAddons

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

To: hallas, davidedmundson, broulik
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23205: [KProcessList] Optimize KProcessList::processInfo

[David Hallas]

hallas updated this revision to Diff 64913.
hallas marked an inline comment as done.
hallas added a comment.


  Fixed review comments, rebased.

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23205?vs=63873=64913

BRANCH
  optimize_kprocesslist_processinfo

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

AFFECTED FILES
  src/lib/util/kprocesslist.cpp
  src/lib/util/kprocesslist_unix.cpp
  src/lib/util/kprocesslist_win.cpp

To: hallas, davidedmundson, broulik
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

KDE CI: Frameworks » kiconthemes » kf5-qt5 WindowsMSVCQt5.13 - Build # 4 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/kiconthemes/job/kf5-qt5%20WindowsMSVCQt5.13/4/
 Project:
kf5-qt5 WindowsMSVCQt5.13
 Date of build:
Thu, 29 Aug 2019 05:31:00 +
 Build duration:
3 min 2 sec and counting
   JUnit Tests
  Name: projectroot Failed: 0 test(s), Passed: 7 test(s), Skipped: 0 test(s), Total: 7 test(s)

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

[David Hallas]

hallas added inline comments.

INLINE COMMENTS

> klistopenfiles.cpp:44
> +if (!path.exists()) {
> +job->setError(KJob::UserDefinedError);
> +job->emitResult();

Should I also set an error string here? And what about defining custom error 
codes, is that how we usually do?

> klistopenfiles.h:37
> +
> +class KCOREADDONS_EXPORT ListOpenFilesJob : public KJob
> +{

I am a little unsure if there is any other KJob functions that should be 
overwritten in this class? The current implementation is quite minimal, but 
maybe that is fine?

REPOSITORY
  R244 KCoreAddons

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

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

[David Hallas]

hallas added a comment.


  @meven , so I finally managed to rewrite this patch to use KJob instead. 
Please take a look at it again and see if this is better approach :)

REPOSITORY
  R244 KCoreAddons

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

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

[David Hallas]

hallas updated this revision to Diff 64911.
hallas added a comment.


  Rewrote the code to use KJob

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21760?vs=59636=64911

BRANCH
  add_list_processes_with_open_files (branched from master)

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/klistopenfilestest.cpp
  autotests/klistopenfilestest.h
  src/lib/CMakeLists.txt
  src/lib/util/klistopenfiles.cpp
  src/lib/util/klistopenfiles.h

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D23500: Support for native Matlab strings

[Dominik Haumann]

dhaumann added a comment.


  You can also add the changes from your other review request if you want, 
since the changes are rather small. But on general it's a good idea to have 
many small manageable review request instead of one big one.

REPOSITORY
  R216 Syntax Highlighting

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

To: bbuch, cullmann
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars

D23552: ECM: remove set_package_properties from FindCanberra

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Compare though all the
  
  > Ideally this is set already directly in the Find-module.
  
  for `DESCRIPTION` and `URL` properties in the docs 
https://cmake.org/cmake/help/latest/module/FeatureSummary.html
  
  Which makes sense to me, at least for FindModules, where one can assume the 
FindX.cmake file exists, so the properties are set, and one only has to add the 
values for the properties `PURPOSE` and `TYPE`, as their are usage dependent.
  So in this case perhaps rather KNotifications should be adapted.

REPOSITORY
  R240 Extra CMake Modules

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

To: dfaure, cgiboudeaux, sitter
Cc: kossebau, kde-buildsystem, kde-frameworks-devel, apol, aacid, LeGast00n, 
GB_2, bencreasy, michaelh, ngraham, bruns

D23552: ECM: remove set_package_properties from FindCanberra

[Harald Sitter]

sitter added a comment.


  No objections from me. This was explicitly suggested during review though, so 
I'd like @cgiboudeaux to approve this.

INLINE COMMENTS

> FindCanberra.cmake:95
>  
>  include(FeatureSummary)
>  

Can be removed as well.

REPOSITORY
  R240 Extra CMake Modules

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

To: dfaure, cgiboudeaux, sitter
Cc: kde-buildsystem, kde-frameworks-devel, apol, aacid, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D23551: CMake config files: use as min dep version the Qt version we built against

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D23551#521571 , @apol wrote:
  
  > This will only work one-way. If the "wrong" Qt is older, but it's not 
necessarily the case.
  >  Also cmake may be linking against the "right" Qt but then use the "wrong" 
one at runtime.
  
  
  Yes, this will not fix everything, just make things a bit more correct, and 
help at least in some situations. There is nothing lost, besides having to 
maintain one more variable.
  
  Because right now, in some builds the generated CMake config is wrong about 
the min version when building against latest Qt (KIconThemes & KIO at least, 
due to some `#if QT_VERSION >= QT_VERSION_CHECK(5, 12, 0)` dependent API usage).

REPOSITORY
  R263 KXmlGui

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

To: kossebau, #frameworks, aacid, #build_system
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23552: ECM: remove set_package_properties from FindCanberra

[David Faure]

dfaure created this revision.
dfaure added reviewers: cgiboudeaux, sitter.
Herald added projects: Frameworks, Build System.
dfaure requested review of this revision.

REVISION SUMMARY
  The user of this find module has to do it, so that they can specify the
  purpose of using canberra in their application. We can't do it twice,
  it leads to
  
  - Warning: Property DESCRIPTION for package Canberra already set to "Event 
sound library", overriding it with "Library for generating event sounds"
  
  The documentation for FeatureSummary always does find_package
  and then set_package_properties, i.e. it was never meant for the
  finder to do this.

TEST PLAN
  knotifications no longer warns

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

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

AFFECTED FILES
  find-modules/FindCanberra.cmake

To: dfaure, cgiboudeaux, sitter
Cc: kde-buildsystem, kde-frameworks-devel, apol, aacid, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D23551: CMake config files: use as min dep version the Qt version we built against

[Aleix Pol Gonzalez]

apol added a comment.


  This will only work one-way. If the "wrong" Qt is older, but it's not 
necessarily the case.
  Also cmake may be linking against the "right" Qt but then use the "wrong" one 
at runtime.
  
  I'm not convinced.

REPOSITORY
  R263 KXmlGui

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

To: kossebau, #frameworks, aacid, #build_system
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21328: Add qml import for KItemModels

[Aleix Pol Gonzalez]

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


  Alright then, let's get this in and kossebau can port it eventually.

REPOSITORY
  R275 KItemModels

BRANCH
  master

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

To: davidedmundson, apol
Cc: kossebau, vkrause, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D23551: CMake config files: use as min dep version the Qt version we built against

[Friedrich W. H. Kossebau]

kossebau added a reviewer: Build System.

REPOSITORY
  R263 KXmlGui

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

To: kossebau, #frameworks, aacid, #build_system
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23550: CMake config files: use as min dep version the Qt version we built against

[Friedrich W. H. Kossebau]

kossebau added a reviewer: Build System.

REPOSITORY
  R244 KCoreAddons

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

To: kossebau, #frameworks, #build_system
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23551: CMake config files: use as min dep version the Qt version we built against

[Friedrich W. H. Kossebau]

kossebau created this revision.
kossebau added reviewers: Frameworks, aacid.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
kossebau requested review of this revision.

REVISION SUMMARY
  Some developers have multiple versions of Qt installed on their system.
  And source code often has some version-dependent code paths, e.g. using
  newer API if found available. In that case the resulting build has an
  effective dependency on the Qt version built against.
  
  To properly reflect that, the Qt version set as minimally required version
  of the listed dependencies in the generated CMake config files should use
  that version instead.
  More, KXmlGui uses private API with Qt5::CorePrivate, so also future
  versions of Qt5::Core might not be compatible.
  
  While the current code of KXmlGui does not have such version dependent
  code variants, they are added now and then, so using the Qt version built
  against as minimally required dependency is a good default, and should not
  change anything for all the users with just one Qt version, and improve
  things for those with, catching bad mixes early in the build time.
  And given the use of private Qt::Core API, requiring exactly the same
  version by using "find_dependency(Qt5Core @USED_QT_VERSION@ EXACT)"
  should be more safe, even if the private API might be stable in reality
  for a set of versions.

TEST PLAN
  Generated CMake config files now uses with the find_dependency(Qt5) calls
  the version of the Qt KXmlGui was built against.

REPOSITORY
  R263 KXmlGui

BRANCH
  setusetqtversionasmindep

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

AFFECTED FILES
  CMakeLists.txt
  KF5XmlGuiConfig.cmake.in

To: kossebau, #frameworks, aacid
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23550: CMake config files: use as min dep version the Qt version we built against

[Friedrich W. H. Kossebau]

kossebau added a comment.


  If you think this makes sense, I would do the same change to all KDE 
Frameworks modules (and hope others pick up for their code with public library 
API).
  
  Target: KF 5.63 earliest.

REPOSITORY
  R244 KCoreAddons

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

To: kossebau, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23550: CMake config files: use as min dep version the Qt version we built against

[Friedrich W. H. Kossebau]

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

REVISION SUMMARY
  Some developers have multiple versions of Qt installed on their system. 
  And source code often has some version-dependent code paths, e.g. using
  newer API if found available. In that case the resulting build has an
  effective dependency on the Qt version built against.
  
  To properly reflect that, the Qt version set as minimally required version
  of the listed dependencies in the generated CMake config files should use
  that version instead.
  While the current code of KCoreAddons does not have such version dependent
  code variants, they are added now and then, so using the Qt version built
  against as minimally required dependency is a good default, and should not
  change anything for all the users with just one Qt version, and improve
  things for those with, catching bad mixes early in the build time.

TEST PLAN
  Generated CMake config files now uses with the find_dependency(Qt5) call the
  version of the Qt KCoreAddons was built against.

REPOSITORY
  R244 KCoreAddons

BRANCH
  setusetqtversionasmindep

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

AFFECTED FILES
  CMakeLists.txt
  KF5CoreAddonsConfig.cmake.in

To: kossebau, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21328: Add qml import for KItemModels

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D21328#521535 , @davidedmundson 
wrote:
  
  > > Can you look into kossebau's coment? Let's get this in once that's ironed 
out.
  >
  > Kossebau's new API is not yet merged. It looks nice though.
  
  
  Yes, sorry, I dropped the ball there for a bit as I was a unsure still about 
usefulness an completeness of those macros, and also since then had not worked 
on own QML modules, while I had hoped I could test-drive the macros some more 
myself before making them public ECM API.
  Shall pick up again in the next weeks, and then also ask for more feedback of 
QML module by all you Plasma QML workers. Feel invited to already be triggered 
to give some more comments (thanks @apol for your review there so far)

REPOSITORY
  R275 KItemModels

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

To: davidedmundson
Cc: kossebau, vkrause, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D21328: Add qml import for KItemModels

[David Edmundson]

davidedmundson added a comment.


  > Can you look into kossebau's coment? Let's get this in once that's ironed 
out.
  
  Kossebau's new API is not yet merged. It looks nice though.

REPOSITORY
  R275 KItemModels

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

To: davidedmundson
Cc: kossebau, vkrause, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D21328: Add qml import for KItemModels

[Aleix Pol Gonzalez]

apol added a comment.


  Can you look into kossebau's coment? Let's get this in once that's ironed out.

INLINE COMMENTS

> CMakeLists.txt:30
>  
> +if (${Qt5Qml_FOUND})
> +ecm_add_tests(

I like `if (TARGET Qt5::Qml)` better.

REPOSITORY
  R275 KItemModels

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

To: davidedmundson
Cc: kossebau, vkrause, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D23399: Add setter to Type, Activity, Agent and UrlFilter query fields

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> kossebau wrote in query.h:101
> Please add "@since 6.62" to all this new API, so consumers of them know which 
> min version of KActivitiesStat they need to require to be able to rely on 
> their existance.

"5.62" of course :)

REPOSITORY
  R159 KActivities Statistics

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

To: meven, ivan
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D22144: Add kio recentlyused:/ to access KActivityStats data

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> CMakeLists.txt:68
>  
> +find_package(KF5ActivitiesStats QUIET)
> +set_package_properties(KF5ActivitiesStats PROPERTIES

Please add a min version, as the code seems to rely on new API only added 
recently:

  find_package(KF5ActivitiesStats 5.62.0 QUIET)

REPOSITORY
  R320 KIO Extras

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

To: meven, ivan, #frameworks, ngraham, dfaure
Cc: kossebau, broulik, dhaumann, elvisangelaccio, kde-frameworks-devel, 
kfm-devel, vmarinescu, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D23399: Add setter to Type, Activity, Agent and UrlFilter query fields

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> query.h:101
>  void addUrlFilters(const QStringList );
> +void setTypes(const Terms::Type );
> +void setAgents(const Terms::Agent );

Please add "@since 6.62" to all this new API, so consumers of them know which 
min version of KActivitiesStat they need to require to be able to rely on their 
existance.

REPOSITORY
  R159 KActivities Statistics

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

To: meven, ivan
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D7446: [Places panel] Revamp the Recently Saved section

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  +1
  
  I don't think @markg'ss change requested status still applies since it was 
for the ancient version of the patch that I submitted two years ago (!!!).
  
  I also think that ultimately we should remove Modified Today and Modified 
Yesterday because the new one is so much better. But maybe we can have that 
discussion later if you don't agree.

REPOSITORY
  R241 KIO

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

To: meven, #dolphin, broulik, elvisangelaccio, markg, #vdg, #frameworks, ngraham
Cc: meven, trickyricky26, andreask, huftis, svenmauch, kde-frameworks-devel, 
spoorun, anthonyfieroni, andreaska, gregormi, markg, alexeymin, broulik, 
elvisangelaccio, dfaure, davidedmundson, ltoscano, #konqueror, vmarinescu, 
fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
michaelh, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, 
mikesomov

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Méven Car]

meven added a comment.


  This could be an alternative to D17595 

REPOSITORY
  R241 KIO

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

To: emateli, #frameworks, dfaure, mlaurent
Cc: meven, anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, 
ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D21328: Add qml import for KItemModels

[David Edmundson]

davidedmundson updated this revision to Diff 64882.
davidedmundson added a comment.


  Add KDescendentsProxyModel

REPOSITORY
  R275 KItemModels

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21328?vs=64881=64882

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  autotests/CMakeLists.txt
  autotests/concatenaterowstest.qml
  autotests/kconcatenaterows_qml.cpp
  src/CMakeLists.txt
  src/core/CMakeLists.txt
  src/core/kbihash_p.h
  src/core/kbreadcrumbselectionmodel.cpp
  src/core/kbreadcrumbselectionmodel.h
  src/core/kcheckableproxymodel.cpp
  src/core/kcheckableproxymodel.h
  src/core/kconcatenaterowsproxymodel.cpp
  src/core/kconcatenaterowsproxymodel.h
  src/core/kdescendantsproxymodel.cpp
  src/core/kdescendantsproxymodel.h
  src/core/kextracolumnsproxymodel.cpp
  src/core/kextracolumnsproxymodel.h
  src/core/klinkitemselectionmodel.cpp
  src/core/klinkitemselectionmodel.h
  src/core/kmodelindexproxymapper.cpp
  src/core/kmodelindexproxymapper.h
  src/core/krearrangecolumnsproxymodel.cpp
  src/core/krearrangecolumnsproxymodel.h
  src/core/krecursivefilterproxymodel.cpp
  src/core/krecursivefilterproxymodel.h
  src/core/kselectionproxymodel.cpp
  src/core/kselectionproxymodel.h
  src/core/kvoidpointerfactory_p.h
  src/kbihash_p.h
  src/kbreadcrumbselectionmodel.cpp
  src/kbreadcrumbselectionmodel.h
  src/kcheckableproxymodel.cpp
  src/kcheckableproxymodel.h
  src/kconcatenaterowsproxymodel.cpp
  src/kconcatenaterowsproxymodel.h
  src/kdescendantsproxymodel.cpp
  src/kdescendantsproxymodel.h
  src/kextracolumnsproxymodel.cpp
  src/kextracolumnsproxymodel.h
  src/klinkitemselectionmodel.cpp
  src/klinkitemselectionmodel.h
  src/kmodelindexproxymapper.cpp
  src/kmodelindexproxymapper.h
  src/krearrangecolumnsproxymodel.cpp
  src/krearrangecolumnsproxymodel.h
  src/krecursivefilterproxymodel.cpp
  src/krecursivefilterproxymodel.h
  src/kselectionproxymodel.cpp
  src/kselectionproxymodel.h
  src/kvoidpointerfactory_p.h
  src/qml/CMakeLists.txt
  src/qml/kconcatenaterowsproxymodel_qml.cpp
  src/qml/kconcatenaterowsproxymodel_qml.h
  src/qml/plugin.cpp
  src/qml/plugin.h
  src/qml/qmldir

To: davidedmundson
Cc: kossebau, vkrause, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D21328: Add qml import for KItemModels

[David Edmundson]

davidedmundson added a comment.


  > I wonder if it was necessary to do the whole move into core :P.
  
  It wasn't necessary, but I was trying to follow the framework convention. 
  If it helps get this, I can remove it.

REPOSITORY
  R275 KItemModels

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

To: davidedmundson
Cc: kossebau, vkrause, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D21328: Add qml import for KItemModels

[David Edmundson]

davidedmundson updated this revision to Diff 64881.
davidedmundson added a comment.


  Port manual test to unit test

REPOSITORY
  R275 KItemModels

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21328?vs=58449=64881

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  autotests/CMakeLists.txt
  autotests/concatenaterowstest.qml
  autotests/kconcatenaterows_qml.cpp
  src/CMakeLists.txt
  src/core/CMakeLists.txt
  src/core/kbihash_p.h
  src/core/kbreadcrumbselectionmodel.cpp
  src/core/kbreadcrumbselectionmodel.h
  src/core/kcheckableproxymodel.cpp
  src/core/kcheckableproxymodel.h
  src/core/kconcatenaterowsproxymodel.cpp
  src/core/kconcatenaterowsproxymodel.h
  src/core/kdescendantsproxymodel.cpp
  src/core/kdescendantsproxymodel.h
  src/core/kextracolumnsproxymodel.cpp
  src/core/kextracolumnsproxymodel.h
  src/core/klinkitemselectionmodel.cpp
  src/core/klinkitemselectionmodel.h
  src/core/kmodelindexproxymapper.cpp
  src/core/kmodelindexproxymapper.h
  src/core/krearrangecolumnsproxymodel.cpp
  src/core/krearrangecolumnsproxymodel.h
  src/core/krecursivefilterproxymodel.cpp
  src/core/krecursivefilterproxymodel.h
  src/core/kselectionproxymodel.cpp
  src/core/kselectionproxymodel.h
  src/core/kvoidpointerfactory_p.h
  src/kbihash_p.h
  src/kbreadcrumbselectionmodel.cpp
  src/kbreadcrumbselectionmodel.h
  src/kcheckableproxymodel.cpp
  src/kcheckableproxymodel.h
  src/kconcatenaterowsproxymodel.cpp
  src/kconcatenaterowsproxymodel.h
  src/kdescendantsproxymodel.cpp
  src/kdescendantsproxymodel.h
  src/kextracolumnsproxymodel.cpp
  src/kextracolumnsproxymodel.h
  src/klinkitemselectionmodel.cpp
  src/klinkitemselectionmodel.h
  src/kmodelindexproxymapper.cpp
  src/kmodelindexproxymapper.h
  src/krearrangecolumnsproxymodel.cpp
  src/krearrangecolumnsproxymodel.h
  src/krecursivefilterproxymodel.cpp
  src/krecursivefilterproxymodel.h
  src/kselectionproxymodel.cpp
  src/kselectionproxymodel.h
  src/kvoidpointerfactory_p.h
  src/qml/CMakeLists.txt
  src/qml/kconcatenaterowsproxymodel_qml.cpp
  src/qml/kconcatenaterowsproxymodel_qml.h
  src/qml/plugin.cpp
  src/qml/plugin.h
  src/qml/qmldir

To: davidedmundson
Cc: kossebau, vkrause, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D7446: [Places panel] Revamp the Recently Saved section

[Méven Car]

meven updated this revision to Diff 64880.
meven added a comment.


  Rebase

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7446?vs=62380=64880

BRANCH
  arcpatch-D7446

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

AFFECTED FILES
  src/filewidgets/kfileplacesitem.cpp
  src/filewidgets/kfileplacesmodel.cpp

To: meven, #dolphin, broulik, elvisangelaccio, markg, #vdg, #frameworks, ngraham
Cc: meven, trickyricky26, andreask, huftis, svenmauch, kde-frameworks-devel, 
spoorun, anthonyfieroni, andreaska, gregormi, markg, alexeymin, broulik, 
elvisangelaccio, dfaure, davidedmundson, ltoscano, #konqueror, aprcela, 
vmarinescu, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, 
feverfew, michaelh, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D7446: [Places panel] Revamp the Recently Saved section

[Méven Car]

meven added a comment.


  With D22144  landing this is now ready 
for prime time !
  
  And thanks to D23112 , this will support 
history from application using gtk as well (firefox, inkscape...)

REPOSITORY
  R241 KIO

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

To: meven, #dolphin, broulik, elvisangelaccio, markg, #vdg, #frameworks, ngraham
Cc: meven, trickyricky26, andreask, huftis, svenmauch, kde-frameworks-devel, 
spoorun, anthonyfieroni, andreaska, gregormi, markg, alexeymin, broulik, 
elvisangelaccio, dfaure, davidedmundson, ltoscano, #konqueror, vmarinescu, 
fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
michaelh, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, 
mikesomov

D7446: [Places panel] Revamp the Recently Saved section

[Méven Car]

meven edited the summary of this revision.

REPOSITORY
  R241 KIO

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

To: meven, #dolphin, broulik, elvisangelaccio, markg, #vdg, #frameworks, ngraham
Cc: meven, trickyricky26, andreask, huftis, svenmauch, kde-frameworks-devel, 
spoorun, anthonyfieroni, andreaska, gregormi, markg, alexeymin, broulik, 
elvisangelaccio, dfaure, davidedmundson, ltoscano, #konqueror, aprcela, 
vmarinescu, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, 
feverfew, michaelh, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D22144: Add kio recentlyused:/ to access KActivityStats data

[Méven Car]

This revision was automatically updated to reflect the committed changes.
Closed by commit R320:ae44042e541c: Add kio recentlyused:/ to access 
KActivityStats data (authored by meven).

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22144?vs=64845=64879

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

AFFECTED FILES
  CMakeLists.txt
  kio-extras.categories
  recentlyused/CMakeLists.txt
  recentlyused/recentlyused.cpp
  recentlyused/recentlyused.h
  recentlyused/recentlyused.json

To: meven, ivan, #frameworks, ngraham, dfaure
Cc: broulik, dhaumann, elvisangelaccio, kde-frameworks-devel, kfm-devel, 
aprcela, vmarinescu, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

Re: detect the Qt version KF5 frameworks were build against in CMake?

[Friedrich W. H. Kossebau]

Am Mittwoch, 28. August 2019, 19:19:21 CEST schrieb René J.V. Bertin:
> Hi,
> 
> Some KF5 framework libraries pull in Qt5 headers and/or libraries via their
> CMake modules, right?

Can you specifiy some more what you mean by "pull in"?

> Take the somewhat unusual situation where you have your KF5 frameworks built
> against, say, Qt 5.9 from the system and you want to test an application
> against a different, newer Qt build installed with Qt's installer. Qt
> guarantees this should work as long as you don't use private APIs. 

It also assumes you use compatible build flags for Qt and do not have e.g. 
different feature flags like for deprecated methods or things like D-Bus or 
other flags which affect symbol names, e.g. Qt namespacing.

> Those
> aforementioned KF5 frameworks will pull in "their" Qt5 headers (e.g. for
> Qt5Network in the project I'm tinkering with), meaning that QT_VERSION
> #ifdefs can behave in an unexpected way.

This sounds like you are talking about "pulling in" at build time, right?
KF modules install CMake config files to be found via cmake's 
find_package(KFFoo). Those ensure that required dependencies for the public 
interface are found as well, by having lines like

find_dependency(Qt5Core "5.9.0")

where the number is generated at build time of the KF5Foo module, taken 
usually from the min version variable via @@ substitution from a templare 
file.
This find_dependency() call will find whatever Qt is to be found first in the 
environment where find_package(KF5Foo) is called. So not necessarily the Qt 
installation which was used to build KF5Foo.

> Is there a way to detect this kind of situation at the cmake level, i.e. is
> there some constant that holds the Qt value the frameworks were built
> against?

Not that I am aware, the installed CMake config files currently just forward 
which minimum version would be expected at build time.

Talking about, this actually could be also wrong sometimes, both in case 
someone accidentally combines an older, yet old enough for the minimum Qt 
version, which would fail only at link/load time then if symbols only 
available in the newer Qt version had been used before.

Or in the case of relying on private Qt symbols, even if in non-published API, 
the CMake config file should actually require the exact version already from 
the start, to also catch unexpected surprises at link/load time.
E.g. by adding "EXACT" to the find_dependency call, with the very version 
found and used at build time:

CMakeLists.txt:
set(USED_QT_VERSION ${Qt5Core_VERSION})

KF5XmlGuiConfig.cmake.in:
find_dependency(Qt5Core @USED_QT_VERSION@ EXACT)

Guess we should change things at least for those KF modules using private Qt 
symbolds, will do and prepare some patches for review and discussion.
This should also improve things for KDE CI when bumping installed Qt versions 
and wondering why some things might fail when reusing previously built 
binaries.

So for the actual question, how to know which Qt version a KF module was build 
again, I think there is no direct info to query.

And the question would be, what is your actual case you need the info for?
In general any variant of the API should be queryable directly by properties 
of the build system or config.h files, not indirectly over knowing which Qt 
version was build against. So perhaps your need could/should be solved some 
other way.

Cheers
Friedrich

D23385: More porting from QRegExp to QRegularExpression

[Dominik Haumann]

dhaumann added a comment.


  Oh, in that case can you or @cullmann simply revert this part or #ifdef it? I 
currently cannot at least until the weekend (sorry!).

REPOSITORY
  R39 KTextEditor

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

To: dhaumann, cullmann
Cc: dvratil, kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

KDE CI: Frameworks » kwayland » kf5-qt5 SUSEQt5.12 - Build # 56 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/kwayland/job/kf5-qt5%20SUSEQt5.12/56/
 Project:
kf5-qt5 SUSEQt5.12
 Date of build:
Wed, 28 Aug 2019 17:38:47 +
 Build duration:
12 min and counting
   BUILD ARTIFACTS
  abi-compatibility-results.yamlacc/KF5Wayland-5.62.0.xmlcompat_reports/KF5Wayland_compat_report.htmllogs/KF5Wayland/5.62.0/log.txt
   JUnit Tests
  Name: (root) Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot.autotests Failed: 0 test(s), Passed: 46 test(s), Skipped: 0 test(s), Total: 46 test(s)
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report63%
(5/8)90%
(240/268)90%
(240/268)85%
(27127/32004)53%
(10908/20534)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests.client100%
(43/43)100%
(43/43)99%
(12452/12525)50%
(6559/13037)autotests.server100%
(5/5)100%
(5/5)99%
(373/376)49%
(177/360)src.client96%
(73/76)96%
(73/76)85%
(6336/7471)64%
(1823/2828)src.compat100%
(2/2)100%
(2/2)100%
(81/81)100%
(0/0)src.server95%
(117/123)95%
(117/123)84%
(7885/9351)64%
(2349/3678)src.tools0%
(0/2)0%
(0/2)0%
(0/785)0%
(0/302)src.tools.testserver0%
(0/3)0%
(0/3)0%
(0/120)0%
(0/14)tests0%
(0/14)0%
(0/14)0%
(0/1295)0%
(0/315)

KDE CI: Frameworks » kwayland » kf5-qt5 SUSEQt5.13 - Build # 12 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/kwayland/job/kf5-qt5%20SUSEQt5.13/12/
 Project:
kf5-qt5 SUSEQt5.13
 Date of build:
Wed, 28 Aug 2019 17:38:47 +
 Build duration:
6 min 18 sec and counting
   BUILD ARTIFACTS
  abi-compatibility-results.yamlacc/KF5Wayland-5.62.0.xmlcompat_reports/KF5Wayland_compat_report.htmllogs/KF5Wayland/5.62.0/log.txt
   JUnit Tests
  Name: (root) Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot.autotests Failed: 0 test(s), Passed: 46 test(s), Skipped: 0 test(s), Total: 46 test(s)
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report63%
(5/8)90%
(240/268)90%
(240/268)85%
(27124/32004)53%
(10906/20534)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests.client100%
(43/43)100%
(43/43)99%
(12452/12525)50%
(6559/13037)autotests.server100%
(5/5)100%
(5/5)99%
(373/376)49%
(177/360)src.client96%
(73/76)96%
(73/76)85%
(6333/7471)64%
(1821/2828)src.compat100%
(2/2)100%
(2/2)100%
(81/81)100%
(0/0)src.server95%
(117/123)95%
(117/123)84%
(7885/9351)64%
(2349/3678)src.tools0%
(0/2)0%
(0/2)0%
(0/785)0%
(0/302)src.tools.testserver0%
(0/3)0%
(0/3)0%
(0/120)0%
(0/14)tests0%
(0/14)0%
(0/14)0%
(0/1295)0%
(0/315)

KDE CI: Frameworks » kwayland » kf5-qt5 FreeBSDQt5.13 - Build # 11 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kwayland/job/kf5-qt5%20FreeBSDQt5.13/11/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Wed, 28 Aug 2019 17:38:47 +
 Build duration:
5 min 57 sec and counting
   JUnit Tests
  Name: projectroot.autotests Failed: 14 test(s), Passed: 28 test(s), Skipped: 0 test(s), Total: 42 test(s)Failed: projectroot.autotests.client.kwayland_testCompositorFailed: projectroot.autotests.client.kwayland_testDataDeviceFailed: projectroot.autotests.client.kwayland_testDataSourceFailed: projectroot.autotests.client.kwayland_testRegionFailed: projectroot.autotests.client.kwayland_testRemoteAccessFailed: projectroot.autotests.client.kwayland_testShmPoolFailed: projectroot.autotests.client.kwayland_testSubCompositorFailed: projectroot.autotests.client.kwayland_testSubSurfaceFailed: projectroot.autotests.client.kwayland_testWaylandConnectionThreadFailed: projectroot.autotests.client.kwayland_testWaylandRegistryFailed: projectroot.autotests.client.kwayland_testWaylandShellFailed: projectroot.autotests.client.kwayland_testWaylandSurfaceFailed: projectroot.autotests.client.kwayland_testXdgShellStableFailed: projectroot.autotests.server.kwayland_testWaylandServerDisplay

D22862: [server] Wrap proxyRemoveSurface in smart pointer

[David Edmundson]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:f77630523705: [server] Wrap proxyRemoveSurface in smart 
pointer (authored by davidedmundson).

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22862?vs=62871=64876

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

AFFECTED FILES
  src/server/datadevice_interface.cpp

To: davidedmundson, #kwin, zzag
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21618: Allow QtQuick applets and wallpapers to defer UiReadyConstraint

[David Edmundson]

davidedmundson abandoned this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: davidedmundson, #plasma, apol
Cc: broulik, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

detect the Qt version KF5 frameworks were build against in CMake?

[René J . V . Bertin]

Hi,

Some KF5 framework libraries pull in Qt5 headers and/or libraries via their 
CMake modules, right?

Take the somewhat unusual situation where you have your KF5 frameworks built 
against, say, Qt 5.9 from the system and you want to test an application 
against a different, newer Qt build installed with Qt's installer. Qt 
guarantees this should work as long as you don't use private APIs.
Those aforementioned KF5 frameworks will pull in "their" Qt5 headers (e.g. for 
Qt5Network in the project I'm tinkering with), meaning that QT_VERSION #ifdefs can behave 
in an unexpected way.

Is there a way to detect this kind of situation at the cmake level, i.e. is 
there some constant that holds the Qt value the frameworks were built against?

Thanks,
R.

D22144: Add kio recentlyused:/ to access KActivityStats data

[Nathaniel Graham]

ngraham added a comment.


  I think this can go in now. It's been reviewed by a bunch of the big cheeses. 
:) Very nice work!

REPOSITORY
  R320 KIO Extras

BRANCH
  arcpatch-D22144_1

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

To: meven, ivan, #frameworks, ngraham, dfaure
Cc: broulik, dhaumann, elvisangelaccio, kde-frameworks-devel, kfm-devel, 
aprcela, vmarinescu, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D23537: extend state verification to open connection commands

[Harald Sitter]

sitter added a comment.


  I do wonder if maybe it would make sense to revise the verification system to 
be based off of a list of allowed/forbidden functions that may be called during 
a command.
  
  e.g. for open the requirements are actually more complicated than this diff 
establishes. It should be: CMD_OPEN must result in exactly 1 opened() call OR 
exactly 1 error() call AND must not ever call finished(). This is a less 
specific variant of what m_finalityCommand represents. Where m_finalityCommand 
simply marks a command as allowed/forbidden to call error and finished, a more 
dynamic system would allow us to tightly control different calls.
  
  Simply put: Any given command has 0:N functions it may or may not call. And 
currently we have no system to codify that.
  
  So maybe something like this would make sense:
  
void validateCall(QString name)
{
q_assert(m_CurrentlyAllowedCalls.include?(name), "mustn't call this 
function");
}

void opened()
{
validateCall("opened");
...
}

void finished()
{
validateCall("finished");
...
}

REPOSITORY
  R241 KIO

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

To: sitter, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23537: extend state verification to open connection commands

[Harald Sitter]

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

REVISION SUMMARY
  - open/seek/read/write: may not call finished (only error)
  - close: must reach finality (finished or error)

TEST PLAN
  when trying to play from sftp in dragonplayer the broken slave implodes

REPOSITORY
  R241 KIO

BRANCH
  assert-file

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

AFFECTED FILES
  src/core/slavebase.cpp

To: sitter, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 64855.
leinir marked 11 inline comments as done.
leinir added a comment.


  Address comments by @ahiemstra
  
  - Add a comment about why Button's configfile isn't aliased
  - Set the object ownership policy for QuickQuestionListener to cpp
  - std::make_shared for clarity, and a bit of debug categorisation
  - Simplify some whiley recursion, and use checkIndex()
  - Remember to check before setting properties to the same value
  - Actually require 5.11 (and sanitise some of the QtQuick imports)
  - Pull out the three delegate components
  - Add a wrapper component for CommentsModel
  - Use the new CommentsModel component

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21721?vs=64465=64855

BRANCH
  knsquick-feature-parity-with-kns (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  src/attica/atticaprovider.cpp
  src/attica/atticaprovider_p.h
  src/core/CMakeLists.txt
  src/core/author.cpp
  src/core/author.h
  src/core/commentsmodel.cpp
  src/core/commentsmodel.h
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/installation.cpp
  src/core/itemsmodel.cpp
  src/core/itemsmodel.h
  src/core/provider.h
  src/core/question.h
  src/qtquick/CMakeLists.txt
  src/qtquick/author.cpp
  src/qtquick/author.h
  src/qtquick/categoriesmodel.cpp
  src/qtquick/categoriesmodel.h
  src/qtquick/commentsmodel.cpp
  src/qtquick/commentsmodel.h
  src/qtquick/qml/Button.qml
  src/qtquick/qml/Dialog.qml
  src/qtquick/qml/DialogContent.qml
  src/qtquick/qml/DownloadItemsSheet.qml
  src/qtquick/qml/EntryDetails.qml
  src/qtquick/qml/NewStuffItem.qml
  src/qtquick/qml/NewStuffList.qml
  src/qtquick/qml/Page.qml
  src/qtquick/qml/QuestionAsker.qml
  src/qtquick/qml/private/ConditionalLoader.qml
  src/qtquick/qml/private/EntryCommentDelegate.qml
  src/qtquick/qml/private/EntryCommentsPage.qml
  src/qtquick/qml/private/EntryScreenshots.qml
  src/qtquick/qml/private/GridTileDelegate.qml
  src/qtquick/qml/private/Rating.qml
  src/qtquick/qml/private/Shadow.qml
  src/qtquick/qml/private/entrygriddelegates/BigPreviewDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/ThumbDelegate.qml
  src/qtquick/qml/private/entrygriddelegates/TileDelegate.qml
  src/qtquick/qmldir
  src/qtquick/qmlplugin.cpp
  src/qtquick/quickengine.cpp
  src/qtquick/quickengine.h
  src/qtquick/quickitemsmodel.cpp
  src/qtquick/quickitemsmodel.h
  src/qtquick/quickquestionlistener.cpp
  src/qtquick/quickquestionlistener.h
  tests/CMakeLists.txt
  tests/khotnewstuff-dialog-ui/main.qml
  tests/khotnewstuff-dialog.cpp

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

[Dan Leinir Turthra Jensen]

leinir marked 12 inline comments as done.
leinir added inline comments.

INLINE COMMENTS

> ahiemstra wrote in atticaprovider.cpp:355
> Since upstream Qt is starting to discourage usage of QList, maybe use QVector 
> instead.

That'd be good, except the rest of the KNewStuff API is all QList based. It'll 
want doing for KF6, but since QList is being deprecated for that anyway, i'm 
thinking we'll end up with a general QList->QVector porting effort for that 
time anyway, and right now it'd just be introducing a different API style for 
seemingly no reason... Otherwise yes :)

> ahiemstra wrote in atticaprovider.cpp:359
> Generally, it's recommended to use `std::shared_ptr 
> knsComment = std::make_shared()`. As a bonus, that allows 
> you to use `auto knsComment`.

Handy :) One of those places where i'm not opposed to auto, as it's easily read 
from the right hand side ;)

> ahiemstra wrote in atticaprovider.cpp:370
> Probably better to use categorised logging for this?

Actually leftovers... but also yes :)

> ahiemstra wrote in commentsmodel.cpp:135
> Might want to add a call to checkIndex here 
> (https://doc.qt.io/qt-5/qabstractitemmodel.html#checkIndex), it would 
> obsolete some additional checking you do later.

After confirming that 5.11 (which introduces that function) is indeed 
acceptable for Frameworks, i'm ll over that ;) Very handy little function, 
that :)

> ahiemstra wrote in commentsmodel.cpp:177
> Since you're already checking for `child` being true (aka not-null), this if 
> becomes rather superfluous.

Hmm... leftovers from earlier versions of that function, there :)

> ahiemstra wrote in commentsmodel.h:50
> Considering this is intended to be used from QML, I would drop this, 
> especially since the current code in engine does nothing special. And 
> changing this would allow for doing `CommentsModel { entry: someEntry }` from 
> QML.

It's not really intended to be used from QML, i really only left out a more 
clever wrapper because... well, it's not really that heavy. But, prompted by 
this, i have in fact produced that abstraction, so there is now a 
NewStuff.CommentsModel, which makes things a bit easier during implementation, 
and more delarativey ;)

> ahiemstra wrote in author.cpp:103
> You should check if the actual property value has changed before changing it. 
> I generally do something along the lines of `if (newEngine == d->engine) 
> return;`. This helps with preventing binding loops in properties exposed to 
> QML.

Used to do that all the time, not honestly sure why i didn't here. Thanks for 
the reminder :)

> ahiemstra wrote in Button.qml:39
> Any particular reason not to alias these directly to the dialog?

Yes, if it's aliased directly, we end up initialising the engine as soon as the 
Button component is instantiated, which we want to avoid (as it's quite an 
expensive operation, and causes internet traffic the user might very well have 
no intention of using... which arguably is also a phone-home situation, and 
that needs to be avoided). I'll make an implementationy comment about that. 
(not really documentation relevant, but certainly relevant when someone 
invariably comes across this in the future and asks the same question)

> ahiemstra wrote in Dialog.qml:34
> If you're using a Qt 5.12 import anyway, might want to unify the other 
> imports to also use .12 versions.

Don't actually need 5.12 ones... so i'll scale back to 5.11, but also a bunch 
of cleanup elsewhere, and also actually /require/ 5.11 because of that other 
bit of handy functionality with checkIndex :)

> ahiemstra wrote in Dialog.qml:76
> Any particular reason for using a signal parameter here instead of just 
> setting a property on the dialog? Using a property generally allows for more 
> declarative use of the item.

Because a changedEntries property would logically be for the lifetime of the 
component instance, where dialogFinished is for this specific time the dialog 
was opened... The property would reasonably also be useful, but it would be 
semantically different (unless it's documented as being cleared when the dialog 
is shown, and then filled with whatever's changed once the dialog has been 
closed... which we could do, but kind of feels uglier than this signal)

> ahiemstra wrote in DownloadItemsSheet.qml:49
> Design wise, I don't think having a card inside an overlaysheet looks too 
> nice. I'm also not sure if the extra attention is needed, so maybe just use a 
> label?

This definitely is something that'd be great to hear from the VDG about (who i 
seem to not be able to @ as a group, i guess because it's not really a user or 
whatnot... certainly would be handy ;) )

> ahiemstra wrote in Page.qml:210
> For readability, it would be better to move these to their own files.

It would, yes... Feared it was a bit clugey, but it was super straightforward, 
so yay for that ;)

> ahiemstra wrote in qmlplugin.cpp:54
> Note that this makes the qml engine take ownvership of your 

D23385: More porting from QRegExp to QRegularExpression

[Daniel Vrátil]

dvratil added inline comments.

INLINE COMMENTS

> katedocument.cpp:4676
>  foreach (const QString , wildcards) {
> -QRegExp wildcard(pattern, Qt::CaseSensitive, QRegExp::Wildcard);
> +QRegularExpression wildcard(QLatin1Char('^') + 
> QRegularExpression::wildcardToRegularExpression(pattern) + QLatin1Char('$'));
>  

@dhaumann `QRegularExpression::wildcardToRegularExpression()` was introduced in 
Qt 5.12, but Frameworks depend on 5.11 :-( So this change breaks build...

REPOSITORY
  R39 KTextEditor

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

To: dhaumann, cullmann
Cc: dvratil, kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann

D23500: Support for native Matlab strings

[Benjamin Buch]

bbuch added a comment.


  Should I add the changes from D23498  
here as well?

REPOSITORY
  R216 Syntax Highlighting

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

To: bbuch, cullmann
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars

D23500: Support for native Matlab strings

[Benjamin Buch]

bbuch updated this revision to Diff 64854.
bbuch added a comment.


  increase file version, kateversion to 5, remove hard coded colors, example to 
autotests input

REPOSITORY
  R216 Syntax Highlighting

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23500?vs=64751=64854

BRANCH
  arcpatch-D23500

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

AFFECTED FILES
  autotests/input/test.m
  data/syntax/matlab.xml

To: bbuch, cullmann
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, 
michaelh, ngraham, bruns, demsking, cullmann, sars

D23194: Making FileJob behave consistently.

[Chinmoy Ranjan Pradhan]

chinmoyr accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R241 KIO

BRANCH
  ConsistentRead (branched from master)

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

To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

KDE CI: Frameworks » kio » kf5-qt5 FreeBSDQt5.13 - Build # 41 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20FreeBSDQt5.13/41/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Wed, 28 Aug 2019 14:23:52 +
 Build duration:
6 min 27 sec and counting
   JUnit Tests
  Name: projectroot Failed: 3 test(s), Passed: 49 test(s), Skipped: 0 test(s), Total: 52 test(s)Failed: projectroot.autotests.kiocore_kmountpointtestFailed: projectroot.autotests.kiowidgets_kdirlistertestFailed: projectroot.autotests.kiowidgets_kdirmodeltestName: projectroot.autotests Failed: 0 test(s), Passed: 6 test(s), Skipped: 0 test(s), Total: 6 test(s)Name: projectroot.src.ioslaves.trash Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot.src.kpasswdserver Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)

KDE CI: Frameworks » kio » kf5-qt5 SUSEQt5.13 - Build # 50 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.13/50/
 Project:
kf5-qt5 SUSEQt5.13
 Date of build:
Wed, 28 Aug 2019 14:23:52 +
 Build duration:
6 min 23 sec and counting
   BUILD ARTIFACTS
  acc/KF5KIO-5.62.0.xmllogs/KF5KIO/5.62.0/log.txt
   JUnit Tests
  Name: (root) Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot Failed: 0 test(s), Passed: 52 test(s), Skipped: 0 test(s), Total: 52 test(s)Name: projectroot.autotests Failed: 0 test(s), Passed: 6 test(s), Skipped: 0 test(s), Total: 6 test(s)Name: projectroot.src.ioslaves.trash Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot.src.kpasswdserver Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report64%
(23/36)66%
(265/402)66%
(265/402)55%
(33557/61126)39%
(17069/43934)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests100%
(57/57)100%
(57/57)95%
(9341/9793)48%
(4287/9008)autotests.http100%
(5/5)100%
(5/5)99%
(581/582)68%
(108/160)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.core87%
(101/116)87%
(101/116)59%
(8558/14443)51%
(4703/9191)src.core.kssl100%
(1/1)100%
(1/1)40%
(35/88)50%
(3/6)src.filewidgets68%
(26/38)68%
(26/38)56%
(4641/8273)42%
(2011/4785)src.gui100%
(2/2)100%
(2/2)94%
(102/108)74%
(49/66)src.ioslaves.file100%
(7/7)100%
(7/7)55%
(668/1206)39%
(374/954)src.ioslaves.file.kauth0%
(0/2)0%
(0/2)0%
(0/168)0%
(0/89)src.ioslaves.ftp0%
(0/1)0%
(0/1)0%
(0/1342)0%
(0/1416)src.ioslaves.help0%
(0/5)0%
(0/5)0%
(0/248)0%
(0/148)src.ioslaves.http88%
(7/8)88%
(7/8)42%
(1793/4290)36%
(1314/3670)src.ioslaves.http.kcookiejar33%
(2/6)33%
(2/6)47%
(630/1332)56%
(601/1081)src.ioslaves.remote100%
(2/2)100%
(2/2)27%
(73/267)8%
(14/184)src.ioslaves.remote.kdedmodule0%
(0/2)0%
(0/2)0%
  

D16311: RFC: [KFilePlacesView] Use asynchronous KIO::FileSystemFreeSpaceJob

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:933887dc334f: [KFilePlacesView] Use asynchronous 
KIO::FileSystemFreeSpaceJob (authored by broulik).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D16311?vs=43909=64852#toc

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16311?vs=43909=64852

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

AFFECTED FILES
  src/filewidgets/kfileplacesview.cpp

To: broulik, #frameworks, dfaure, lbeltrame
Cc: davidedmundson, anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D23352: make button icons follow a reasonable size and layout

[Camilo Higuita]

camiloh updated this revision to Diff 64851.
camiloh added a comment.


  correct slip from toolbutton

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23352?vs=64850=64851

BRANCH
  arcpatch-D23352_2

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

AFFECTED FILES
  src/declarativeimports/plasmacomponents3/Button.qml
  src/declarativeimports/plasmacomponents3/ToolButton.qml

To: camiloh, #plasma, mart, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23352: make button icons follow a reasonable size and layout

[Camilo Higuita]

camiloh updated this revision to Diff 64850.
camiloh added a comment.


  suggested changes. use preferred width and height when the icon group height 
and width props are set

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23352?vs=64807=64850

BRANCH
  arcpatch-D23352

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

AFFECTED FILES
  src/declarativeimports/plasmacomponents3/Button.qml
  src/declarativeimports/plasmacomponents3/ToolButton.qml

To: camiloh, #plasma, mart, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23367: make the sizing of the toolbuttons more coherent

[Camilo Higuita]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:069b4e95c155: make the sizing of the toolbuttons more 
coherent (authored by camiloh).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23367?vs=64844=64848

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

AFFECTED FILES
  src/declarativeimports/plasmacomponents3/ToolButton.qml

To: camiloh, #plasma, mart, ngraham
Cc: broulik, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D23344: assert slave command finality

[David Faure]

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


  You put on shows? On TV? :-)

REPOSITORY
  R241 KIO

BRANCH
  assert

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

To: sitter, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23388: add a border for the toolbars WIP

[Marco Martin]

mart added inline comments.

INLINE COMMENTS

> ToolBar.qml:47
> +bottom: control.position == T.ToolBar.Footer || 
> (control.parent.footer && control.parent.footer == control) ? undefined : 
> parent.bottom
> +}
> +}

for now you need for now to add there:
Kirigami.Theme.textColor: theme.textColor

REPOSITORY
  R242 Plasma Framework (Library)

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

To: camiloh, #plasma, mart, ngraham, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23367: make the sizing of the toolbuttons more coherent

[Marco Martin]

mart accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  arcpatch-D23367

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

To: camiloh, #plasma, mart, ngraham
Cc: broulik, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D23352: make button icons follow a reasonable size and layout

[Marco Martin]

mart reopened this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: camiloh, #plasma, mart, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D22144: Add kio recentlyused:/ to access KActivityStats data

[Méven Car]

meven updated this revision to Diff 64845.
meven added a comment.


  Fix path example

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22144?vs=64736=64845

BRANCH
  arcpatch-D22144_1

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

AFFECTED FILES
  CMakeLists.txt
  kio-extras.categories
  recentlyused/CMakeLists.txt
  recentlyused/recentlyused.cpp
  recentlyused/recentlyused.h
  recentlyused/recentlyused.json

To: meven, ivan, #frameworks, ngraham, dfaure
Cc: broulik, dhaumann, elvisangelaccio, kde-frameworks-devel, kfm-devel, 
aprcela, vmarinescu, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D23367: make the sizing of the toolbuttons more coherent

[Camilo Higuita]

camiloh updated this revision to Diff 64844.
camiloh added a comment.


  make the icon fill height and width if the icon group height or width props 
are not set or are 0

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23367?vs=64832=64844

BRANCH
  arcpatch-D23367

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

AFFECTED FILES
  src/declarativeimports/plasmacomponents3/ToolButton.qml

To: camiloh, #plasma, mart, ngraham
Cc: broulik, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

KDE CI: Frameworks » kwayland » kf5-qt5 SUSEQt5.13 - Build # 11 - Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kwayland/job/kf5-qt5%20SUSEQt5.13/11/
 Project:
kf5-qt5 SUSEQt5.13
 Date of build:
Wed, 28 Aug 2019 13:01:46 +
 Build duration:
21 min and counting
   BUILD ARTIFACTS
  abi-compatibility-results.yamlacc/KF5Wayland-5.62.0.xmlcompat_reports/KF5Wayland_compat_report.htmllogs/KF5Wayland/5.62.0/log.txt
   JUnit Tests
  Name: (root) Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot.autotests Failed: 2 test(s), Passed: 44 test(s), Skipped: 0 test(s), Total: 46 test(s)Failed: projectroot.autotests.client.kwayland_testRemoteAccessFailed: projectroot.autotests.client.kwayland_testWaylandSeat
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report63%
(5/8)90%
(240/268)90%
(240/268)85%
(27106/32005)53%
(10891/20532)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests.client100%
(43/43)100%
(43/43)99%
(12423/12524)50%
(6540/13037)autotests.server100%
(5/5)100%
(5/5)99%
(373/376)49%
(177/360)src.client96%
(73/76)96%
(73/76)85%
(6343/7473)65%
(1825/2828)src.compat100%
(2/2)100%
(2/2)100%
(81/81)100%
(0/0)src.server95%
(117/123)95%
(117/123)84%
(7886/9351)64%
(2349/3676)src.tools0%
(0/2)0%
(0/2)0%
(0/785)0%
(0/302)src.tools.testserver0%
(0/3)0%
(0/3)0%
(0/120)0%
(0/14)tests0%
(0/14)0%
(0/14)0%
(0/1295)0%
(0/315)

KDE CI: Frameworks » kwayland » kf5-qt5 SUSEQt5.12 - Build # 55 - Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kwayland/job/kf5-qt5%20SUSEQt5.12/55/
 Project:
kf5-qt5 SUSEQt5.12
 Date of build:
Wed, 28 Aug 2019 13:01:45 +
 Build duration:
20 min and counting
   BUILD ARTIFACTS
  abi-compatibility-results.yamlacc/KF5Wayland-5.62.0.xmlcompat_reports/KF5Wayland_compat_report.htmllogs/KF5Wayland/5.62.0/log.txt
   JUnit Tests
  Name: (root) Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot.autotests Failed: 1 test(s), Passed: 45 test(s), Skipped: 0 test(s), Total: 46 test(s)Failed: projectroot.autotests.client.kwayland_testPlasmaWindowModel
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report63%
(5/8)89%
(238/268)89%
(238/268)82%
(26120/31956)51%
(10402/20532)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests.client98%
(42/43)98%
(42/43)95%
(11835/12520)48%
(6217/13037)autotests.server100%
(5/5)100%
(5/5)99%
(373/376)49%
(177/360)src.client95%
(72/76)95%
(72/76)81%
(6020/7447)59%
(1670/2828)src.compat100%
(2/2)100%
(2/2)100%
(81/81)100%
(0/0)src.server95%
(117/123)95%
(117/123)84%
(7811/9332)64%
(2338/3676)src.tools0%
(0/2)0%
(0/2)0%
(0/785)0%
(0/302)src.tools.testserver0%
(0/3)0%
(0/3)0%
(0/120)0%
(0/14)tests0%
(0/14)0%
(0/14)0%
(0/1295)0%
(0/315)

KDE CI: Frameworks » kwayland » kf5-qt5 FreeBSDQt5.13 - Build # 10 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kwayland/job/kf5-qt5%20FreeBSDQt5.13/10/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Wed, 28 Aug 2019 13:01:46 +
 Build duration:
20 min and counting
   JUnit Tests
  Name: projectroot.autotests Failed: 13 test(s), Passed: 29 test(s), Skipped: 0 test(s), Total: 42 test(s)Failed: projectroot.autotests.client.kwayland_testCompositorFailed: projectroot.autotests.client.kwayland_testDataDeviceFailed: projectroot.autotests.client.kwayland_testDataSourceFailed: projectroot.autotests.client.kwayland_testRegionFailed: projectroot.autotests.client.kwayland_testShmPoolFailed: projectroot.autotests.client.kwayland_testSubCompositorFailed: projectroot.autotests.client.kwayland_testSubSurfaceFailed: projectroot.autotests.client.kwayland_testWaylandConnectionThreadFailed: projectroot.autotests.client.kwayland_testWaylandRegistryFailed: projectroot.autotests.client.kwayland_testWaylandShellFailed: projectroot.autotests.client.kwayland_testWaylandSurfaceFailed: projectroot.autotests.client.kwayland_testXdgShellV5Failed: projectroot.autotests.server.kwayland_testWaylandServerDisplay

D23535: [server] Use cached current mode more and assert validness

[Roman Gilg]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:0c0a2710dd3b: [server] Use cached current mode more and 
assert validness (authored by romangg).

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23535?vs=64833=64843

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

AFFECTED FILES
  src/server/outputdevice_interface.cpp
  src/server/outputdevice_interface.h

To: romangg, #kwin, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23493: [server] Cache current mode

[Roman Gilg]

This revision was automatically updated to reflect the committed changes.
romangg marked an inline comment as done.
Closed by commit R127:ce5e25a4b63c: [server] Cache current mode (authored by 
romangg).

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23493?vs=64740=64838

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

AFFECTED FILES
  src/server/outputdevice_interface.cpp

To: romangg, #kwin, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D22968: Make it possible to modify contacts

[Aleix Pol Gonzalez]

apol updated this revision to Diff 64834.
apol added a comment.


  Address Bhushan's comments

REPOSITORY
  R307 KPeople

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22968?vs=64325=64834

BRANCH
  master

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

AFFECTED FILES
  src/backends/CMakeLists.txt
  src/backends/abstractcontact.cpp
  src/backends/abstractcontact.h
  src/backends/abstracteditablecontact.cpp
  src/backends/abstracteditablecontact.h
  src/backends/basepersonsdatasource.cpp
  src/backends/basepersonsdatasource.h
  src/declarative/peopleqmlplugin.cpp
  src/global.h
  src/persondata.cpp
  src/persondata.h
  src/personmanager_p.h
  src/personpluginmanager.cpp
  src/personpluginmanager.h

To: apol, #frameworks, jbbgameich, bshah
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D22968: Make it possible to modify contacts

[Aleix Pol Gonzalez]

apol marked 3 inline comments as done.

REPOSITORY
  R307 KPeople

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

To: apol, #frameworks, jbbgameich, bshah
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23535: [server] Use cached current mode more and assert validness

[David Edmundson]

davidedmundson accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R127 KWayland

BRANCH
  useCachedMode

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

To: romangg, #kwin, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23493: [server] Cache current mode

[Roman Gilg]

romangg marked 2 inline comments as done.
romangg added inline comments.

INLINE COMMENTS

> romangg wrote in outputdevice_interface.cpp:143
> Thanks. Will put it into a second diff.

See D23535 

REPOSITORY
  R127 KWayland

BRANCH
  cacheMode

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

To: romangg, #kwin, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D23535: [server] Use cached current mode more and assert validness

[Roman Gilg]

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

REVISION SUMMARY
  We can use the cached current mode in one more case. Additionally make it
  more explicit what a valid mode is and when to add them.

TEST PLAN
  Relevant autotests pass. KWin Wayland nested and DRM session work.

REPOSITORY
  R127 KWayland

BRANCH
  useCachedMode

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

AFFECTED FILES
  src/server/outputdevice_interface.cpp
  src/server/outputdevice_interface.h

To: romangg, #kwin
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23367: make the sizing of the toolbuttons more coherent

[Camilo Higuita]

camiloh updated this revision to Diff 64832.
camiloh added a comment.


  make usage of the icon group properties: height and width

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23367?vs=64449=64832

BRANCH
  arcpatch-D23367

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

AFFECTED FILES
  src/declarativeimports/plasmacomponents3/ToolButton.qml

To: camiloh, #plasma, mart, ngraham
Cc: broulik, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D23490: [KUrlNavigator] Add MIME types supported by krarc to isCompressedPath

[N. Higa]

nhiga added a comment.


  When this patch is applied and Krusader is not installed:
  
  Dolphin (with "Open archives as folder" enabled): Opens the 7z/RAR archive 
using Ark.
  Gwenview: 7z/RAR archives will not appear (probably because it is unsupported 
without krarc).

REPOSITORY
  R241 KIO

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

To: nhiga, #frameworks, cfeck, pino, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23490: [KUrlNavigator] Add MIME types supported by krarc to isCompressedPath

[N. Higa]

nhiga edited the summary of this revision.
nhiga edited the test plan for this revision.

REPOSITORY
  R241 KIO

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

To: nhiga, #frameworks, cfeck, pino, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23207: Fixing implementation of FileJob interface in smb/sftp slaves

[Harald Sitter]

sitter added a comment.


  @dfaure this is actually a different beast altogether, the current state 
validation is only run on the API for all slaves, seek/read/write are run in a 
completely different loop which I see isn't being verified at all. I'll add it 
in a separate diff once the assertion is in. Should be fairly trivial. As I 
understand it open/read/write/seek must only ever call error() or their 
specific "send" function (opened()/data()/written()/position()) but never 
finished. Close meanwhile must reach finality (finished or error).

REPOSITORY
  R320 KIO Extras

BRANCH
  fixSFTP (branched from master)

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

To: feverfew, chinmoyr, fvogt, sitter, dfaure
Cc: kde-frameworks-devel, kfm-devel, aprcela, vmarinescu, fprice, LeGast00n, 
MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, 
spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, 
mikesomov

D23344: assert slave command finality

[Harald Sitter]

sitter updated this revision to Diff 64826.
sitter added a comment.


  some days one has to wonder how I manage to put on shows. fix warning branch 
as per David's comments to actually do nothing when the cond was matched

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23344?vs=64744=64826

BRANCH
  assert

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

AFFECTED FILES
  CMakeLists.txt
  src/core/config-kiocore.h.cmake
  src/core/slavebase.cpp

To: sitter, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

KDE CI: Frameworks » kwayland » kf5-qt5 FreeBSDQt5.13 - Build # 9 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kwayland/job/kf5-qt5%20FreeBSDQt5.13/9/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Wed, 28 Aug 2019 11:10:50 +
 Build duration:
7 min 33 sec and counting
   JUnit Tests
  Name: projectroot.autotests Failed: 13 test(s), Passed: 29 test(s), Skipped: 0 test(s), Total: 42 test(s)Failed: projectroot.autotests.client.kwayland_testCompositorFailed: projectroot.autotests.client.kwayland_testDataDeviceFailed: projectroot.autotests.client.kwayland_testDataSourceFailed: projectroot.autotests.client.kwayland_testRegionFailed: projectroot.autotests.client.kwayland_testShmPoolFailed: projectroot.autotests.client.kwayland_testSubCompositorFailed: projectroot.autotests.client.kwayland_testSubSurfaceFailed: projectroot.autotests.client.kwayland_testWaylandConnectionThreadFailed: projectroot.autotests.client.kwayland_testWaylandRegistryFailed: projectroot.autotests.client.kwayland_testWaylandShellFailed: projectroot.autotests.client.kwayland_testWaylandSurfaceFailed: projectroot.autotests.client.kwayland_testXdgShellStableFailed: projectroot.autotests.server.kwayland_testWaylandServerDisplay

D23493: [server] Cache current mode

[Roman Gilg]

romangg added inline comments.

INLINE COMMENTS

> davidedmundson wrote in outputdevice_interface.cpp:143
> we can also just use currentMode directly here and remove the loop above

Thanks. Will put it into a second diff.

REPOSITORY
  R127 KWayland

BRANCH
  cacheMode

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

To: romangg, #kwin, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D23493: [server] Cache current mode

[David Edmundson]

davidedmundson accepted this revision.
davidedmundson added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> outputdevice_interface.cpp:143
>  for (auto it = d->resources.constBegin(); it != 
> d->resources.constEnd(); ++it) {
>  d->sendMode((*it).resource, *currentModeIt);
>  d->sendDone(*it);

we can also just use currentMode directly here and remove the loop above

REPOSITORY
  R127 KWayland

BRANCH
  cacheMode

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

To: romangg, #kwin, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

[Méven Car]

meven updated this revision to Diff 64816.
meven added a comment.


  Make mapConfig return a const ref

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23523?vs=64815=64816

BRANCH
  arcpatch-D23523

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

AFFECTED FILES
  src/core/slavebase.cpp
  src/core/slavebase.h

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

[Méven Car]

meven marked an inline comment as done.

REPOSITORY
  R241 KIO

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> slavebase.h:343
> + */
> +QMap *mapConfig();
> +

Why return a pointer, and why is this method not `const`?
Or is this supposed to be writable? Then I would prefer returning a reference 
to communicate ownership properly.

REPOSITORY
  R241 KIO

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

To: meven, davidedmundson, dfaure, #frameworks
Cc: broulik, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23523: [SlaveBase] Use QMap instead of KConfig to store ioslave config

[Méven Car]

meven created this revision.
meven added reviewers: davidedmundson, dfaure, Frameworks.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
meven requested review of this revision.

REVISION SUMMARY
  Inspired by 
https://github.com/blue-systems/plasma-5.16/issues/139#issuecomment-441704579
  After some review, it seems to me using KConfig is not necessary here.
  Removing its use should improve IO throughput since rebuildConfig is called 
in finished.
  So mark the old config() function as deprecated.
  
  TODO
  
  - Discussion: does it make sense ?
  - benchmark (if anyone has some, it would be very appreciated)
  - port ioslaves in kio and kio-extras to use mapConfig() instead of config()
  - Clean up this description

TEST PLAN
  ctest

REPOSITORY
  R241 KIO

BRANCH
  master

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

AFFECTED FILES
  src/core/slavebase.cpp
  src/core/slavebase.h

To: meven, davidedmundson, dfaure, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

KDE CI: Frameworks » kio » kf5-qt5 SUSEQt5.13 - Build # 49 - Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.13/49/
 Project:
kf5-qt5 SUSEQt5.13
 Date of build:
Wed, 28 Aug 2019 06:49:04 +
 Build duration:
3 hr 10 min and counting
   BUILD ARTIFACTS
  acc/KF5KIO-5.62.0.xmllogs/KF5KIO/5.62.0/log.txt
   JUnit Tests
  Name: (root) Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot Failed: 1 test(s), Passed: 51 test(s), Skipped: 0 test(s), Total: 52 test(s)Failed: projectroot.autotests.kiocore_threadtestName: projectroot.autotests Failed: 0 test(s), Passed: 6 test(s), Skipped: 0 test(s), Total: 6 test(s)Name: projectroot.src.ioslaves.trash Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)Name: projectroot.src.kpasswdserver Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 test(s)
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report64%
(23/36)65%
(263/402)65%
(263/402)55%
(33465/61104)39%
(17025/43924)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests98%
(56/57)98%
(56/57)95%
(9310/9792)47%
(4278/9008)autotests.http100%
(5/5)100%
(5/5)99%
(581/582)68%
(108/160)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.core86%
(100/116)86%
(100/116)59%
(8527/14439)51%
(4683/9191)src.core.kssl100%
(1/1)100%
(1/1)40%
(35/88)50%
(3/6)src.filewidgets68%
(26/38)68%
(26/38)56%
(4632/8258)42%
(2004/4775)src.gui100%
(2/2)100%
(2/2)94%
(102/108)74%
(49/66)src.ioslaves.file100%
(7/7)100%
(7/7)54%
(653/1204)39%
(371/954)src.ioslaves.file.kauth0%
(0/2)0%
(0/2)0%
(0/168)0%
(0/89)src.ioslaves.ftp0%
(0/1)0%
(0/1)0%
(0/1342)0%
(0/1416)src.ioslaves.help0%
(0/5)0%
(0/5)0%
(0/248)0%
(0/148)src.ioslaves.http88%
(7/8)88%
(7/8)42%
(1793/4290)36%
(1314/3670)src.ioslaves.http.kcookiejar33%
(2/6)33%
(2/6)47%
(630/1332)56%
(601/1081)src.ioslaves.remote100%
(2/2)100%
(2/2)27%
(73/267)8%
(14/184)src.ioslaves.remote.kdedmodule0%
(0/2)0%

D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

[Arjen Hiemstra]

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


  I have probably missed stuff, but here are my initial comments.

INLINE COMMENTS

> atticaprovider.cpp:355
> +
> +QList> getCommentsList(const 
> Attica::Comment::List , std::shared_ptr parent) {
> +QList> knsComments;

Since upstream Qt is starting to discourage usage of QList, maybe use QVector 
instead.

> atticaprovider.cpp:359
> +qDebug() << "Appending comment with id" << comment.id() << ", which 
> has" << comment.childCount() << "children";
> +std::shared_ptr knsComment(new KNSCore::Comment());
> +knsComment->id = comment.id();

Generally, it's recommended to use `std::shared_ptr 
knsComment = std::make_shared()`. As a bonus, that allows you 
to use `auto knsComment`.

> atticaprovider.cpp:370
> +if (comment.childCount() > 0) {
> +qDebug() << "Getting more comments, as this one has children, 
> and we currently have this number of comments:" << knsComments.count();
> +knsComments << getCommentsList(comment.children(), knsComment);

Probably better to use categorised logging for this?

> atticaprovider.cpp:410
> +
> +std::shared_ptr author(new KNSCore::Author);
> +author->setId(job->property("username").toString()); // This is a touch 
> hack-like, but it ensures we actually have the data in case it is not 
> returned by the server

Like above, std::make_shared

> commentsmodel.cpp:135
> +{
> +QVariant value;
> +if (index.isValid() && index.row() < d->comments.count()) {

Might want to add a call to checkIndex here 
(https://doc.qt.io/qt-5/qabstractitemmodel.html#checkIndex), it would obsolete 
some additional checking you do later.

> commentsmodel.cpp:177
> +++depth;
> +if (child->parent) {
> +child = child->parent;

Since you're already checking for `child` being true (aka not-null), this if 
becomes rather superfluous.

> commentsmodel.h:50
> + *
> + * This model should preferably be constructed by asking the Engine to give 
> a model
> + * instance to you for a specific entry using the commentsForEntry function. 
> If you

Considering this is intended to be used from QML, I would drop this, especially 
since the current code in engine does nothing special. And changing this would 
allow for doing `CommentsModel { entry: someEntry }` from QML.

> author.cpp:103
> +{
> +d->engine = qobject_cast(newEngine);
> +d->resetConnections();

You should check if the actual property value has changed before changing it. I 
generally do something along the lines of `if (newEngine == d->engine) 
return;`. This helps with preventing binding loops in properties exposed to QML.

> Button.qml:39
> + */
> +property string configFile: ghnsDialog.configFile
> +

Any particular reason not to alias these directly to the dialog?

> Dialog.qml:34
> +import QtQuick.Dialogs 1.3 as QtDialogs
> +import QtQuick.Layouts 1.12 as QtLayouts
> +import org.kde.newstuff 1.1 as NewStuff

If you're using a Qt 5.12 import anyway, might want to unify the other imports 
to also use .12 versions.

> Dialog.qml:76
> + */
> +signal dialogFinished(var changedEntries);
> +

Any particular reason for using a signal parameter here instead of just setting 
a property on the dialog? Using a property generally allows for more 
declarative use of the item.

> DownloadItemsSheet.qml:49
> +}
> +Kirigami.AbstractCard {
> +QtLayouts.Layout.leftMargin: Kirigami.Units.iconSizes.smallMedium

Design wise, I don't think having a card inside an overlaysheet looks too nice. 
I'm also not sure if the extra attention is needed, so maybe just use a label?

> Page.qml:210
> +
> +Component {
> +id: bigPreviewDelegate

For readability, it would be better to move these to their own files.

> qmlplugin.cpp:54
> +Q_UNUSED(scriptEngine)
> +return KNewStuffQuick::QuickQuestionListener::instance();
> +});

Note that this makes the qml engine take ownvership of your singleton instance 
and it will try to delete it when the engine gets deleted. You probably want to 
do `engine->setOwnership(instance, QQmlEngine::CppOwnership)` before returning 
the instance.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, 
GB_2, michaelh, bruns

D22143: Add proper logging using ECMQtDeclareLoggingCategory

[Méven Car]

This revision was automatically updated to reflect the committed changes.
Closed by commit R159:2bb035ff67fd: Add proper logging using 
ECMQtDeclareLoggingCategory (authored by meven).

REPOSITORY
  R159 KActivities Statistics

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22143?vs=64789=64808

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

AFFECTED FILES
  CMakeLists.txt
  autotests/CMakeLists.txt
  kactivities-stats.categories
  src/CMakeLists.txt
  src/common/database/Database.cpp
  src/resultmodel.cpp
  src/resultset.cpp
  src/resultwatcher.cpp

To: meven, ivan, #frameworks, kossebau
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23352: make button icons follow a reasonable size and layout

[Marco Martin]

This revision was not accepted when it landed; it landed in state "Needs 
Revision".
This revision was automatically updated to reflect the committed changes.
Closed by commit R242:c6c882521f80: Revert make the sizing of the 
toolbuttons more coherent (authored by mart).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D23352?vs=64366=64807#toc

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23352?vs=64366=64807

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

AFFECTED FILES
  src/declarativeimports/plasmacomponents3/ToolButton.qml

To: camiloh, #plasma, mart, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D22144: Add kio recentlyused:/ to access KActivityStats data

[Méven Car]

meven added a comment.


  In D22144#520684 , @dfaure wrote:
  
  > Yep. Either query argument, or KIO metaData(). Both require explicit action 
from the application, there's no magic currently to know which app is making 
the request. We could add that, but there are so many way to identify an 
application that I'm not sure it would work for all cases.
  
  
  This "magic" precisely is what I would have wished to find, it seemed to me 
something other ioslaves might want, to have contextual log or error message at 
least.
  The pid of the calling application could be a minimum.
  The application parameter is already here, it is called agent here.
  It will be on KFileWidget to fill this parameter, which should be fine.
  Anyway that's another day issue.
  
  Should I wait a few days more days for review, or merge to broaden the 
testing population ?
  D7446  is the next step

REPOSITORY
  R320 KIO Extras

BRANCH
  arcpatch-D22144_1

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

To: meven, ivan, #frameworks, ngraham, dfaure
Cc: broulik, dhaumann, elvisangelaccio, kde-frameworks-devel, kfm-devel, 
aprcela, vmarinescu, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D23352: make button icons follow a reasonable size and layout

[Marco Martin]

mart added inline comments.

INLINE COMMENTS

> Button.qml:53
> +id: icon
> +anchors.centerIn: parent
> +visible: parent.visible

no anchors and no enclosing Item.

This was ok as it was, including fillwidth/fillheight (perhaps)
but with the line:
Layout.preferredWidth: control.icon.width > 0 ? control.icon.width : -1
same for preferredHeight

REPOSITORY
  R242 Plasma Framework (Library)

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

To: camiloh, #plasma, mart, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23367: make the sizing of the toolbuttons more coherent

[Marco Martin]

mart added inline comments.

INLINE COMMENTS

> ToolButton.qml:49
> +
> +Item {
> +Layout.preferredWidth: Kirigami.Units.iconSizes.smallMedium + 
> Kirigami.Units.largeSpacing

this further item enclosing is not necessary, is an object instantation more 
for no functional gain

> ToolButton.qml:55
> +id: icon
> +anchors.centerIn: parent
> +height: Kirigami.Units.iconSizes.smallMedium

This was ok as it was, including fillwidth/fillheight (perhaps)
but with the line:
Layout.preferredWidth: control.icon.width > 0 ? control.icon.width : -1
same for preferredHeight

REPOSITORY
  R242 Plasma Framework (Library)

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

To: camiloh, #plasma, mart, ngraham
Cc: broulik, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D23367: make the sizing of the toolbuttons more coherent

[Marco Martin]

mart added a comment.


  In D23367#517658 , @ngraham wrote:
  
  > Not that it's an inappropriate fix, but are you sure you want to be using 
PlasmaComponents ToolButtons in Index? Why not use regular old QQC2 ToolButtons?
  
  
  The phone is using the plasma style for its qqc2

REPOSITORY
  R242 Plasma Framework (Library)

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

To: camiloh, #plasma, mart, ngraham
Cc: broulik, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

KDE CI: Frameworks » knotifications » kf5-qt5 WindowsMSVCQt5.13 - Build # 12 - Still unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/knotifications/job/kf5-qt5%20WindowsMSVCQt5.13/12/
 Project:
kf5-qt5 WindowsMSVCQt5.13
 Date of build:
Mon, 26 Aug 2019 17:34:06 +
 Build duration:
1 day 15 hr and counting
   JUnit Tests
  Name: projectroot Failed: 1 test(s), Passed: 0 test(s), Skipped: 0 test(s), Total: 1 test(s)Failed: projectroot.autotests.KNotificationTest

D23352: make button icons follow a reasonable size and layout

[Marco Martin]

mart requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: camiloh, #plasma, mart, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23367: make the sizing of the toolbuttons more coherent

[Marco Martin]

mart requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: camiloh, #plasma, mart, ngraham
Cc: broulik, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D23352: make button icons follow a reasonable size and layout

[Marco Martin]

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


  Same problem as the toolbutton one, button icons need to scale unless icon 
size fixed from client side

REPOSITORY
  R242 Plasma Framework (Library)

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

To: camiloh, #plasma, mart, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23367: make the sizing of the toolbuttons more coherent

[Marco Martin]

mart reopened this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: camiloh, #plasma, mart, ngraham
Cc: broulik, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D23367: make the sizing of the toolbuttons more coherent

[Marco Martin]

mart added inline comments.

INLINE COMMENTS

> ToolButton.qml:56
> +anchors.centerIn: parent
> +height: Kirigami.Units.iconSizes.smallMedium
> +width:  Kirigami.Units.iconSizes.smallMedium

This fixes the icons to a single size no matter what, and wether it fits or 
not, and is going to break badly any time a bigger or smaller buttons is 
needed, as shown by breaking media controller.

I have to revert this now, the icon should scale only to "standard" sizes, but 
still scale with the button size.
if you really need fixed icons no matter what, this size hint may follow 
icon.width/icon.height from the control iff set to a valid measure

REPOSITORY
  R242 Plasma Framework (Library)

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

To: camiloh, #plasma, mart, ngraham
Cc: broulik, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns