Re: KDE CI: Frameworks » kquickcharts » kf5-qt5 WindowsMSVCQt5.15 - Build # 101 - Still Failing!

[Arjen Hiemstra]

On Monday, 14 March 2022 11:18:48 CET Arjen Hiemstra wrote:
> On Monday, 14 March 2022 09:30:09 CET Ben Cooksley wrote:
> > Hi all,
> > 
> > Can someone please confirm whether it is expected that KQuickCharts is
> > Linux/FreeBSD only given that it is QtQuick based (and therefore should be
> > fairly platform agnostic?)
> 
> It should work fine on Windows or Mac, I have just never tested it properly
> so I didn't list it in the "supported platforms" metadata. Now that that's
> used more aggressively the other platforms should probably just be added.
> 

Done that now with https://invent.kde.org/frameworks/kquickcharts/commit/
6214f29960a35f2d418cf95a6c4e60512f3516c0

> - Arjen
> 
> > Cheers,
> > Ben
> > 
> > On Mon, Mar 14, 2022 at 6:01 PM CI System  wrote:
> > > *BUILD FAILURE*
> > > Build URL
> > > https://build.kde.org/job/Frameworks/job/kquickcharts/job/kf5-qt5%20Wind
> > > ow
> > > sMSVCQt5.15/101/ Project: kf5-qt5 WindowsMSVCQt5.15
> > > Date of build: Mon, 14 Mar 2022 05:00:46 +
> > > Build duration: 23 sec and counting
> > > * CONSOLE OUTPUT *
> > > [...truncated 140 lines...]
> > > [2022-03-14T05:01:02.160Z] PROCESSOR_REVISION = '0102'
> > > [2022-03-14T05:01:02.160Z] PROGRAMDATA = 'C:\ProgramData'
> > > [2022-03-14T05:01:02.160Z] PROGRAMFILES = 'C:\Program Files'
> > > [2022-03-14T05:01:02.160Z] PROGRAMFILES(X86) = 'C:\Program Files (x86)'
> > > [2022-03-14T05:01:02.160Z] PROGRAMW6432 = 'C:\Program Files'
> > > [2022-03-14T05:01:02.160Z] PROMPT = '$P$G'
> > > [2022-03-14T05:01:02.160Z] PSMODULEPATH =
> > > '%ProgramFiles%\WindowsPowerShell\Modules;C:\WINDOWS\system32\WindowsPow
> > > er
> > > Shell\v1.0\Modules' [2022-03-14T05:01:02.160Z] PUBLIC =
> > > 'C:\Users\Public'
> > > [2022-03-14T05:01:02.160Z] RUN_ARTIFACTS_DISPLAY_URL = '
> > > https://build.kde.org/job/Frameworks/job/kquickcharts/job/kf5-qt5%20Wind
> > > ow
> > > sMSVCQt5.15/101/display/redirect?page=artifacts '
> > > [2022-03-14T05:01:02.160Z] RUN_CHANGES_DISPLAY_URL = '
> > > https://build.kde.org/job/Frameworks/job/kquickcharts/job/kf5-qt5%20Wind
> > > ow
> > > sMSVCQt5.15/101/display/redirect?page=changes '
> > > [2022-03-14T05:01:02.160Z] RUN_DISPLAY_URL = '
> > > https://build.kde.org/job/Frameworks/job/kquickcharts/job/kf5-qt5%20Wind
> > > ow
> > > sMSVCQt5.15/101/display/redirect '
> > > [2022-03-14T05:01:02.160Z] RUN_TESTS_DISPLAY_URL = '
> > > https://build.kde.org/job/Frameworks/job/kquickcharts/job/kf5-qt5%20Wind
> > > ow
> > > sMSVCQt5.15/101/display/redirect?page=tests '
> > > [2022-03-14T05:01:02.160Z] STAGE_NAME = 'Configuring Build'
> > > [2022-03-14T05:01:02.160Z] SYSTEMDRIVE = 'C:'
> > > [2022-03-14T05:01:02.160Z] SYSTEMROOT = 'C:\WINDOWS'
> > > [2022-03-14T05:01:02.160Z] TEMP = 'C:\Users\Jenkins\AppData\Local\Temp'
> > > [2022-03-14T05:01:02.160Z] TMP = 'C:\Users\Jenkins\AppData\Local\Temp'
> > > [2022-03-14T05:01:02.160Z] UCRTVERSION = '10.0.19041.0'
> > > [2022-03-14T05:01:02.160Z] UNIVERSALCRTSDKDIR = 'C:\Program Files
> > > (x86)\Windows Kits\10\'
> > > [2022-03-14T05:01:02.160Z] USERDOMAIN = 'DESKTOP-9TVNRIT'
> > > [2022-03-14T05:01:02.160Z] USERNAME = 'Jenkins'
> > > [2022-03-14T05:01:02.160Z] USERPROFILE = 'C:\Users\Jenkins'
> > > [2022-03-14T05:01:02.160Z] VCIDEINSTALLDIR = 'C:\Program Files
> > > (x86)\Microsoft Visual Studio\2019\Professional\Common7\IDE\VC\'
> > > [2022-03-14T05:01:02.160Z] VCINSTALLDIR = 'C:\Program Files
> > > (x86)\Microsoft Visual Studio\2019\Professional\VC\'
> > > [2022-03-14T05:01:02.160Z] VCTOOLSINSTALLDIR = 'C:\Program Files
> > > (x86)\Microsoft Visual
> > > Studio\2019\Professional\VC\Tools\MSVC\14.29.30037\'
> > > [2022-03-14T05:01:02.160Z] VCTOOLSREDISTDIR = 'C:\Program Files
> > > (x86)\Microsoft Visual
> > > Studio\2019\Professional\VC\Redist\MSVC\14.29.30036\'
> > > [2022-03-14T05:01:02.160Z] VCTOOLSVERSION = '14.29.30037'
> > > [2022-03-14T05:01:02.160Z] VISUALSTUDIOVERSION = '16.0'
> > > [2022-03-14T05:01:02.160Z] VS160COMNTOOLS = 'C:\Program Files
> > > (x86)\Microsoft Visual Studio\2019\Professional\Common7\Tools\'
> > > [2022-03-14T05:01:02.160Z] VSCMD_ARG_APP_PLAT = 'Desktop'
> > > [2022-03-14T05:01:02.160Z] VSCMD_ARG_HOST_ARCH = 'x64'
> > > [2022-03-14T05:01:02.160Z] VSCMD_ARG_TGT_ARCH = 'x64'
> > > [2022-03-14T05:01:02.160Z] VSCMD_VER = '16.10.2'
> > > [2022-03-14T05:01:02.160Z] VSINSTAL

Re: KDE CI: Frameworks » kquickcharts » kf5-qt5 WindowsMSVCQt5.15 - Build # 101 - Still Failing!

[Arjen Hiemstra]

On Monday, 14 March 2022 09:30:09 CET Ben Cooksley wrote:
> Hi all,
> 
> Can someone please confirm whether it is expected that KQuickCharts is
> Linux/FreeBSD only given that it is QtQuick based (and therefore should be
> fairly platform agnostic?)
> 

It should work fine on Windows or Mac, I have just never tested it properly so 
I didn't list it in the "supported platforms" metadata. Now that that's used 
more aggressively the other platforms should probably just be added.

- Arjen

> Cheers,
> Ben
> 
> On Mon, Mar 14, 2022 at 6:01 PM CI System  wrote:
> > *BUILD FAILURE*
> > Build URL
> > https://build.kde.org/job/Frameworks/job/kquickcharts/job/kf5-qt5%20Window
> > sMSVCQt5.15/101/ Project: kf5-qt5 WindowsMSVCQt5.15
> > Date of build: Mon, 14 Mar 2022 05:00:46 +
> > Build duration: 23 sec and counting
> > * CONSOLE OUTPUT *
> > [...truncated 140 lines...]
> > [2022-03-14T05:01:02.160Z] PROCESSOR_REVISION = '0102'
> > [2022-03-14T05:01:02.160Z] PROGRAMDATA = 'C:\ProgramData'
> > [2022-03-14T05:01:02.160Z] PROGRAMFILES = 'C:\Program Files'
> > [2022-03-14T05:01:02.160Z] PROGRAMFILES(X86) = 'C:\Program Files (x86)'
> > [2022-03-14T05:01:02.160Z] PROGRAMW6432 = 'C:\Program Files'
> > [2022-03-14T05:01:02.160Z] PROMPT = '$P$G'
> > [2022-03-14T05:01:02.160Z] PSMODULEPATH =
> > '%ProgramFiles%\WindowsPowerShell\Modules;C:\WINDOWS\system32\WindowsPower
> > Shell\v1.0\Modules' [2022-03-14T05:01:02.160Z] PUBLIC = 'C:\Users\Public'
> > [2022-03-14T05:01:02.160Z] RUN_ARTIFACTS_DISPLAY_URL = '
> > https://build.kde.org/job/Frameworks/job/kquickcharts/job/kf5-qt5%20Window
> > sMSVCQt5.15/101/display/redirect?page=artifacts '
> > [2022-03-14T05:01:02.160Z] RUN_CHANGES_DISPLAY_URL = '
> > https://build.kde.org/job/Frameworks/job/kquickcharts/job/kf5-qt5%20Window
> > sMSVCQt5.15/101/display/redirect?page=changes '
> > [2022-03-14T05:01:02.160Z] RUN_DISPLAY_URL = '
> > https://build.kde.org/job/Frameworks/job/kquickcharts/job/kf5-qt5%20Window
> > sMSVCQt5.15/101/display/redirect '
> > [2022-03-14T05:01:02.160Z] RUN_TESTS_DISPLAY_URL = '
> > https://build.kde.org/job/Frameworks/job/kquickcharts/job/kf5-qt5%20Window
> > sMSVCQt5.15/101/display/redirect?page=tests '
> > [2022-03-14T05:01:02.160Z] STAGE_NAME = 'Configuring Build'
> > [2022-03-14T05:01:02.160Z] SYSTEMDRIVE = 'C:'
> > [2022-03-14T05:01:02.160Z] SYSTEMROOT = 'C:\WINDOWS'
> > [2022-03-14T05:01:02.160Z] TEMP = 'C:\Users\Jenkins\AppData\Local\Temp'
> > [2022-03-14T05:01:02.160Z] TMP = 'C:\Users\Jenkins\AppData\Local\Temp'
> > [2022-03-14T05:01:02.160Z] UCRTVERSION = '10.0.19041.0'
> > [2022-03-14T05:01:02.160Z] UNIVERSALCRTSDKDIR = 'C:\Program Files
> > (x86)\Windows Kits\10\'
> > [2022-03-14T05:01:02.160Z] USERDOMAIN = 'DESKTOP-9TVNRIT'
> > [2022-03-14T05:01:02.160Z] USERNAME = 'Jenkins'
> > [2022-03-14T05:01:02.160Z] USERPROFILE = 'C:\Users\Jenkins'
> > [2022-03-14T05:01:02.160Z] VCIDEINSTALLDIR = 'C:\Program Files
> > (x86)\Microsoft Visual Studio\2019\Professional\Common7\IDE\VC\'
> > [2022-03-14T05:01:02.160Z] VCINSTALLDIR = 'C:\Program Files
> > (x86)\Microsoft Visual Studio\2019\Professional\VC\'
> > [2022-03-14T05:01:02.160Z] VCTOOLSINSTALLDIR = 'C:\Program Files
> > (x86)\Microsoft Visual
> > Studio\2019\Professional\VC\Tools\MSVC\14.29.30037\'
> > [2022-03-14T05:01:02.160Z] VCTOOLSREDISTDIR = 'C:\Program Files
> > (x86)\Microsoft Visual
> > Studio\2019\Professional\VC\Redist\MSVC\14.29.30036\'
> > [2022-03-14T05:01:02.160Z] VCTOOLSVERSION = '14.29.30037'
> > [2022-03-14T05:01:02.160Z] VISUALSTUDIOVERSION = '16.0'
> > [2022-03-14T05:01:02.160Z] VS160COMNTOOLS = 'C:\Program Files
> > (x86)\Microsoft Visual Studio\2019\Professional\Common7\Tools\'
> > [2022-03-14T05:01:02.160Z] VSCMD_ARG_APP_PLAT = 'Desktop'
> > [2022-03-14T05:01:02.160Z] VSCMD_ARG_HOST_ARCH = 'x64'
> > [2022-03-14T05:01:02.160Z] VSCMD_ARG_TGT_ARCH = 'x64'
> > [2022-03-14T05:01:02.160Z] VSCMD_VER = '16.10.2'
> > [2022-03-14T05:01:02.160Z] VSINSTALLDIR = 'C:\Program Files
> > (x86)\Microsoft Visual Studio\2019\Professional\'
> > [2022-03-14T05:01:02.160Z] WINDIR = 'C:\WINDOWS'
> > [2022-03-14T05:01:02.160Z] WINDOWSLIBPATH = 'C:\Program Files
> > (x86)\Windows Kits\10\UnionMetadata\10.0.19041.0;C:\Program Files
> > (x86)\Windows Kits\10\References\10.0.19041.0'
> > [2022-03-14T05:01:02.160Z] WINDOWSSDKBINPATH = 'C:\Program Files
> > (x86)\Windows Kits\10\bin\'
> > [2022-03-14T05:01:02.160Z] WINDOWSSDKDIR = 'C:\Program Files (x86)\Windows
> > Kits\10\'
> > [2022-03-14T05:01:02.160Z] WINDOWSSDKLIBVERSION = '10.0.19041.0\'
> > [2022-03-14T05:01:02.161Z] WINDOWSSDKVERBINPATH = 'C:\Program Files
> > (x86)\Windows Kits\10\bin\10.0.19041.0\'
> > [2022-03-14T05:01:02.161Z] WINDOWSSDKVERSION = '10.0.19041.0\'
> > [2022-03-14T05:01:02.161Z] WORKSPACE = 'C:/CI/Job Build'
> > [2022-03-14T05:01:02.161Z] WORKSPACE_TMP = 'C:/CI/Job Build@tmp'
> > [2022-03-14T05:01:02.161Z] __DEVINIT_PATH = 'C:\Program Files
> > (x86)\Microsoft Visual
> > Studio\2019\Professional\Common7\Tools\devinit\devinit.exe'
> > 

Re: Urgent Kirigami issue (Fwd: Re: 3 respins for 5.91.0)

[Arjen Hiemstra]

This should be fixed on master actually with https://invent.kde.org/frameworks/
kirigami/-/commit/321b49a8fc1c9fa936400713f271e590a3914b27 .

Though I think I forgot to mention that it is needed, sorry about that.

- Arjen

On Saturday, 12 February 2022 15:03:03 CET David Faure wrote:
> --  Forwarded Message  --
> 
> Subject: Re: 3 respins for 5.91.0
> Date: samedi 12 février 2022, 12:48:15 CET
> From: Tobias C. Berner 
> To: David Faure 
> 
> Moin moin
> 
> kirigami2 seems to fail with ninja now:
> 
> CMake Error:
>  Running
> 
>   '/usr/local/bin/ninja' '-C'
> '/wrkdirs/usr/ports/x11-toolkits/kf5-kirigami2/work/.build' '-t'
> 'recompact'
> 
>  failed with:
> 
>   ninja: error: build.ninja:1504: multiple rules generate
> src/org/kde/kirigami.2/swipenavigator/PageTab.qml [
> -w dupbuild=err]
> 
> 
> mfg Tobias
> 
> On Sat, 12 Feb 2022 at 11:04, David Faure  wrote:
> > Many severe bugs found and fixed this week...
> > 
> > kirigami v5.91.0-rc3
> > e44ad90a733b476fda87ce9dec462c118911ed2e
> > 083204c5f3d12fc85e24d0e1763c935120efcf6d69482bd9e19b0e97bf8822a3  sources/
> 
> kirigami2-5.91.0.tar.xz
> 
> > due to https://invent.kde.org/frameworks/kirigami/-/merge_requests/
> 
> 494#note_396227
> 
> > kio v5.91.0-rc2
> > 7571449b2fb5b960db693de443fe319bb76c4f8b
> > f2ddea299f3dc98835df445dd1d622d90233115ad0200e75da5e954523466293  sources/
> 
> kio-5.91.0.tar.xz
> 
> > due to https://invent.kde.org/frameworks/kio/-/merge_requests/
> 
> 752#note_396030
> 
> > knewstuff v5.91.0-rc2
> > 2a1129c5686f5c278cd661532265531da8550ead
> > 5dd9fb32fe7e99b64f8dc4b8801bbdba5dc5ba2eda9bec2fb1fc563a53ec6a2a  sources/
> 
> knewstuff-5.91.0.tar.xz
> 
> > due to https://marc.info/?l=kde-frameworks-devel=164445401017044=2
> > 
> > Thanks for updating the packages. I'll make the tarballs public
> > in 8 hours or so, to give you time to do that.
> > 
> > --
> > David Faure, fa...@kde.org, http://www.davidfaure.fr
> > Working on KDE Frameworks 5
> 
> -

Re: Kirigami Respin

[Arjen Hiemstra]

On Monday, 7 February 2022 12:15:39 CET David Faure wrote:
> On lundi 7 février 2022 00:56:50 CET Arjen Hiemstra wrote:
> > Hi,
> > 
> > Any chance of a respin for Kirigami? https://invent.kde.org/frameworks/
> > kirigami/-/commit/5e2416257bc5b07d3468647204671a6d360fbd1b fixes an issue
> > where some files are installed to the wrong location.
> 
> Done:
> 
> kirigami v5.91.0-rc2
> bdb5605880696b48dd7dd8bdd36699773451537a
> ad6a810b00a5a74ddb1598b46253abc979f526512884507106007f0c2efd4f4f 
> sources/kirigami2-5.91.0.tar.xz

Thank you!

Kirigami Respin

[Arjen Hiemstra]

Hi,

Any chance of a respin for Kirigami? https://invent.kde.org/frameworks/
kirigami/-/commit/5e2416257bc5b07d3468647204671a6d360fbd1b fixes an issue where 
some files are installed to the wrong location.

Thanks,
Arjen

D29534: Implement scroll and drag adjustment of values for SpinBox control

[Arjen Hiemstra]

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

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  implement-spinbox-wheel-adjustment (branched from master)

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

To: ngraham, #vdg, #plasma, ahiemstra
Cc: ahiemstra, abetts, ndavis, cblack, kde-frameworks-devel, LeGast00n, 
michaelh, ngraham, bruns

D27805: Add a screenshot capture item and use it to test rendering

[Arjen Hiemstra]

ahiemstra abandoned this revision.
ahiemstra added a comment.


  Will continue this on Gitlab.

REPOSITORY
  R1049 KQuickCharts

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

To: ahiemstra
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29742: Avoid potential disconnect of all signals in IconItem

[Arjen Hiemstra]

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


  This fixes bug 421170 for me.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

To: davidedmundson, #plasma, ahiemstra
Cc: ahiemstra, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29534: Implement scroll and drag adjustment of values for SpinBox control

[Arjen Hiemstra]

ahiemstra added inline comments.

INLINE COMMENTS

> SpinBox.qml:54
> +// Because we're stomping the Text Input's own mouse handling
> +textField.forceActiveFocus()
> +}

You can do `mouse.accepted = false` instead of forceActiveFocus to allow the 
mouse press to go to items below it. However, you might instead want to use 
DragHandler's onActiveChanged for this, as that signals when the actual drag is 
active.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #vdg, #plasma
Cc: ahiemstra, abetts, ndavis, cblack, kde-frameworks-devel, LeGast00n, 
michaelh, ngraham, bruns

D29527: Add MapProxySource

[Arjen Hiemstra]

This revision was automatically updated to reflect the committed changes.
Closed by commit R1049:fa334cf9e809: Add MapProxySource (authored by ahiemstra).

REPOSITORY
  R1049 KQuickCharts

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29527?vs=82256=82278

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/MapProxySourceTest.cpp
  src/CMakeLists.txt
  src/ChartsPlugin.cpp
  src/datasource/MapProxySource.cpp
  src/datasource/MapProxySource.h

To: ahiemstra, mart
Cc: mart, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29527: Add MapProxySource

[Arjen Hiemstra]

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

REVISION SUMMARY
  A source that provides values from a map based on keys from a different
  source.

TEST PLAN
  The auto test passes.

REPOSITORY
  R1049 KQuickCharts

BRANCH
  mapproxysource

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/MapProxySourceTest.cpp
  src/CMakeLists.txt
  src/ChartsPlugin.cpp
  src/datasource/MapProxySource.cpp
  src/datasource/MapProxySource.h

To: ahiemstra
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D25814: [KColorScheme] Add SeparatorColor

[Arjen Hiemstra]

ahiemstra added a comment.


  I actually ran into this with the new Card designs I implemented in Kirigami. 
Kirigami right now does a lot of `tint(backgroundColor, textColor)` to generate 
separator colours. However, this leads to potential differences when the color 
set is View vs. Window or other cases, making separators looks slightly 
different based on where they are used. I don't think that's a good idea 
personally and in any case, having an explicit separator color would make it an 
explicit design decision instead of an implicit technical artifact. So +1 from 
my side for adding this.

REPOSITORY
  R265 KConfigWidgets

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

To: ndavis, #frameworks, #vdg
Cc: ahiemstra, broulik, manueljlin, alexde, ngraham, davidedmundson, filipf, 
cfeck, hpereiradacosta, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D28220: Switch to using Kirigami's ShadowedRectangle

[Arjen Hiemstra]

ahiemstra added a comment.


  It's fine with me but I'm going to leave the final call to @broulik since he 
did a more in-depth review.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #frameworks, #plasma, ahiemstra, broulik, mart, #vdg
Cc: davidedmundson, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, bruns

D28372: Added a merged look to the plasmoidheading and remove roundedborders

[Arjen Hiemstra]

ahiemstra added subscribers: ndavis, ahiemstra.
ahiemstra added a comment.


  The same thing was actually proposed by I think @ndavis a while back, see 
https://phabricator.kde.org/T10470#219994 .
  
  To summarize, it's perfectly technically possible and I actually rather like 
it.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #plasma
Cc: ahiemstra, ndavis, ngraham, mart, davidedmundson, kde-frameworks-devel, 
LeGast00n, cblack, GB_2, michaelh, bruns

D28220: Switch to using Kirigami's ShadowedRectangle

[Arjen Hiemstra]

ahiemstra added inline comments.

INLINE COMMENTS

> EntryScreenshots.qml:134
> +Kirigami.Theme.colorSet: Kirigami.Theme.View
> +shadow.xOffset: 0
> +shadow.yOffset: 0

The x and y offsets should be 0 by default, so there's little reason to specify 
that I'd say.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #frameworks, #plasma, ahiemstra, broulik, mart, #vdg
Cc: davidedmundson, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, bruns

D27544: Fix update scenarios with no explicit downloadlink selected

[Arjen Hiemstra]

ahiemstra accepted this revision.

REPOSITORY
  R304 KNewStuff

BRANCH
  fix-update (branched from master)

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

To: leinir, #knewstuff, #frameworks, #plasma, ngraham, apol, 
#discover_software_store, ahiemstra
Cc: ahiemstra, alexde, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27544: Fix update scenarios with no explicit downloadlink selected

[Arjen Hiemstra]

ahiemstra added inline comments.

INLINE COMMENTS

> engine.cpp:614
> +QString identifiedLink;
> +const QString& payloadToIdentify = d->payloadToIdentify[entry];
> +const QStringList& payloads = d->payloads[entry];

Code style: & attaches to the name, not the type. (Yes I hate it too).

There's a few instances of this around.

> engine.cpp:621
> +// Next simplest option, filename is the same but in a 
> different folder
> +const QStringRef& fileName = 
> payloadToIdentify.splitRef(QChar::fromLatin1('/')).last();
> +for (const QString& payload : payloads) {

This makes a reference to a temporary which I'm not sure is a good idea. Since 
you are already using splitRef, just make a copy.

> engine.cpp:631
> +// Least simple option, no match - ask the user to pick 
> (and if we still haven't got one... that's us done, no installation)
> +Question* question = new 
> Question(Question::SelectFromListQuestion, this);
> +question->setTitle(i18n("Pick Update Item"));

This object will linger until the engine is destroyed, which seems like a 
suboptimal thing. Maybe better to do:

  auto question = std::make_unique(Question::SelectFromListQuestion);

then it will be automatically cleaned up once you exit the scope.

> engine.cpp:644
> +m_installation->install(theEntry);
> +}
> +// As the serverside data may change before next time this is 
> called, even in the same session,

You may want to log something in the else here, at least then it will be 
possible to figure out why an update is not happening.

> EntryDetails.qml:101
>  if (component.downloadCount == 1) {
> -newStuffModel.installItem(component.index);
> +newStuffModel.installItem(component.index, 1);
>  } else {

That "1" here is a bit mysterious, what does it actually mean?

> quickitemsmodel.h:154
>   * @param index The index of the item to install or update
> + * @param linkId The download item to install. If this is -1, it is 
> assumed to be an update with an unknown payload, and a number of heuristics 
> will be applied by the engine
> + * @see Engine::downloadLinkLoaded implementation for details

Rather than using -1, you could add an enum that defines these values a bit 
more, like:

  enum LinkId {
  AutoDetectLink = -1,
  FirstItem = 1
  }

You can then also expose that to QML to avoid the magical 1 up there.

REPOSITORY
  R304 KNewStuff

BRANCH
  fix-update (branched from master)

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

To: leinir, #knewstuff, #frameworks, #plasma, ngraham, apol, 
#discover_software_store
Cc: ahiemstra, alexde, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27805: Add a screenshot capture item and use it to test rendering

[Arjen Hiemstra]

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

REVISION SUMMARY
  This adds the ItemCapture item to the test runner, which can be
  used to capture images of items and compare them to a previous 
  version. It also updates the three chart tests to make use of
  this and adds the images for these tests.

TEST PLAN
  Autotests still pass.

REPOSITORY
  R1049 KQuickCharts

BRANCH
  screenshot_autotest

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/ChartTestCase.qml
  autotests/ItemCapture.cpp
  autotests/ItemCapture.h
  autotests/qmltest.cpp
  autotests/screenshots/BarChart_simple.png
  autotests/screenshots/LineChart_simple.png
  autotests/screenshots/PieChart_model.png
  autotests/screenshots/PieChart_multiValue.png
  autotests/screenshots/PieChart_simple.png
  autotests/tst_BarChart.qml
  autotests/tst_LineChart.qml
  autotests/tst_PieChart.qml

To: ahiemstra
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27695: WIP: Create a new TopArea element using widgets/toparea svg

[Arjen Hiemstra]

ahiemstra added inline comments.

INLINE COMMENTS

> TopArea.qml:40
> +rightInset: -topAreaSvg.margins.right
> +topInset: -topAreaSvg.margins.bottom ? flipped : -topAreaSvg.margins.top
> +bottomInset: -topAreaSvg.margins.bottom ? flipped : 
> -topAreaSvg.margins.top

This conditional is wrong. The syntax is `condition ? true_case : false_case` 
so you want to write `flipped ? -topAreaSvg.margins.top : 
-topAreaSvg.margins.bottom` or something similar.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, mart
Cc: ahiemstra, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D25794: ArraySourceTest: Use QTEST_GUILESS_MAIN

[Arjen Hiemstra]

ahiemstra added a comment.


  @heikobecker This patch has been open for a long time... Do you have commit 
access or do you need someone to commit it for you?

BRANCH
  master

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

To: heikobecker, #frameworks, ahiemstra, davidedmundson

D25795: Rename kf5quickcharts_example to kquickcharts_example

[Arjen Hiemstra]

ahiemstra added a comment.


  @heikobecker This patch has been open for a long time... Do you have commit 
access or do you need someone to commit it for you?

BRANCH
  master

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

To: heikobecker, #frameworks, ahiemstra, davidedmundson

D25326: Move Plasma's SortFilterProxyModel into KItemModel's QML plugin

[Arjen Hiemstra]

ahiemstra updated this revision to Diff 74601.
ahiemstra added a comment.


  - Use QQmlParserStatus to slightly delay syncing of role names

REPOSITORY
  R275 KItemModels

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25326?vs=74592=74601

BRANCH
  arcpatch-D25326

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/ksortfilterproxymodel_qml.cpp
  src/qml/CMakeLists.txt
  src/qml/ksortfilterproxymodel.cpp
  src/qml/ksortfilterproxymodel.h
  src/qml/plugin.cpp

To: davidedmundson
Cc: kmaterka, iasensio, ahmadsamir, broulik, ahiemstra, mart, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25326: Move Plasma's SortFilterProxyModel into KItemModel's QML plugin

[Arjen Hiemstra]

ahiemstra added inline comments.

INLINE COMMENTS

> ksortfilterproxymodel.cpp:74
> +QJSValueList args = {QJSValue(source_row), 
> engine->toScriptValue(source_parent)};
> +return const_cast *>(this)->m_filterCallback.call(args).toBool();
> +}

Suggestion: Instead of directly returning the value, do this: (Also for 
filterAcceptsColumn below)

  auto result = const_cast(this)->m_filterCallback.call(args);
  if (result.isError()) {
  qCWarning(KITEMMODELS_LOG) << "Row filter callback produced an error:";
  qCWarning(KITEMMODELS_LOG) << result.toString();
  } else {
  return result.toBool();
  }

That way at least some error will be displayed when the callback fails.

REPOSITORY
  R275 KItemModels

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

To: davidedmundson
Cc: kmaterka, iasensio, ahmadsamir, broulik, ahiemstra, mart, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25326: Move Plasma's SortFilterProxyModel into KItemModel's QML plugin

[Arjen Hiemstra]

ahiemstra added inline comments.

INLINE COMMENTS

> ksortfilterproxymodel.cpp:85
> +QJSValueList args = {QJSValue(source_column), 
> engine->toScriptValue(source_parent)};
> +return const_cast *>(this)->m_filterCallback.call(args).toBool();
> +}

This is using the wrong callback, it should be using m_filterColumnCallback

REPOSITORY
  R275 KItemModels

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

To: davidedmundson
Cc: kmaterka, iasensio, ahmadsamir, broulik, ahiemstra, mart, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25326: Move Plasma's SortFilterProxyModel into KItemModel's QML plugin

[Arjen Hiemstra]

ahiemstra added a comment.


  Something I noticed just now while trying to use this: the `filterString` 
property has disappeared. PlasmaCore.SortFilterProxy does have this. I think we 
should restore it, otherwise you need to manually call 
QSFPM::setFilterFixedString or hope that you can set it as a regular expression.

REPOSITORY
  R275 KItemModels

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

To: davidedmundson
Cc: kmaterka, iasensio, ahmadsamir, broulik, ahiemstra, mart, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25326: Move Plasma's SortFilterProxyModel into KItemModel's QML plugin

[Arjen Hiemstra]

ahiemstra added inline comments.

INLINE COMMENTS

> kmaterka wrote in ksortfilterproxymodel_qml.cpp:111
> Can you add a test for sortColumn? It might be useful for newcomers (like me) 
> to understand how it works. I had real trouble understanding when it is 
> needed and when not (ConfigEntries.qml 
> )

I don't think a unit test is the right place for example code. It's probably 
better to either improve the documentation of the property or add an example 
somewhere where it makes sense.

> kmaterka wrote in ksortfilterproxymodel.cpp:165
> When using PlasmaCore.SortFilterModel sortColumn is sometimes set to -1 and 
> sorting is not working when `sortColumn` is not set. If I remember correctly, 
> -1 is the default in QSortFilterProxyModel. Is `std::max(sortColumn(), 0)` 
> added to fix that? What will happen in this situation:
> 
>   KSortFilterProxyModel {
> sourceModel: testModel
> sortColumn: -1
> sortRole: "user"
>   }
> 
> Maybe is should be documented in `sortRole` and `sortOrder` properties that 
> these two set sortColumn to 0 (or -1 in case of empty role)?

In that example, you'd be sorting on the "user" role of column 0, which seems 
like a reasonable default to me. I would actually suggest to make sortColumn 
always at least 0, I don't think there really is much of a use case for -1 as 
default.

> kmaterka wrote in ksortfilterproxymodel.h:67
> can we have something similar for sorting? `sortColumnCallback` and use it in 
> overridden `lessThan`?

Let's keep this a bit constrained, if we add a stub implementation of 
`lessThan()`, we can later on add a property to expose a JS callback for it 
somehow.

REPOSITORY
  R275 KItemModels

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

To: davidedmundson
Cc: kmaterka, iasensio, ahmadsamir, broulik, ahiemstra, mart, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26431: Fix build of python bindings for kcolumnheadersmodel

[Arjen Hiemstra]

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


  Silly sip :/

REPOSITORY
  R275 KItemModels

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

To: arojas, dfaure, ahiemstra, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25311: Add KColumnHeadersProxyModel

[Arjen Hiemstra]

This revision was automatically updated to reflect the committed changes.
Closed by commit R275:241e9dec6896: Add KColumnHeadersProxyModel (authored by 
ahiemstra).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D25311?vs=70948=71204#toc

REPOSITORY
  R275 KItemModels

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25311?vs=70948=71204

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kcolumnheadersmodeltest.cpp
  src/core/CMakeLists.txt
  src/core/kcolumnheadersmodel.cpp
  src/core/kcolumnheadersmodel.h
  src/qml/plugin.cpp
  tests/qml/columnheaders.qml

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

D25311: Add KColumnHeadersProxyModel

[Arjen Hiemstra]

ahiemstra added a comment.


  Sure, I can land it after saturday.

REPOSITORY
  R275 KItemModels

BRANCH
  columnheadersmodel

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

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

D25311: Add KColumnHeadersProxyModel

[Arjen Hiemstra]

ahiemstra marked an inline comment as done.

REPOSITORY
  R275 KItemModels

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

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

D25311: Add KColumnHeadersProxyModel

[Arjen Hiemstra]

ahiemstra updated this revision to Diff 70948.
ahiemstra added a comment.


  - Add missing layoutChanged/reset signals

REPOSITORY
  R275 KItemModels

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25311?vs=70437=70948

BRANCH
  columnheadersmodel

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kcolumnheadersmodeltest.cpp
  src/core/CMakeLists.txt
  src/core/kcolumnheadersmodel.cpp
  src/core/kcolumnheadersmodel.h
  src/qml/plugin.cpp
  tests/qml/columnheaders.qml

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

D25683: KDirOperator: Use a fixed line height for scroll speed

[Arjen Hiemstra]

ahiemstra edited the summary of this revision.

REPOSITORY
  R241 KIO

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

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

D25683: KDirOperator: Use a fixed line height for scroll speed

[Arjen Hiemstra]

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

REVISION SUMMARY
  This patch intends to do mostly the same as D19190 
, but for KDirOperator.
  This uses the font's line height multiplied by the amount of scroll lines
  from QApplication as a scroll speed for views. This makes the scroll speed
  not depend on the icon size, which greatly improves scrolling behaviour.

TEST PLAN
  Run tests/kfilewidgettest_gui and go to a directory with a lot of items.
  Scroll around, then change icon size and see that scrolling speed remains
  the same.

REPOSITORY
  R241 KIO

BRANCH
  diroperator_scrollspeed

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

AFFECTED FILES
  src/filewidgets/kdiroperatordetailview.cpp
  src/filewidgets/kdiroperatoriconview.cpp

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

D25311: Add KColumnHeadersProxyModel

[Arjen Hiemstra]

ahiemstra marked an inline comment as done.
ahiemstra added a comment.


  @vkrause Is the updated version more what you expected?

REPOSITORY
  R275 KItemModels

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

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

D25311: Add KColumnHeadersProxyModel

[Arjen Hiemstra]

ahiemstra marked an inline comment as done.
ahiemstra added inline comments.

INLINE COMMENTS

> kossebau wrote in kcolumnheadersmodel.h:57
> For consistency I would prefer having a full Q_SIGNALS section:
> 
>   Q_SIGNALS:
>   void sourceModelChanged();
> 
> That matches the usual pattern and helps when reading raw headers to learn 
> about API of classes.

I actually prefer the combined style since it groups together all related bits 
regarding a single property. But I've changed it to a separate section now.

> kossebau wrote in kcolumnheadersmodel.h:60
> Why not QScopedPointer (for consistency in KF code)? Is that one going to be 
> deprecated in favour of std::unique_ptr?

Mostly force of habit. I also see little point in using QScopedPointer 
nowadays, as it does not offer anything over std::unique_ptr.

REPOSITORY
  R275 KItemModels

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

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

D25311: Add KColumnHeadersProxyModel

[Arjen Hiemstra]

ahiemstra updated this revision to Diff 70437.
ahiemstra added a comment.


  - Use separate Q_SIGNALS for consistency

REPOSITORY
  R275 KItemModels

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25311?vs=70405=70437

BRANCH
  columnheadersmodel

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kcolumnheadersmodeltest.cpp
  src/core/CMakeLists.txt
  src/core/kcolumnheadersmodel.cpp
  src/core/kcolumnheadersmodel.h
  src/qml/plugin.cpp
  tests/qml/columnheaders.qml

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

D25311: Add KColumnHeadersProxyModel

[Arjen Hiemstra]

ahiemstra updated this revision to Diff 70405.
ahiemstra added a comment.


  - Do not use nested Private class

REPOSITORY
  R275 KItemModels

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25311?vs=70359=70405

BRANCH
  columnheadersmodel

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kcolumnheadersmodeltest.cpp
  src/core/CMakeLists.txt
  src/core/kcolumnheadersmodel.cpp
  src/core/kcolumnheadersmodel.h
  src/qml/plugin.cpp
  tests/qml/columnheaders.qml

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

D25311: Add KColumnHeadersProxyModel

[Arjen Hiemstra]

ahiemstra updated this revision to Diff 70359.
ahiemstra added a comment.


  - Change to using QAbstractListModel as base

REPOSITORY
  R275 KItemModels

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25311?vs=69807=70359

BRANCH
  columnheadersmodel

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kcolumnheadersmodeltest.cpp
  src/core/CMakeLists.txt
  src/core/kcolumnheadersmodel.cpp
  src/core/kcolumnheadersmodel.h
  src/qml/plugin.cpp
  tests/qml/columnheaders.qml

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

D25311: Add KColumnHeadersProxyModel

[Arjen Hiemstra]

ahiemstra marked an inline comment as done.
ahiemstra added a comment.


  > We should probably have a unit test, even if it just covers the basic case.
  
  Done.
  
  > This currently assumes the source model's columns are static. At a minimum 
that needs a comment.
  
  Oops. That was not the intention. Fixed.
  
  > I'm wondering if this is technically a proxy model, or rather a 
QAbstractListModel? Its content is not representing cells in the source model 
after all, which mapTo/FromSource assumes I think.
  
  Yeah I'm not entirely sure. If it was possible I would probably have based 
this on QTransposeProxyModel, but that is Qt 5.13+ stuff.
  
  > It works in the example as you implement the entire QAIM interface needed 
for it, so the mapping functions are probably not even called, until the source 
model changes, or you add a selection model on top.
  
  Most of what I needed I needed to implement because QAbstractProxyModel does 
not reimplement them.

REPOSITORY
  R275 KItemModels

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

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

D25311: Add KColumnHeadersProxyModel

[Arjen Hiemstra]

ahiemstra updated this revision to Diff 69807.
ahiemstra added a comment.


  - Add a unit test for KColumnHeadersProxyModel

REPOSITORY
  R275 KItemModels

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25311?vs=69806=69807

BRANCH
  columnheadersmodel

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kcolumnheadersproxymodeltest.cpp
  src/core/CMakeLists.txt
  src/core/kcolumnheadersproxymodel.cpp
  src/core/kcolumnheadersproxymodel.h
  src/qml/plugin.cpp
  tests/qml/columnheaders.qml

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

D25311: Add KColumnHeadersProxyModel

[Arjen Hiemstra]

ahiemstra updated this revision to Diff 69806.
ahiemstra added a comment.


  - Forward column change singals as row change signals
  - Forward header data change signals as dataChanged
  - Add a unit test for KColumnHeadersProxyModel

REPOSITORY
  R275 KItemModels

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25311?vs=69765=69806

BRANCH
  columnheadersmodel

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kcolumnheadersproxymodeltest.cpp
  src/core/CMakeLists.txt
  src/core/kcolumnheadersproxymodel.cpp
  src/core/kcolumnheadersproxymodel.h
  src/qml/plugin.cpp
  tests/qml/columnheaders.qml

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

D25326: Move Plasma's SortFilterProxyModel into KItemModel's QML plugin

[Arjen Hiemstra]

ahiemstra added inline comments.

INLINE COMMENTS

> sortfiltermodel.h:63
> + */
> +Q_PROPERTY(QString filterRole READ filterRole WRITE setFilterRole)
> +

This (and sortRole below) hides the properties from QSortFilterProxy with the 
same name. Maybe it is a better idea to use filterRoleName/sortRoleName?

> sortfiltermodel.h:74
> +Q_PROPERTY(Qt::SortOrder sortOrder READ sortOrder WRITE setSortOrder)
> +
> +/**

Having a sortColumn property would also be useful, since that is apparently 
missing from QSortFilterProxy. I seem to recall that was added to the Plasma 
version a while ago.

REPOSITORY
  R275 KItemModels

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

To: davidedmundson
Cc: ahiemstra, mart, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D25311: Add KColumnHeadersProxyModel

[Arjen Hiemstra]

ahiemstra updated this revision to Diff 69765.
ahiemstra added a comment.


  - Add @since for the class

REPOSITORY
  R275 KItemModels

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25311?vs=69764=69765

BRANCH
  columnheadersmodel

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/kcolumnheadersproxymodel.cpp
  src/core/kcolumnheadersproxymodel.h
  src/qml/plugin.cpp
  tests/qml/columnheaders.qml

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

D25311: Add KColumnHeadersProxyModel

[Arjen Hiemstra]

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

REVISION SUMMARY
  It converts a model's column headers into a simple list model. It is
  mainly useful as a helper to create headers for QtQuick TableView but
  may also be useful for other things.

TEST PLAN
  The added test QML file shows a single 1 as described in the test.

REPOSITORY
  R275 KItemModels

BRANCH
  columnheadersmodel

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/kcolumnheadersproxymodel.cpp
  src/core/kcolumnheadersproxymodel.h
  src/qml/plugin.cpp
  tests/qml/columnheaders.qml

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

D25303: Expose KDescendantsProxyModel to QML

[Arjen Hiemstra]

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

REPOSITORY
  R275 KItemModels

BRANCH
  master

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

To: davidedmundson, ahiemstra
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

Re: Quick Charts in KDE Review

[Arjen Hiemstra]

On 07-11-2019 15:34, Friedrich W. H. Kossebau wrote:

Am Montag, 21. Oktober 2019, 15:22:23 CET schrieb Arjen Hiemstra:

Hi,

Quick Charts has been moved to KDE review. The intent is to make it a
Tier 1 framework.


Any chance the official name can be "KQuickCharts"? "Quick Charts" is a
generic name which only asks for being misunderstood, is hard to google 
etc..


Fair point. Since the repo is already kquickcharts, I will update the 
documentation

to match.



Quick Charts is a QML module that implements a set of 
high-performance,

GPU accelerated
charts. Currently the main user of it is a new KSysGuard UI I have 
been

working on, but
once it is part of Frameworks I also hope to convert several bits of
Plasma to using it.


If there is only one user currently, might it perhaps be a better idea 
to do
some independent releases for a while to get more feedback on the API, 
before
settling to the API freeze by being part of KDE Frameworks? It will be 
at

least a year until KF6 is there to properly fix up any potential API
inconveniences which users might find.
I would at least recommend to first get some API shaping by real-world
exposure.


So there is one known user currently, KSysGuard. Additionally, there are
several places in Plasma that I want to look at to use them. There is 
also
the KInfoCenter energy information page where we want to look at using 
this.
Additionally, there has been some interest from others. I do not know 
what

Plasma's policy is regarding dependencies, but having it in Frameworks
would make things quite a bit simpler to port existing things.

The module has been in development for about 5 months at this point. We 
have
been using it for the mentioned new KSysGuard UI as well as some other 
bits

we want to upstream. At this point, I am fairly sure that any major API
changes would amount to a "KF6" version anyway. I would rather commit to 
the

stable API so that people will know there is no large chance of things
suddenly changing from underneath them.

 - Arjen



Sorry otherwise for not reviewing myself, not into QML the recent 
months.


Cheers
Friedrich

Re: Quick Charts in KDE Review

[Arjen Hiemstra]

On 07-11-2019 15:24, Alexander Potashev wrote:

чт, 7 нояб. 2019 г. в 13:53, Arjen Hiemstra :


On 21-10-2019 15:22, Arjen Hiemstra wrote:
> Hi,
>
> Quick Charts has been moved to KDE review. The intent is to make it a
> Tier 1 framework.
>
> Quick Charts is a QML module that implements a set of
> high-performance, GPU accelerated
> charts. Currently the main user of it is a new KSysGuard UI I have
> been working on, but
> once it is part of Frameworks I also hope to convert several bits of
> Plasma to using it.

It has now been a little over two weeks. If there are no objections, I
will move this to frameworks later today.


Hi Arjen,

Did anyone from the Plasma team approve the new KSysGuard UI?


It is being developed together with several people from the Plasma team
actually. :)

It will also go through a separate review, but it needs Quick Charts
to be ready before that can happen.

 - Arjen

Re: Quick Charts in KDE Review

[Arjen Hiemstra]

On 21-10-2019 15:22, Arjen Hiemstra wrote:

Hi,

Quick Charts has been moved to KDE review. The intent is to make it a
Tier 1 framework.

Quick Charts is a QML module that implements a set of
high-performance, GPU accelerated
charts. Currently the main user of it is a new KSysGuard UI I have
been working on, but
once it is part of Frameworks I also hope to convert several bits of
Plasma to using it.


It has now been a little over two weeks. If there are no objections, I
will move this to frameworks later today.



- Arjen

D25067: Fix the header layouts for EntryDetails and Page components

[Arjen Hiemstra]

ahiemstra added inline comments.

INLINE COMMENTS

> EntryDetails.qml:94
>  title: i18nc("Combined title for the entry details page made of the name 
> of the entry, and the author's name", "%1 by 
> %2").arg(component.name).arg(entryAuthor.name)
>  titleDelegate: QtLayouts.RowLayout {
> +QtLayouts.Layout.fillWidth: true

Hmm, with the new toolbar code, there's actually little reason to use a custom 
delegate here, since the toolbar header already uses a title + tool buttons 
style. You could convert the three toolbuttons to actions and drop the rest of 
the delegate.

REPOSITORY
  R304 KNewStuff

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

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

D25067: Fix the header layouts for EntryDetails and Page components

[Arjen Hiemstra]

ahiemstra added inline comments.

INLINE COMMENTS

> ngraham wrote in EntryDetails.qml:95
> I don't think this works, since it's not inside another layout. Instead maybe 
> `anchors.fill: parent` would work better?

It does not. I am also unsure whether `anchors.fill` will work, since the 
titleDelegate is instantiated by a loader and its sizing behaviour is somewhat 
fuzzy. The only option that is guaranteed to work is setting a proper 
implicitWidth for the delegate. I actually spent some time trying to get 
balanced auto sizing behaviour working for titles when doing the ActionToolBar 
work, but in the end gave up because things get really tricky when you have two 
items that resize based on the size of the other item.

REPOSITORY
  R304 KNewStuff

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

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

Re: Quick Charts in KDE Review

[Arjen Hiemstra]

On 22-10-2019 18:09, Luigi Toscano wrote:

Jonathan Riddell ha scritto:

There's a file called LICENSE which contains a copy of the LGPL 2.1

But all the files with an explicit licence seem to be GPL 2+3+KDEeV

Please decide which licence you want to apply and if it's GPL 
2+3+KDEeV remove
LICENSE and put a copy of the GPL 2 called COPYING which is our normal 
practice.




Also, if the goal is to make it part of Frameworks, it can't be GPL 
(but LGPL

or less restrictive licenses).

https://community.kde.org/Policies/Licensing_Policy


Yeah, I knew that. It should've been LGPL 2.1+ all along, but apparently 
I copied
the headers wrong. :) Now fixed, everything should be LGPL 2.1+ with EV 
exception.

I have also renamed the license file to COPYING.

Re: Quick Charts in KDE Review

[Arjen Hiemstra]

On 21-10-2019 16:45, Alexander Potashev wrote:

Hi Arjen,

1. I don't see any checks in the autotests. Would it be easy to
compare the displayed chart against a reference screenshot image, in
each of the tests?

The same screenshots could also help developers understand what kind
of charts kquickcharts is good for.


They are very simple QML based tests that currently only verify that the
main Chart types still get created correctly. It'd be nice to expand 
them,
though I am slightly hesitant to go with image based tests as they tend 
to

break easily. On the other hand, one of the main reasons for not having
more extensive tests is that I did not have an idea on how to do that, 
and

image based tests would solve part of that problem.



2. How to add a grid, ticks and labels to a line chart?



For the grid and labels, these are what I generally refer to as 
"decorations".
These are implemented as separate items that you use the normal QML 
layout
options with to layout. You can have a look at 
controls/LineChartControl.qml
to see how it is done for a Line chart. I still need to write some 
better

examples for this, but the control should give you a decent idea.

For ticks, assuming with those you mean some marker on the chart data 
points,
these are not currently implemented. I do have some ideas regarding 
these but

I did not need them yet.

- Arjen


пн, 21 окт. 2019 г. в 16:46, Arjen Hiemstra :


On 21-10-2019 15:22, Arjen Hiemstra wrote:
> Hi,
>
> Quick Charts has been moved to KDE review. The intent is to make it a
> Tier 1 framework.
>
> Quick Charts is a QML module that implements a set of
> high-performance, GPU accelerated
> charts. Currently the main user of it is a new KSysGuard UI I have
> been working on, but
> once it is part of Frameworks I also hope to convert several bits of
> Plasma to using it.

And of course, a link would be useful:

https://invent.kde.org/kde/kquickcharts

>
> - Arjen

Re: Quick Charts in KDE Review

[Arjen Hiemstra]

On 21-10-2019 15:22, Arjen Hiemstra wrote:

Hi,

Quick Charts has been moved to KDE review. The intent is to make it a
Tier 1 framework.

Quick Charts is a QML module that implements a set of
high-performance, GPU accelerated
charts. Currently the main user of it is a new KSysGuard UI I have
been working on, but
once it is part of Frameworks I also hope to convert several bits of
Plasma to using it.


And of course, a link would be useful:

https://invent.kde.org/kde/kquickcharts



- Arjen

Quick Charts in KDE Review

[Arjen Hiemstra]

Hi,

Quick Charts has been moved to KDE review. The intent is to make it a 
Tier 1 framework.


Quick Charts is a QML module that implements a set of high-performance, 
GPU accelerated
charts. Currently the main user of it is a new KSysGuard UI I have been 
working on, but
once it is part of Frameworks I also hope to convert several bits of 
Plasma to using it.


- Arjen

D24630: Replace all qWarning and related calls with categorised logging

[Arjen Hiemstra]

This revision was automatically updated to reflect the committed changes.
Closed by commit R249:b45c3cea521e: Replace all qWarning and related calls with 
categorised logging (authored by ahiemstra).

REPOSITORY
  R249 KI18n

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24630?vs=67893=67962

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

AFFECTED FILES
  CMakeLists.txt
  ki18n.categories
  src/CMakeLists.txt
  src/kcatalog.cpp
  src/klocalizedcontext.cpp
  src/klocalizedstring.cpp
  src/kuitmarkup.cpp

To: ahiemstra, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24630: Replace all qWarning and related calls with categorised logging

[Arjen Hiemstra]

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

REVISION SUMMARY
  This replaces all uncategorised qWarning() calls with calls to qCWarning(), 
using the 
  kf5.ki18n category for the main warnings, with kf5.ki18n.kuit used for 
warnings 
  produced by the kuit code.

TEST PLAN
  Tests still pass, warnings printed by ki18n during tests now use categorised 
logging.

REPOSITORY
  R249 KI18n

BRANCH
  categorised_logging

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

AFFECTED FILES
  CMakeLists.txt
  ki18n.categories
  src/CMakeLists.txt
  src/kcatalog.cpp
  src/klocalizedcontext.cpp
  src/klocalizedstring.cpp
  src/kuitmarkup.cpp

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

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

[Arjen Hiemstra]

ahiemstra added inline comments.

INLINE COMMENTS

> leinir wrote in Button.qml:112
> Humhum, yes... so it needs to be possible to change enabled from outside, but 
> it also needs to be locked to disabled if allowedByKiosk is false... i'm 
> thinking this probably won't be the prettiest thing, but... catch enabled 
> changing, check whether allowedByKiosk is false and force it to false in the 
> case it is, and otherwise ignore it... unless there's a magic trick i'm not 
> aware of, which would be neat ;)

With a Binding object I can make sure that a button is kept disabled even when 
the main enabled is bound to a different value. However, I can not prevent 
javascript "button.enabled = true" from enabling the button, so forcing things 
in an enabledChanged handler seems the better solution.

REPOSITORY
  R304 KNewStuff

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

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

Re: Proposing Quick Charts as a new framework

[Arjen Hiemstra]

On 18-09-2019 12:06, Arjen Hiemstra wrote:

On 06-09-2019 11:02, Semke, Alexander wrote:

Am 06.09.2019 um 02:49 schrieb Aleix Pol :

Does it really have the same features? Otherwise it doesn't make 
sense

to deprecate anything. It will just frustrate the users of the
framework.

Qt has https://doc.qt.io/qt-5/qtcharts-index.html. Does kqtquickcharts
or this new framework provide more features?



Personally, I consider the QtQuick side of QtCharts to be barely 
usable,

since it just wraps the QtWidgets bits and embeds them in your QtQuick
scene. This leads to all sorts of weird issues.



(Looks like part of my message got lost, here's the rest:)

Again, the most major difference is that Quick Charts is completely 
built

to use GPU accelerated rendering. Currently it does not have the same
features as QtCharts but there is no reason the feature set cannot be
expanded.

- Arjen





—
Alexander

Re: Proposing Quick Charts as a new framework

[Arjen Hiemstra]

On 06-09-2019 11:02, Semke, Alexander wrote:

Am 06.09.2019 um 02:49 schrieb Aleix Pol :

Does it really have the same features? Otherwise it doesn't make sense
to deprecate anything. It will just frustrate the users of the
framework.

Qt has https://doc.qt.io/qt-5/qtcharts-index.html. Does kqtquickcharts
or this new framework provide more features?



Personally, I consider the QtQuick side of QtCharts to be barely usable,
since it just wraps the QtWidgets bits and embeds them in your QtQuick
scene. This leads to all sorts of weird issues.

Again, the most major difference is that Quick Charts is completely 
built

on top o



—
Alexander

Re: Proposing Quick Charts as a new framework

[Arjen Hiemstra]

On 09-09-2019 10:24, Dominik Haumann wrote:

Hi,

On Sat, Sep 7, 2019 at 3:59 PM Arjen Hiemstra 
wrote:


On 06-09-2019 02:49, Aleix Pol wrote:

On Thu, Sep 5, 2019 at 10:53 PM Arjen Hiemstra



wrote:


On 02-09-2019 19:26, Luigi Toscano wrote:

Arjen Hiemstra ha scritto:


[...]


That's actually a good point, though the kf5 is only in the

repository

name, I
name it "Quick Charts" everywhere else. Which is probably a good
reason
to have
the repo name changed in the first place. :) Originally, I put

kf5 in

the repo
name because that's what is used once installed as part of

Frameworks,

but I
agree that it can lead to confusing things.


It should be called kquickchart (which indeed is far too similar

to

kqtquickcharts). Much like you use KF5CoreAddons, but the

repository

is called kcoreaddons.


To be fair, KCoreAddons is called KCoreAddons in its documentation,
so
it
does not seem to be called "CoreAddons". At the same time, there are

plenty
of Frameworks that do not start with a K, like Solid, Prison or
BluezQt.
So I do not really see why it should be "kquickcharts".


Just my 2 cents:

The reason is consistency. Don't underestimate consistency. We have
done naming mistakes from time to time, but that should not imply
we should do it again.

+1 for KQuickCharts, names do matter.


At the same time, I could argue that we are trying to move away from k-
names in applications and might want to do the same for other parts of
KDE. But let's not argue semantics, if people insist on kquickcharts I
will change it to kquickcharts.



Best regards
Dominik

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

[Arjen Hiemstra]

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


  I went over it again and found a few more small things and also added some 
suggestions. Feel free to apply or ignore the suggestions. Once the other items 
have been taken care of, I think this is good to go.

INLINE COMMENTS

> commentsmodel.cpp:99
> +int pageToLoad = comments.count() / commentsPerPage;
> +qDebug() << "Loading comments, page" << pageToLoad << "with 
> current comment count" << comments.count() << "out of a total of" << 
> entry.numberOfComments();
> +provider->loadComments(entry, commentsPerPage, pageToLoad);

Categorise or drop :)

> engine.h:155
> + * The sort mode set on the current request
> + * @see setFilter(Provider::SortMode)
> + * @since 5.62

I assume you mean setSortMode here?

> author.cpp:37
> +typedef QHash> AllAuthorsHash;
> +Q_GLOBAL_STATIC(AllAuthorsHash, allAuthors)
> +

Suggestion: It might make sense to do this caching at the provider level so 
users don't need to reimplement this.

> categoriesmodel.cpp:49
> +{
> +QHash roles;
> +roles[NameRole] = "name";

Suggestion: I tend to make roles static since it doesn't change between 
instances anyway.
Like so:

  static QHash roles{
  { NameRole, "name" },
  ...
  };

> categoriesmodel.h:61
> +class Private;
> +Private* d;
> +};

Suggestion: I use `const std::unique_ptr d;` these days, it removes 
the need to explicitly delete the d pointer.

> commentsmodel.h:38
> + */
> +class CommentsModel : public QIdentityProxyModel
> +{

Like Author, it might be a good idea to use QQmlParserStatus here.

> Button.qml:112
> +visible: enabled || visibleWhenDisabled
> +enabled: ghnsDialog.engine.allowedByKiosk
> +

Hmm, this is quite tricky, as a user of the Button I can now no longer safely 
bind enabled as that would override the allowedByKiosk binding. You should 
probably make this an explicit binding so it doesn't break as easily.

REPOSITORY
  R304 KNewStuff

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

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

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

Re: Proposing Quick Charts as a new framework

[Arjen Hiemstra]

On 06-09-2019 02:49, Aleix Pol wrote:
On Thu, Sep 5, 2019 at 10:53 PM Arjen Hiemstra  
wrote:


On 02-09-2019 19:26, Luigi Toscano wrote:
> Arjen Hiemstra ha scritto:
>> Hi,
>>
>> I have been working on a library the past few months that provides a
>> QtQuick
>> module for rendering gpu-accelerated charts. It currently lives in a
>> playground
>> repository, here: https://invent.kde.org/kde/kf5quickcharts . I would
>> like for
>> this library to be included as a Tier 1 Framework.
>
> Hi,
> I've noticed that some time ago (I didn't enable the translations yet,
> but
> maybe it's not needed).

There are only user-visible strings in the example application. I
haven't marked them
to be translated since it is only an example.

> I have a question and a comment:
>
> - what is the difference between this one and the existing
> kqtquickcharts,
> part of the KDE Applications bundle, from the KDEEdu project? At least
> their
> description overlaps. Is there any transition path? Having two
> duplicate
> libraries with the exact same goal may not be the best scenario.

The main difference is in the rendering methods: kqtquickcharts uses
QPainter for
rendering, Quick Charts uses a technique called Signed Distance Fields
to render
certain charts with GPU acceleration and others use pure QtQuick
scenegraph. I
don't know if it would have been possible to retrofit that on to
kqtquickcharts,
but my guess is it would have been a major overhaul of that library
anyway.

An additional issue is that kqtquickcharts seems to be unmaintained,
with the last
major feature work having been done several years ago. So my personal
opinion is
that when Quick Charts is accepted as part of Frameworks, we deprecate
kqtquickcharts as well as some other bits like the plotter item from
kdeclarative.


Does it really have the same features? Otherwise it doesn't make sense
to deprecate anything. It will just frustrate the users of the
framework.


There is a significant amount of overlap. The kqtquickcharts library has 
a
few features that Quick Charts does not have, most but at the same time 
Quick

Charts has features kqtquickcharts does not have. Some of the things in
kqtquickcharts I had already planned to include, a few others would be 
nice

to include as well. I agree, however, that we should not just blindly
deprecate it.




> - I don't think that "5" should be part of the name; in two years from
> now we
> may have a Frameworks 6, and having a kf5quickchart as part of it may
> be
> confusing.

That's actually a good point, though the kf5 is only in the repository
name, I
name it "Quick Charts" everywhere else. Which is probably a good 
reason

to have
the repo name changed in the first place. :) Originally, I put kf5 in
the repo
name because that's what is used once installed as part of Frameworks,
but I
agree that it can lead to confusing things.


It should be called kquickchart (which indeed is far too similar to
kqtquickcharts). Much like you use KF5CoreAddons, but the repository
is called kcoreaddons.


To be fair, KCoreAddons is called KCoreAddons in its documentation, so 
it
does not seem to be called "CoreAddons". At the same time, there are 
plenty

of Frameworks that do not start with a K, like Solid, Prison or BluezQt.
So I do not really see why it should be "kquickcharts".

- Arjen



Aleix

Re: Proposing Quick Charts as a new framework

[Arjen Hiemstra]

On 02-09-2019 19:26, Luigi Toscano wrote:

Arjen Hiemstra ha scritto:

Hi,

I have been working on a library the past few months that provides a 
QtQuick
module for rendering gpu-accelerated charts. It currently lives in a 
playground
repository, here: https://invent.kde.org/kde/kf5quickcharts . I would 
like for

this library to be included as a Tier 1 Framework.


Hi,
I've noticed that some time ago (I didn't enable the translations yet, 
but

maybe it's not needed).


There are only user-visible strings in the example application. I 
haven't marked them

to be translated since it is only an example.


I have a question and a comment:

- what is the difference between this one and the existing 
kqtquickcharts,
part of the KDE Applications bundle, from the KDEEdu project? At least 
their
description overlaps. Is there any transition path? Having two 
duplicate

libraries with the exact same goal may not be the best scenario.


The main difference is in the rendering methods: kqtquickcharts uses 
QPainter for
rendering, Quick Charts uses a technique called Signed Distance Fields 
to render
certain charts with GPU acceleration and others use pure QtQuick 
scenegraph. I
don't know if it would have been possible to retrofit that on to 
kqtquickcharts,
but my guess is it would have been a major overhaul of that library 
anyway.


An additional issue is that kqtquickcharts seems to be unmaintained, 
with the last
major feature work having been done several years ago. So my personal 
opinion is

that when Quick Charts is accepted as part of Frameworks, we deprecate
kqtquickcharts as well as some other bits like the plotter item from 
kdeclarative.


- I don't think that "5" should be part of the name; in two years from 
now we
may have a Frameworks 6, and having a kf5quickchart as part of it may 
be

confusing.


That's actually a good point, though the kf5 is only in the repository 
name, I
name it "Quick Charts" everywhere else. Which is probably a good reason 
to have
the repo name changed in the first place. :) Originally, I put kf5 in 
the repo
name because that's what is used once installed as part of Frameworks, 
but I

agree that it can lead to confusing things.

Cheers,
Arjen



Ciao

Proposing Quick Charts as a new framework

[Arjen Hiemstra]

Hi,

I have been working on a library the past few months that provides a 
QtQuick
module for rendering gpu-accelerated charts. It currently lives in a 
playground
repository, here: https://invent.kde.org/kde/kf5quickcharts . I would 
like for

this library to be included as a Tier 1 Framework.

Current features are that it supports rendering of three different chart 
types:
pie, line and bar, which can be fed from multiple types of data sources. 
There
is also support for axis labels, an axis grid and a legend. 
Additionally, there

is a submodule that contains some convenience items.

The pie and line charts are rendered using a technique called signed 
distance
fields, which allows efficient GPU-accelerated rendering of 2D shapes 
without

loss of quality.

I would like to hear what people think about it and if this is something 
others

would also like to have included in Frameworks.

- Arjen

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

[Arjen Hiemstra]

ahiemstra added inline comments.

INLINE COMMENTS

> leinir wrote in atticaprovider.cpp:355
> 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 :)

Fair enough. I do try to make sure to use vectors as much as possible in new 
API, but consistency is also a good argument. :)

> leinir wrote in Dialog.qml:76
> 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)

It would match with the FileDialog API 
(https://doc.qt.io/qt-5/qml-qtquick-dialogs-filedialog.html#fileUrls-prop) 
however, which also has this behaviour. My main problem with signal parameters 
is that you cannot bind to them, so using the result gets trickier.

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

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

D6513: Add support for Attica tags support

[Arjen Hiemstra]

ahiemstra accepted this revision.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra, mlaurent
Cc: mlaurent, ngraham, ahiemstra, kde-frameworks-devel, #knewstuff, michaelh, 
ZrenBot, bruns

D6513: Add support for Attica tags support

[Arjen Hiemstra]

ahiemstra requested changes to this revision.
ahiemstra added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> atticaprovider.cpp:283
> +}
> +if(filterAcceptsDownloads && checker.filterAccepts(content.tags())) {
> +mCachedContent.insert(content.id(), content);

Wouldn't it be simpler to first check if the content should be considered at 
all, and then check if one of the downloads is valid? So something like:

  if(!checker.filterAccepts(content.tags()) {
continue
  }
  
  foreach download {
   if(downloadsChecker.filterAccepts(download.tags) {
 add download
 break
   }
  }

> engine.cpp:131
>  
> +d->tagFilter = group.readEntry("TagFilter").split(QStringLiteral(","));
> +if(d->tagFilter.length() == 1 && d->tagFilter.at(0).isEmpty()) {

Considering KConfig has methods to read and write lists, wouldn't it be better 
to use those?

> engine.h:190
> + * out entries marked as ghns_exclude=1. To retain this when setting a 
> custom
> + * filter, add "ghns_exclude!=1" as one of the filters.
> + *

Would it make sense to add an `addTagFilter` method or similar for convenience 
that appends instead of replaces? Or maybe make the setter a bit more 
intelligent so it is harder to forget adding the `ghns_exclude!=1` filter? 
Right now, if I need to do anything with the tag filters, I will constantly 
need to remember to add the ghns_exclude filter.

Maybe even completely separate the ghns_exclude filter from this setter and 
make a separate "also show excluded content" switch that removes the filter?

> engine.h:227
> + */
> +void setTagFilter(QStringList filter);
> +/**

`const QStringList&`

(Also applies to setDownloadTagFilter below)

> entryinternal.cpp:165
> +
> +void KNSCore::EntryInternal::setTags(const QStringList& tags)
> +{

Nitpick, but this doesn't match with the coding style of the declaration 
(`const QStringList& tags`  vs `const QStringList `)

> tagsfilterchecker.cpp:43
> +public:
> +Validator(const QString& tag, const QString value) {
> +m_tag = tag;

I think you mean `const QString& value` ?

> tagsfilterchecker.cpp:98
> +QString value = filter.mid(tag.length() + 2);
> +Validator* val = validators.value(tag, nullptr);
> +if(val)

Wouldn't this be simpler?

  if(!validators.contains(tag)) {
validators[tag] = new EqualityValidator(tag, "");
  }
  validators.value(tag)->m_acceptedValues << value;

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, apol, #kde_store, whiting, ahiemstra
Cc: ahiemstra, kde-frameworks-devel, #knewstuff, michaelh, ZrenBot, ngraham, 
bruns

D6512: Add support for proposed tags addition in OCS 1.7

[Arjen Hiemstra]

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

REPOSITORY
  R235 Attica

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

To: leinir, #knewstuff, apol, whiting, #kde_store, ahiemstra
Cc: ahiemstra, ngraham, kde-frameworks-devel, #kde_store, michaelh, ZrenBot, 
bruns, akiraohgaki, alexanderschmidt, siyuandong, ronaldv, mikesomov, starbuck, 
sebas

D6512: Add support for proposed tags addition in OCS 1.7

[Arjen Hiemstra]

ahiemstra added a comment.


  Right, so leave it to application users how to deal with that, maybe later on 
add some convenience functions. Sounds sensible. :)

REPOSITORY
  R235 Attica

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

To: leinir, #knewstuff, apol, whiting, #kde_store
Cc: ahiemstra, ngraham, kde-frameworks-devel, #kde_store, michaelh, ZrenBot, 
bruns, akiraohgaki, alexanderschmidt, siyuandong, ronaldv, mikesomov, starbuck, 
sebas

D6512: Add support for proposed tags addition in OCS 1.7

[Arjen Hiemstra]

ahiemstra added a comment.


  T6133  suggests that tags are formatted as 
"group##key=value" or something similar. Wouldn't it make sense to handle 
parsing that format here as well? Or are tags more intended as exact matches?

REPOSITORY
  R235 Attica

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

To: leinir, #knewstuff, apol, whiting, #kde_store
Cc: ahiemstra, ngraham, kde-frameworks-devel, #kde_store, michaelh, ZrenBot, 
bruns, akiraohgaki, alexanderschmidt, siyuandong, ronaldv, mikesomov, starbuck, 
sebas