D26604: Check if there is an activatable service when notification service owner changes

[Kai Uwe Broulik]

broulik added a comment.


  What again was the reason for not just sending a notification where DBus 
activation will do its thing?

REPOSITORY
  R289 KNotifications

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

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

D26099: Port QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir updated this revision to Diff 73367.
ahmadsamir added a comment.


  - Rebase\n- Move second match inside if condition, otherwise the control flow 
never passes through the next else if branch

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26099?vs=72040=73367

BRANCH
  l-qregexp (branched from master)

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

AFFECTED FILES
  src/kioslaves/timeline/timelinetools.cpp

To: ahmadsamir, #baloo, meven, bruns, astippich, mlaurent, apol, dfaure
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, fbampaloukas, GB_2, 
domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D26614: Remove extra flush when we close file it will flush

[Laurent Montel]

mlaurent added a reviewer: dfaure.

REPOSITORY
  R269 BluezQt

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

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

D26614: Remove extra flush when we close file it will flush

[Laurent Montel]

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

REVISION SUMMARY
  Remove extra flush

REPOSITORY
  R269 BluezQt

BRANCH
  remove_extra_flush (branched from master)

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

AFFECTED FILES
  tools/bluezapi2qt/CppGenerator.cpp
  tools/bluezapi2qt/XmlGenerator.cpp

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

D26484: Add KIO::DropJobFlag to allow manually showing the menu

[Tranter Madi]

trmdi updated this revision to Diff 73364.
trmdi added a comment.


  - Typo

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26484?vs=73363=73364

BRANCH
  master

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

AFFECTED FILES
  src/widgets/dropjob.cpp
  src/widgets/dropjob.h

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

D26484: Add KIO::DropJobFlag to allow manually showing the menu

[Tranter Madi]

trmdi marked 3 inline comments as done.

REPOSITORY
  R241 KIO

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

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

D26484: Add KIO::DropJobFlag to allow manually showing the menu

[Tranter Madi]

trmdi updated this revision to Diff 73363.
trmdi added a comment.


  - Improve comment/doc

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26484?vs=73337=73363

BRANCH
  master

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

AFFECTED FILES
  src/widgets/dropjob.cpp
  src/widgets/dropjob.h

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

Re: KApplicationTrader API

[Albert Astals Cid]

El diumenge, 12 de gener de 2020, a les 11:50:03 CET, David Faure va escriure:
> I wrote a KApplicationTrader class in https://phabricator.kde.org/D25698
> and I need input on API.
> 
> Use case: the user types "km" in krunner.
> In order to find all applications that contain "km" (case insensitive), do we 
> want to write
> 
> auto offers = KApplicationTrader::self()->query("'km' ~~ Name");
> 
> or
> 
> auto filter = [](const KService::Ptr ) { return 
> service->name().contains("km", Qt::CaseInsensitive); }
> auto offers = KApplicationTrader::self()->query(filter);
> 
> ?

The second one fails if i type service->name() wrong, the first one doesn't if 
i write NaMe, so the second one is millions of times better in that regard in 
my opinion.

I mean i already know how to write C++ why should i learn "weird language"?

Cheers,
  Albert

Re: KApplicationTrader API

[Ivan Čukić]

> auto filter = [](const KService::Ptr ) { return
> service->name().contains("km", Qt::CaseInsensitive); } auto offers =
> KApplicationTrader::self()->query(filter);

I'd vote for lambda/function object/callable (hello std::invoke in KF6?). If a 
more terse syntax is needed, it could be later provided with an eDSL along the 
lines of:

KApplicationTrader::query(name.contains("km", Qt::CaseInsensitive));
KApplicationTrader::query(
filter::name.endsWith("km") &&
filter::showInKDE);


> auto offers = KApplicationTrader::query(...);

+1, no need to put everything in an object.


> Note that the least-porting-effort solution is self() and trader language,
> since the code currently uses KServiceTypeTrader for this.

I'm aware I voted for the most-porting-effort solution ;)


Cheers,
Ivan


-- 
dr Ivan Čukić
KDE, ivan.cu...@kde.org, https://cukic.co/
gpg key fingerprint: 8FE4 D32F 7061 EA9C 8232  07AE 01C6 CE2B FF04 1C12

D26484: Add KIO::DropJobFlag to allow manually showing the menu

[David Faure]

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


  Almost there.
  
  Please add @since 5.67 to the new enum and the new methods.

INLINE COMMENTS

> dfaure wrote in dropjob.cpp:416
> This '{' goes on its own line.

Not fixed.

> dropjob.h:147
>  
> +KIOWIDGETS_EXPORT DropJob *drop(const QDropEvent *dropEvent, const QUrl 
> , DropJobFlags dropjobFlags, JobFlags flags = DefaultFlags); // BCI: 
> merge wih DropJobFlags dropjobFlag = DropJobDefaultFlag
> +

Please document this method (copy/pasting is OK)

And add @since 5.67

REPOSITORY
  R241 KIO

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

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

D26457: Introduce shadows API

[Vlad Zahorodnii]

zzag retitled this revision from "Introduce shadow API" to "Introduce shadows 
API".

REPOSITORY
  R278 KWindowSystem

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

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

D26457: Introduce shadow API

[Vlad Zahorodnii]

zzag updated this revision to Diff 73352.
zzag added a comment.


  - Make KWindowShadow a QObject subclass (for memory management)
  - Clarify that it is okay to call destroy() after window() had been removed
  - Check whether QWindow has QPlatformWindow associated with it

REPOSITORY
  R278 KWindowSystem

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26457?vs=72880=73352

BRANCH
  kwindowshadow

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

AFFECTED FILES
  src/CMakeLists.txt
  src/kwindowshadow.cpp
  src/kwindowshadow.h
  src/kwindowshadow_dummy_p.h
  src/kwindowshadow_p.h
  src/kwindowsystemplugininterface.cpp
  src/kwindowsystemplugininterface_p.h
  src/platforms/xcb/CMakeLists.txt
  src/platforms/xcb/kwindowshadow.cpp
  src/platforms/xcb/kwindowshadow_p_x11.h
  src/platforms/xcb/plugin.cpp
  src/platforms/xcb/plugin.h
  src/pluginwrapper.cpp
  src/pluginwrapper_p.h

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

D26607: Fix build on Windows

[Hannah von Reth]

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

REPOSITORY
  R241 KIO

BRANCH
  master

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

AFFECTED FILES
  tests/kioslavetest.cpp

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

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava added a comment.


  @dfaure , @ervin
  
  Right now the code passes all the tests and I tried to be extra careful not 
to break away with the general architecture. I think it's the first "safe" 
version to review.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 73345.
tcanabrava added a comment.


  - Fix bug ediging param variable that's supposed to be const
  - Const Correctness:

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=73336=73345

BRANCH
  arcpatch-D26202

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

AFFECTED FILES
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  autotests/kconfig_compiler/test1.h.ref
  autotests/kconfig_compiler/test10.h.ref
  autotests/kconfig_compiler/test11.h.ref
  autotests/kconfig_compiler/test11a.h.ref
  autotests/kconfig_compiler/test12.h.ref
  autotests/kconfig_compiler/test13.h.ref
  autotests/kconfig_compiler/test9.h.ref
  autotests/kconfig_compiler/test_dpointer.cpp.ref
  autotests/kconfig_compiler/test_translation_kde.h.ref
  autotests/kconfig_compiler/test_translation_kde_domain.h.ref
  autotests/kconfig_compiler/test_translation_qt.h.ref
  autotests/kconfigtest.h
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KCFGXmlParser.cpp
  src/kconfig_compiler/KCFGXmlParser.h
  src/kconfig_compiler/KConfigCodeGeneratorBase.cpp
  src/kconfig_compiler/KConfigCodeGeneratorBase.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26532: Don't use KWindowSystem on Android

[Nicolas Fella]

This revision was automatically updated to reflect the committed changes.
Closed by commit R289:33742dde2f73: Dont use KWindowSystem on Android 
(authored by nicolasfella).

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26532?vs=73095=73343

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

AFFECTED FILES
  CMakeLists.txt
  src/CMakeLists.txt
  src/config-knotifications.h.cmake
  src/kpassivepopup.cpp

To: nicolasfella, #frameworks, vkrause, apol, dfaure
Cc: dfaure, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26604: Check if there is an activatable service when notification service owner changes

[Nicolas Fella]

nicolasfella added a dependent revision: D26605: Remove fallback to 
KPassivePopup.

REPOSITORY
  R289 KNotifications

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

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

D26605: Remove fallback to KPassivePopup

[Nicolas Fella]

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

REVISION SUMMARY
  KNotifications has proper support for all major platforms (freedesktop, 
windows, macOS, Android). The KPassivePopup fallback is only relevant for Linux 
when no FDO notification daemon is running. D26604 
 addresses the corner case of a crashing 
notification daemon. IMO the case of the user having no notification daemon at 
all is not worth supporting since the users intention is verly clearly to not 
have notifications.
  
  This allows to drop a good amount of code. It reduces the dependency on 
Widgets so we can eventually get rid of it which is nice for Android. 
Furthermore the reduced complexity will make it easier to implement our plans 
for KF6

TEST PLAN
  Still get regular notifications
  
  Depends on D26604 

REPOSITORY
  R289 KNotifications

BRANCH
  nofallback

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

AFFECTED FILES
  src/notifybypopup.cpp
  src/notifybypopup.h

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

D25517: Add an option to extract image data and add front cover property

[Alexander Stippich]

astippich marked 2 inline comments as done.
astippich added a comment.


  In D25517#588029 , @bruns wrote:
  
  > The problem is the extractor serves two different use cases:
  >
  > - retrieval for searching/caching properties
  > - on-demand extraction
  >
  >   Baloo falls into the first category. It is only interested in properties 
which can be queried/compared in a meaningful way. Even storing the properties 
as a cache for e.g. Dolphin is a little bit of a stretch.
  >
  >   Elisa falls into the second category. It does not store the extracted 
data persistently, but is fine extracting e.g. the front cover on demand.
  >
  >   Currently the only large/blob property is the front cover. But what about 
BackCover, Leaflet pages, ..., Video Thumbnails? Returning all of these even 
when not required is wasteful. If you want a front cover, say so.
  
  
  Yeah, in that case it would be a little bit wasteful. But again, I think if 
we implement this fine-grained control, it should be done for all properties.
  And this is out-of-scope for this patch.
  
  > For KF6, ExtractEverything should just be removed. It is not especially 
useful, not even for unit tests.
  >  Request the data you want and are able to handle, and do so explicitly.
  
  I have adjusted this as requested.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #baloo, bruns, mgallien, ngraham
Cc: kde-frameworks-devel, hurikhan77, lots0logs, LeGast00n, fbampaloukas, GB_2, 
domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D26604: Check if there is an activatable service when notification service owner changes

[Nicolas Fella]

nicolasfella updated this revision to Diff 73340.
nicolasfella added a comment.


  - Move code

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26604?vs=73338=73340

BRANCH
  checkactivatable

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

AFFECTED FILES
  src/notifybypopup.cpp

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

D26604: Check if there is an activatable service when notification service owner changes

[Nicolas Fella]

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

REVISION SUMMARY
  At notification backend initialization we don't just check whether the 
notification service exists but also whether one could be activated and treat 
it as existant then.
  When the service owner changes (e.g. plasmashell is crashing) we check if a 
new service exists but not whether one could be activated. A notification 
coming in while plasmashell is crashed thus triggeres the horrible 
KPassivePopup fallback.
  
  plasmashell itself is not DBus activatable, but it has a waitforname helper 
that is and forwards the notification to Plasma. With this patch instead of 
using the fallback we DBus-activate the helper and the notification appears as 
soon as Plasma is restarted.

TEST PLAN
  - application is started and sends notification after plasma started: No 
change
  - application sends notification before plasma is started: No change, 
notification is shown as soon as Plasma starts
  - application sends notification with running plasma, then plasma quits, 
application sends another notification, plasma restarts: Second notification is 
shown as soon as plasma starts instead of creating KPassivePopup

REPOSITORY
  R289 KNotifications

BRANCH
  checkactivatable

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

AFFECTED FILES
  src/notifybypopup.cpp

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

D26484: Add KIO::DropJobFlag to allow manually showing the menu

[Tranter Madi]

trmdi edited the summary of this revision.

REPOSITORY
  R241 KIO

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

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

D26484: Add KIO::DropJobFlag to allow manually showing the menu

[Tranter Madi]

trmdi retitled this revision from "Add KIO::DropJobFlag flag and 
DropJob::menuPopup()" to "Add KIO::DropJobFlag to allow manually showing the 
menu".

REPOSITORY
  R241 KIO

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

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

D26484: Add KIO::DropJobFlag flag and DropJob::menuPopup()

[Tranter Madi]

trmdi retitled this revision from "Add KIO::DelayPopup flag and 
DropJob::menuPopup()" to "Add KIO::DropJobFlag flag and DropJob::menuPopup()".

REPOSITORY
  R241 KIO

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

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

D26484: Add KIO::DelayPopup flag and DropJob::menuPopup()

[Tranter Madi]

trmdi updated this revision to Diff 73337.
trmdi added a comment.


  - Move the flag to DropJob header, rename

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26484?vs=73273=73337

BRANCH
  master

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

AFFECTED FILES
  src/widgets/dropjob.cpp
  src/widgets/dropjob.h

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

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava marked 8 inline comments as done.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava added inline comments.

INLINE COMMENTS

> tcanabrava wrote in kconfig_compiler.cpp:753
> that was a bit harder than I want, but done. Inside of the code generation 
> there was code that manipulated the ParseResult. I think this is one of the 
> good spots that show that this rewrite is really needed.

aand no, this introduced regressions, I'll try to solve it later but it's 
not as simple. this will unfortunately still be modified inside of the 
generator files. :/

REPOSITORY
  R237 KConfig

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

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 73336.
tcanabrava marked 4 inline comments as done.
tcanabrava added a comment.


  - Rename KConfigCodeGenerator to KconfigCodeGeneratorBase
  - Simplify diff file saving
  - Add License Headers
  - Documentation, Privatization, Unused method / variable
  - Const, Documentation
  - Newlines at the end of files
  - Spacing
  - Remove Globals
  - Remove unused code and Debugs
  - Constify ParseResult
  - English Typos
  - qAsConst
  - Public / Private issues
  - Simplify Construction
  - Revert "Constify ParseResult"

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26202?vs=73310=73336

BRANCH
  arcpatch-D26202

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

AFFECTED FILES
  autotests/kconfig_compiler/kconfigcompiler_test.cpp
  autotests/kconfig_compiler/test1.h.ref
  autotests/kconfig_compiler/test10.h.ref
  autotests/kconfig_compiler/test11.h.ref
  autotests/kconfig_compiler/test11a.h.ref
  autotests/kconfig_compiler/test12.h.ref
  autotests/kconfig_compiler/test13.h.ref
  autotests/kconfig_compiler/test9.h.ref
  autotests/kconfig_compiler/test_dpointer.cpp.ref
  autotests/kconfig_compiler/test_translation_kde.h.ref
  autotests/kconfig_compiler/test_translation_kde_domain.h.ref
  autotests/kconfig_compiler/test_translation_qt.h.ref
  autotests/kconfigtest.h
  src/kconfig_compiler/CMakeLists.txt
  src/kconfig_compiler/KCFGXmlParser.cpp
  src/kconfig_compiler/KCFGXmlParser.h
  src/kconfig_compiler/KConfigCodeGeneratorBase.cpp
  src/kconfig_compiler/KConfigCodeGeneratorBase.h
  src/kconfig_compiler/KConfigCommonStructs.h
  src/kconfig_compiler/KConfigHeaderGenerator.cpp
  src/kconfig_compiler/KConfigHeaderGenerator.h
  src/kconfig_compiler/KConfigSourceGenerator.cpp
  src/kconfig_compiler/KConfigSourceGenerator.h
  src/kconfig_compiler/KConfigXTParameters.cpp
  src/kconfig_compiler/KConfigXTParameters.h
  src/kconfig_compiler/kconfig_compiler.cpp

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D26202: WIP: Refactor KConfigXT

[Tomaz Canabrava]

tcanabrava marked 22 inline comments as done.
tcanabrava added inline comments.

INLINE COMMENTS

> dfaure wrote in kconfigcompiler_test.cpp:129
> Why did you remove this? It's just a more user-friendly version of the 
> QVERIFY on the next line, so it can't possibly have failed while the next 
> line passes...

the text was really not friendly, it was a big blob of diff content pasted on 
screen. Considering that the next line will also fail I replaced this by saving 
the diff file on disk, this way we can actually look at the file that failed 
and take the time to understand the error.

> dfaure wrote in KConfigCodeGenerator.h:91
> urgh, a public variable

that was a honest mistake :)

> dfaure wrote in KConfigCommonStructs.h:14
> done already?

No, there's a struct SignalArguments and a struct Param that are  basically the 
same thing. A name and a Type. Still in the TODO.

> dfaure wrote in KConfigHeaderGenerator.h:73
> I thought most editors took care of that, these days...

Kate / KDevelop here. I'm manually adding them.

> dfaure wrote in KConfigSourceGenerator.cpp:40
> remove (or switch to qCDebug)

removed, those are temporaries debug calls that I used to be sure things are as 
supposed.

> dfaure wrote in kconfig_compiler.cpp:753
> const ... ?

that was a bit harder than I want, but done. Inside of the code generation 
there was code that manipulated the ParseResult. I think this is one of the 
good spots that show that this rewrite is really needed.

REPOSITORY
  R237 KConfig

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

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25660: Decouple KBookmarksMenu from KActionCollection

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Some API (docs) nitpicks :)

INLINE COMMENTS

> kbookmarkmenu.cpp:88
> +m_parentMenu(_parentMenu),
> +m_parentAddress(QLatin1String(""))   //TODO KBookmarkAdress::root
> +{

Why the `QLatin1String("")` instead of QString()? Is there a need for a 
non-null empty string? It#s copied from the other constructor, but should get a 
comment while at it if that is the case, or just get to be QString()

> kbookmarkmenu.h:89
>   */
> +#if KBOOKMARKS_ENABLE_DEPRECATED_SINCE(5, 65)
> +KBOOKMARKS_DEPRECATED_VERSION(5, 65, "Use overload without 
> KActionCollection and add actions manually to your actioncollection if 
> desired")

The visibility guard for deprecated methods should start before the API dox , 
if only for consistency, so please move to front.

See https://community.kde.org/Policies/Library_Code_Policy#Deprecation_of_API

> kbookmarkmenu.h:99
> + *
> + * @param mgr The bookmark manager to use (i.e. for reading and writing)
> + * @param owner implementation of the KBookmarkOwner callback interface.

-> lowercase "the", cmp. 
https://community.kde.org/Frameworks/Frameworks_Documentation_Policy#Document_Public_and_Protected_Members

> kbookmarkmenu.h:101
> + * @param owner implementation of the KBookmarkOwner callback interface.
> + * Note: If you pass a null KBookmarkOwner to the constructor, the
> + * openBookmark signal is not emitted, instead QDesktopServices::openUrl 
> is used to open the bookmark.

`Note:` -> `@note`

> kbookmarkmenu.h:104
> + * @param parentMenu menu to be filled
> + * @param collec parent collection for the KActions.
> + * @since 5.65

No such parameter "collec"?

> kbookmarkmenu.h:107
> + */
> +KBookmarkMenu(KBookmarkManager *mgr, KBookmarkOwner *owner, QMenu 
> *parentMenu);
>  

"mgr" -> "manager" while at it, please

> kbookmarkmenu.h:144
> +/**
> + * Action for adding a bookmark. If you are using KXMLGui add it to your 
> action collection.
> + * @code

Official casing is "KXmlGui" :) Also below.

> kbookmarkmenu.h:144
> +/**
> + * Action for adding a bookmark. If you are using KXMLGui add it to your 
> action collection.
> + * @code

This being a method, not a property "Action which is" is not the perfect text, 
please describe what the method does (getting access to the action owned by the 
menu).

> kbookmarkmenu.h:149
> + * @endcode
> + * @since 5.65
> + */

Please add a `@return ...` here and to the other methods, so e.g. IDEs can have 
better metadata avaialble.

> kbookmarkmenu.h:151
> + */
> +QAction *addBookmarkAction();
> +

Can be const methods, no?

REPOSITORY
  R294 KBookmarks

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

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

KDE CI: Frameworks » kglobalaccel » kf5-qt5 WindowsMSVCQt5.13 - Build # 35 - Failure!

[CI System]

balaccel.h[2020-01-12T12:14:37.341Z] -- Installing: C:/CI/workspace/Frameworks/kglobalaccel/kf5-qt5 WindowsMSVCQt5.13/install-divert/CI/Software Installs/kglobalaccel/include/KF5/KGlobalAccel/kglobalshortcutinfo.h[2020-01-12T12:14:37.341Z] -- Installing: C:/CI/workspace/Frameworks/kglobalaccel/kf5-qt5 WindowsMSVCQt5.13/install-divert/CI/Software Installs/kglobalaccel/bin/data/doc/qch/KF5GlobalAccel.qch[2020-01-12T12:14:37.341Z] -- Installing: C:/CI/workspace/Frameworks/kglobalaccel/kf5-qt5 WindowsMSVCQt5.13/install-divert/CI/Software Installs/kglobalaccel/bin/data/doc/qch/KF5GlobalAccel.tags[2020-01-12T12:14:37.341Z] -- Installing: C:/CI/workspace/Frameworks/kglobalaccel/kf5-qt5 WindowsMSVCQt5.13/install-divert/CI/Software Installs/kglobalaccel/mkspecs/modules/qt_KGlobalAccel.pri[2020-01-12T12:14:37.341Z] -- Installing: C:/CI/workspace/Frameworks/kglobalaccel/kf5-qt5 WindowsMSVCQt5.13/install-divert/CI/Software Installs/kglobalaccel/lib/KF5GlobalAccelPrivate.lib[2020-01-12T12:14:37.341Z] -- Installing: C:/CI/workspace/Frameworks/kglobalaccel/kf5-qt5 WindowsMSVCQt5.13/install-divert/CI/Software Installs/kglobalaccel/bin/KF5GlobalAccelPrivate.dll[2020-01-12T12:14:37.341Z] -- Installing: C:/CI/workspace/Frameworks/kglobalaccel/kf5-qt5 WindowsMSVCQt5.13/install-divert/CI/Software Installs/kglobalaccel/bin/kglobalaccel5.exe[2020-01-12T12:14:37.341Z] -- Installing: C:/CI/workspace/Frameworks/kglobalaccel/kf5-qt5 WindowsMSVCQt5.13/install-divert/CI/Software Installs/kglobalaccel/bin/data/kservices5/kglobalaccel5.desktop[2020-01-12T12:14:37.341Z] -- Installing: C:/CI/workspace/Frameworks/kglobalaccel/kf5-qt5 WindowsMSVCQt5.13/install-divert/CI/Software Installs/kglobalaccel/include/KF5/KGlobalAccel/private/kf5globalaccelprivate_export.h[2020-01-12T12:14:37.341Z] -- Installing: C:/CI/workspace/Frameworks/kglobalaccel/kf5-qt5 WindowsMSVCQt5.13/install-divert/CI/Software Installs/kglobalaccel/include/KF5/KGlobalAccel/private/kglobalacceld.h[2020-01-12T12:14:37.341Z] -- Installing: C:/CI/workspace/Frameworks/kglobalaccel/kf5-qt5 WindowsMSVCQt5.13/install-divert/CI/Software Installs/kglobalaccel/include/KF5/KGlobalAccel/private/kglobalaccel_interface.h[2020-01-12T12:14:37.341Z] -- Installing: C:/CI/workspace/Frameworks/kglobalaccel/kf5-qt5 WindowsMSVCQt5.13/install-divert/CI/Software Installs/kglobalaccel/bin/data/dbus-1/services/org.kde.kglobalaccel.service[Pipeline] }[Pipeline] // stage[Pipeline] stage[Pipeline] { (Running Tests)[Pipeline] bat[2020-01-12T12:14:41.792Z] [2020-01-12T12:14:41.792Z] C:\CI\workspace\Frameworks\kglobalaccel\kf5-qt5 WindowsMSVCQt5.13>call "C:/Program Files (x86)/Microsoft Visual Studio/2019/Professional/VC/Auxiliary/Build/vcvars64.bat" -vcvars_ver=14.16 [2020-01-12T12:14:42.049Z] **[2020-01-12T12:14:42.049Z] ** Visual Studio 2019 Developer Command Prompt v16.4.2[2020-01-12T12:14:42.049Z] ** Copyright (c) 2019 Microsoft Corporation[2020-01-12T12:14:42.049Z] **[2020-01-12T12:14:42.612Z] [vcvarsall.bat] Environment initialized for: 'x64'[2020-01-12T12:14:45.156Z] 'kdeinit5' is not recognized as an internal or external command,[2020-01-12T12:14:45.156Z] operable program or batch file.[2020-01-12T12:14:50.514Z] Cannot find file: C:/CI/workspace/Frameworks/kglobalaccel/kf5-qt5 WindowsMSVCQt5.13/build/DartConfiguration.tcl[2020-01-12T12:14:50.514Z]Site: [2020-01-12T12:14:50.514Z]Build name: (empty)[2020-01-12T12:14:50.514Z] Create new tag: 20200112-1214 - Experimental[2020-01-12T12:14:50.514Z] Cannot find file: C:/CI/workspace/Frameworks/kglobalaccel/kf5-qt5 WindowsMSVCQt5.13/build/DartConfiguration.tcl[2020-01-12T12:14:50.514Z] Test project C:/CI/workspace/Frameworks/kglobalaccel/kf5-qt5 WindowsMSVCQt5.13/build[2020-01-12T12:14:50.514Z] Start 1: kglobalshortcuttest[2020-01-12T12:14:52.424Z] 1/1 Test #1: kglobalshortcuttest ..   Passed2.35 sec[2020-01-12T12:14:52.424Z] [2020-01-12T12:14:52.424Z] 100% tests passed, 0 tests failed out of 1[2020-01-12T12:14:52.424Z] [2020-01-12T12:14:52.424Z] Total Test time (real) =   2.36 sec[2020-01-12T12:14:52.424Z] ERROR: The process "kded5.exe" not found.[2020-01-12T12:14:52.682Z] ERROR: The process "klauncher.exe" not found.[2020-01-12T12:14:52.682Z] ERROR: The process "kdeinit5.exe" not found.[2020-01-12T12:14:52.682Z] ERROR: The process "test_crasher.exe" not found.[2020-01-12T12:14:52.682Z] SUCCESS: The process with PID 18312 (child process of PID 15140) has been terminated.[2020-01-12T12:14:52.682Z] SUCCESS: The process with PID 15140 (child process of PID 8648) has been terminated.[2020-01-12T12:16:00.455Z] ERROR: The process "kioslave.exe" not found.[2020-01-12T12:16:00.455Z] ERROR: The process "vctip.exe" not found.[Pipeline] junit[2020-01-12T12:16:00.717Z] Recording test results[2020-01-12T12:16:00.749Z] Test report

KDE CI: Frameworks » purpose » kf5-qt5 SUSEQt5.13 - Build # 96 - Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/purpose/job/kf5-qt5%20SUSEQt5.13/96/
 Project:
kf5-qt5 SUSEQt5.13
 Date of build:
Sun, 12 Jan 2020 11:32:09 +
 Build duration:
31 min and counting
   BUILD ARTIFACTS
  abi-compatibility-results.yamlacc/KF5Purpose-5.67.0.xmlcompat_reports/KF5Purpose_compat_report.htmllogs/KF5Purpose/5.67.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: 1 test(s), Skipped: 0 test(s), Total: 2 test(s)Failed: projectroot.autotests.menutest
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report23%
(5/22)30%
(14/47)30%
(14/47)22%
(440/2035)18%
(166/939)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests100%
(2/2)100%
(2/2)92%
(135/146)53%
(49/92)src100%
(8/8)100%
(8/8)68%
(225/333)49%
(89/183)src.externalprocess0%
(0/2)0%
(0/2)0%
(0/137)0%
(0/98)src.fileitemactionplugin0%
(0/1)0%
(0/1)0%
(0/24)0%
(0/18)src.plugins.bluetooth0%
(0/1)0%
(0/1)0%
(0/33)0%
(0/8)src.plugins.email0%
(0/1)0%
(0/1)0%
(0/64)0%
(0/24)src.plugins.imgur0%
(0/2)0%
(0/2)0%
(0/184)0%
(0/63)src.plugins.kdeconnect0%
(0/1)0%
(0/1)0%
(0/31)0%
(0/6)src.plugins.kdeconnect_sms0%
(0/1)0%
(0/1)0%
(0/16)0%
(0/2)src.plugins.ktp-sendfile0%
(0/1)0%
(0/1)0%
(0/28)0%
(0/6)src.plugins.pastebin0%
(0/1)0%
(0/1)0%
(0/54)0%
(0/23)src.plugins.phabricator0%
(0/3)0%
(0/3)0%
(0/216)0%
(0/74)src.plugins.phabricator.quick0%
(0/5)0%
(0/5)0%
(0/93)0%
(0/48)src.plugins.phabricator.tests0%
(0/1)0%
(0/1)0%
(0/60)0%
(0/22)src.plugins.reviewboard0%
(0/3)0%
(0/3)0%
(0/229)0%
(0/70)src.plugins.reviewboard.quick0%
(0/7)0%
(0/7)0%
(0/154)0%
(0/80)src.plugins.saveas100%
(1/1)100%
(1/1)57%
(29/51)61%
 

KDE CI: Frameworks » kpeople » kf5-qt5 WindowsMSVCQt5.13 - Build # 41 - Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kpeople/job/kf5-qt5%20WindowsMSVCQt5.13/41/
 Project:
kf5-qt5 WindowsMSVCQt5.13
 Date of build:
Sun, 12 Jan 2020 11:30:42 +
 Build duration:
28 min and counting
   JUnit Tests
  Name: projectroot Failed: 1 test(s), Passed: 2 test(s), Skipped: 0 test(s), Total: 3 test(s)Failed: projectroot.autotests.personsmodeltest

D25660: Decouple KBookmarksMenu from KActionCollection

[David Faure]

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


  Thanks for the patch.
  
  Please call setObjectName() on the actions so that their object name is fixed 
(no matter which ctor is used).  (this is because KActionCollection::addAction 
calls setObjectName on the actions)
  
  This can even help apps to just get all 3 actions and put them into an action 
collection without having to copy the standard names from the documentation.
  
QAction *addAction = menu->addBookmarkAction();
actionCollection()->addAction(addAction->objectName(), addAction);
  
  Maybe you can use the C++11 feature of one ctor calling another to avoid the 
setup() method which will look even more strange once the deprecated ctor is 
removed.
  
  The @since value needs to be updated obviously.

INLINE COMMENTS

> kbookmarkmenu.cpp:347
> +
> +if (m_actionCollection) {
> +m_actionCollection->addAction(QStringLiteral("add_bookmark"), 
> d->addAddBookmark);

The m_bIsRoot check disappeared in the refactoring!

Actions in submenus should not be named.

Same thing for the action named add_bookmarks_list above.

REPOSITORY
  R294 KBookmarks

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

To: nicolasfella, #frameworks, dfaure
Cc: dfaure, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

KDE CI: Frameworks » ktexteditor » kf5-qt5 FreeBSDQt5.13 - Build # 165 - Fixed!

[CI System]

BUILD SUCCESS
 Build URL
https://build.kde.org/job/Frameworks/job/ktexteditor/job/kf5-qt5%20FreeBSDQt5.13/165/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Sun, 12 Jan 2020 11:33:44 +
 Build duration:
22 min and counting
   JUnit Tests
  Name: projectroot Failed: 0 test(s), Passed: 62 test(s), Skipped: 0 test(s), Total: 62 test(s)Name: projectroot.autotests.src Failed: 0 test(s), Passed: 5 test(s), Skipped: 0 test(s), Total: 5 test(s)

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

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kimageformats/job/kf5-qt5%20FreeBSDQt5.13/29/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Sun, 12 Jan 2020 11:46:44 +
 Build duration:
5 min 20 sec and counting
   JUnit Tests
  Name: projectroot Failed: 1 test(s), Passed: 13 test(s), Skipped: 0 test(s), Total: 14 test(s)Failed: projectroot.autotests.kimageformats_read_hdr

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

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kpackage/job/kf5-qt5%20FreeBSDQt5.13/51/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Sun, 12 Jan 2020 11:32:49 +
 Build duration:
15 min and counting
   JUnit Tests
  Name: projectroot Failed: 1 test(s), Passed: 10 test(s), Skipped: 0 test(s), Total: 11 test(s)Failed: projectroot.autotests.plasma_packagestructuretest

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

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kpty/job/kf5-qt5%20FreeBSDQt5.13/30/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Sun, 12 Jan 2020 11:31:34 +
 Build duration:
13 min 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.kptyprocesstest

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

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kdesu/job/kf5-qt5%20FreeBSDQt5.13/35/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Sun, 12 Jan 2020 11:31:20 +
 Build duration:
12 min 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.kdesutest

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

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20FreeBSDQt5.13/265/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Sun, 12 Jan 2020 11:23:45 +
 Build duration:
9 min 47 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)

D26557: Allow to handle apps with Terminal=True in their desktop file, handle their associated mimetype properly

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> dfaure wrote in desktopexecparser.cpp:213
> Move result of method call into local const variable. The usual 
> range-for-detaches problem.
> 
> Can you call mimeTypes() instead of serviceTypes() here? I'm trying to slowly 
> split the two notions, after it all got mixed up long ago.

> Can you call mimeTypes() instead of serviceTypes() here? I'm trying to slowly 
> split the two notions, after it all got mixed up long ago.

No in fact, "x-scheme-handler/" are not included in 
KService::mimeTypes() :

>   if (db.mimeTypeForName(sv).isValid()) { // keep only mimetypes, filter out 
> servicetypes

REPOSITORY
  R241 KIO

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

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

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

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kdeclarative/job/kf5-qt5%20FreeBSDQt5.13/69/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Sun, 12 Jan 2020 11:27:42 +
 Build duration:
4 min 43 sec and counting
   JUnit Tests
  Name: projectroot Failed: 1 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 2 test(s)Failed: projectroot.autotests.quickviewsharedengine

D26557: Allow to handle apps with Terminal=True in their desktop file, handle their associated mimetype properly

[David Faure]

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


  Please add me as reviewer when touching my code in KIO. I almost didn't 
notice this one.

INLINE COMMENTS

> desktopexecparser.cpp:213
> +// add x-scheme-handler/
> +for (const auto  : service.serviceTypes()) {
> +if (mimeType.startsWith(QStringLiteral("x-scheme-handler/"))) {

Move result of method call into local const variable. The usual 
range-for-detaches problem.

Can you call mimeTypes() instead of serviceTypes() here? I'm trying to slowly 
split the two notions, after it all got mixed up long ago.

> desktopexecparser.cpp:214
> +for (const auto  : service.serviceTypes()) {
> +if (mimeType.startsWith(QStringLiteral("x-scheme-handler/"))) {
> +supportedProtocols << mimeType.mid(17);

QLatin1String is enough here

> krun.cpp:101
> -{
> -// We have up to two sources of data, for protocols not handled by 
> kioslaves (so called "helper") :
> -// 1) the exec line of the .protocol file, if there's one

OK I wasn't happy about this docu disappearing but actually it fits better into 
hasSchemeHandler() which I later on moved to KIO::DesktopExecParser.

Done in https://commits.kde.org/kio/45f5d79600809f4c153c7b39ad90652cb921875c
You can now remove it from here indeed :)

> krun.cpp:106
>  }
> -Q_ASSERT(KProtocolInfo::isHelperProtocol(protocol));
> -return KProtocolInfo::exec(protocol);
> +return KService::Ptr();
>  }

Just remove the if then. This method becomes a one-liner.

> krun.cpp:956
> +//  if there's one...
> +if (runApplication(*service, QList() << d->m_strURL, 
> d->m_window)) {
>  d->m_bFinished = true;

You forgot to pass d->m_asn here (last argument of runApplication).

> krun.cpp:967
> +// the scheme has no service or protocol associated
> +// use url scheme associated protocol
> +
> mimeTypeDetermined(KProtocolManager::defaultMimetype(d->m_strURL));

I think what you meant here was scheme-associated *mimetype* ?
or "default mimetype from the protocol file".

In a URL, scheme==protocol, but in KIO we're mostly using the word protocol to 
refer to those .protocol files. KProtocolInfo::exec() is also reading from the 
[scheme associated] protocol file so this comment is confusing.

(I wonder if this isn't dead code though - helper protocols don't set an empty 
exec line and a default mimetype, this would make no sense, only real ioslaves 
like kio_man set a default mimetype, text/html)

> krun.cpp:971
> +} else {
> +if ( run(exec, QList() << d->m_strURL, d->m_window, 
> QString(), QString(), d->m_asn)) {
> +d->m_bFinished = true;

remove space before 'run'

REPOSITORY
  R241 KIO

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

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

KDE CI: Frameworks » kbookmarks » kf5-qt5 WindowsMSVCQt5.13 - Build # 44 - Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kbookmarks/job/kf5-qt5%20WindowsMSVCQt5.13/44/
 Project:
kf5-qt5 WindowsMSVCQt5.13
 Date of build:
Sun, 12 Jan 2020 10:54:03 +
 Build duration:
28 min and counting
   JUnit Tests
  Name: projectroot Failed: 1 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 2 test(s)Failed: projectroot.autotests.kbookmarktest

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

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kdeclarative/job/kf5-qt5%20FreeBSDQt5.13/68/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Sun, 12 Jan 2020 11:04:31 +
 Build duration:
1 min 58 sec and counting
   JUnit Tests
  Name: projectroot Failed: 1 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 2 test(s)Failed: projectroot.autotests.quickviewsharedengine

D26603: Port away from deprecated IconSize() method

[Volker Krause]

This revision was automatically updated to reflect the committed changes.
Closed by commit R296:3489a1d049fb: Port away from deprecated IconSize() method 
(authored by vkrause).

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26603?vs=73332=7

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

AFFECTED FILES
  src/kdeclarative/private/kiconprovider.cpp

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

D26603: Port away from deprecated IconSize() method

[David Faure]

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


  OK, simpler than I thought.

REPOSITORY
  R296 KDeclarative

BRANCH
  master

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

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

D26057: Deprecate the top-level IconSize() function

[Volker Krause]

vkrause added a comment.


  In D26057#592147 , @dfaure wrote:
  
  > Please port kdeclarative.
  >
  > kdeclarative/private/kiconprovider.cpp:44:56: error: ‘IconSize’ was not 
declared in this scope
  >
  >   pixmap = 
QIcon::fromTheme(source.at(0)).pixmap(IconSize(KIconLoader::Desktop));
  >   
  >
  > Detected by http://www.davidfaure.fr/2020/542.diff (to be applied to 
kdeclarative)
  >
  > Thanks.
  
  
  Done in D26603 .

REPOSITORY
  R302 KIconThemes

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

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

D26603: Port away from deprecated IconSize() method

[Volker Krause]

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

REPOSITORY
  R296 KDeclarative

BRANCH
  master

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

AFFECTED FILES
  src/kdeclarative/private/kiconprovider.cpp

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

D26564: Clean kbuildsycoca4 support

[David Faure]

dfaure added a comment.


  Code added by Eike Hein in commit ee5303a337876914 
, 
let's get his input.

REPOSITORY
  R241 KIO

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

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

D26564: Clean kbuildsycoca4 support

[David Faure]

dfaure added a reviewer: hein.

REPOSITORY
  R241 KIO

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

To: meven, ervin, #frameworks, hein
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

KApplicationTrader API

[David Faure]

I wrote a KApplicationTrader class in https://phabricator.kde.org/D25698
and I need input on API.

Use case: the user types "km" in krunner.
In order to find all applications that contain "km" (case insensitive), do we 
want to write

auto offers = KApplicationTrader::self()->query("'km' ~~ Name");

or

auto filter = [](const KService::Ptr ) { return 
service->name().contains("km", Qt::CaseInsensitive); }
auto offers = KApplicationTrader::self()->query(filter);

?

And once we have decided on trader-language (like the old traders) vs filter 
function (like KPluginLoader)
we need to decide on how to provide such a global method in a C++ world ;)

auto offers = KApplicationTrader::self()->query(...);
or
auto offers = KApplicationTrader::query(...);
or
auto offers = KApplicationTrader().query(...);

Input very welcome.

Note that the least-porting-effort solution is self() and trader language, 
since the code currently uses KServiceTypeTrader for this.

But I'm tempted to say lambdas are nicer (you can debug the filtering more 
easily), and would allow to kill the whole
trader-language-parser from kservice in KF6.
And for the second question, Qt doesn't have any self(). I don't think it has 
many functions-in-namespace either?
The last one is more like QFontDatabase/QMimeDatabase, but creating an instance 
is annoying.
I think I like the middle one (function-in-namespace) best, from the 
calling-site point of view.

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

D26595: Add network-wireless-hotspot icon

[Noah Davis]

ndavis added a comment.


  In D26595#592368 , @ngraham wrote:
  
  > Hmm, at normal size, the 8, 16, and 22px versions look too busy to me.:
  
  
  I kind of agree.
  
  > For the 16 and 22px versions, maybe reduce the number of waves and increase 
the spacing between them. And do we even need an 8px version?
  
  We don't need an 8px version.
  
  @cblack, if we did need an 8px version, you'd have to edit `index.theme` and 
define the 8px device icons folder.
  
  -
  
  One trick I learned from looking at the wifi icon is that you don't have to 
perfectly center the lines and the cut. If you offset them in a consistent way, 
you can get the spacing you want and the lines will still look fine.
  
  Here are edited versions of the 16 and 22px icons with a couple variations 
each: F7878874: network-wireless-hotspot.svg 
 F7878875: network-wireless-hotspot.svg 

  
  In the 16px version, I used a 14px outer circle, lowered it by 1px and cut it 
with a right triangle.
  
  F7878890: Screenshot_20200112_053351.PNG 

  
  In the 22px version, I cut it the same way, but only to give the triangle 
bottom variant a bit more space. It's not needed if you use the antenna variant.
  F7878893: Screenshot_20200112_053723.PNG 

  
  --
  
  I think the radio waves around the 32px version should be 1px thick.

REPOSITORY
  R266 Breeze Icons

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

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

D26602: Define K_DOXYGEN as macro to check if kapidox/doxygen is run

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Some similar macro definitions used by other projects, as I found by doing 
some random grepping over my system includes:
  
  - `__GTK_DOC_IGNORE__`
  - `Q_QDOC` & `Q_CLANG_QDOC`
  - `DO_NOT_DOCUMENT`
  - `EIGEN_PARSED_BY_DOXYGEN`
  - `DOXYGEN`
  - `U_IN_DOXYGEN`
  - `BOOST_DOXYGEN_INVOKED` & lots of similar
  - `CV_DOXYGEN`
  - `DOXYGEN_ENABLED`
  
  So seems there is no real standard sadly. Going for a `K_` prefix as 
namespace should also avoid any conflicts with potential definitions pulled in 
from others.
  
  If this is in, I would then also adapt all KF code as well as ECMADDQCH.

REPOSITORY
  R264 KApiDox

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

To: kossebau, #frameworks
Cc: kde-frameworks-devel, kde-doc-english, LeGast00n, gennad, fbampaloukas, 
GB_2, michaelh, ngraham, bruns, skadinna

D26602: Define K_DOXYGEN as macro to check if kapidox/doxygen is run

[Friedrich W. H. Kossebau]

kossebau created this revision.
kossebau added a reviewer: Frameworks.
Herald added projects: Frameworks, Documentation.
Herald added subscribers: kde-doc-english, kde-frameworks-devel.
kossebau requested review of this revision.

REVISION SUMMARY
  Currently kapidox uses the macro DOXYGEN_SHOULD_SKIP_THIS to enable code
  it processes to control what doxygen sees. That macro, while taken from
  the example given in the official doxygen documentation,
  ( http://www.doxygen.nl/manual/faq.html#faq_code ) has some disadvantages:
  
  - it is not a macro officially set by doxygen
  - its semantic can be confusing to human readers in case it needs to be used 
with #ifdef instead of the #ifndef as in the doxygen documentation example
  
  Example:
  #ifdef DOXYGEN_SHOULD_SKIP_THIS
  // will this code be processed by kapidox/doxygen? Yes, it will
  #endif
  
  Using a K_DOXYGEN macro definition instead follows an existing pattern
  known from code checking the (compiler) tool being processed by, like
  __GNUC__, __clang__ or Q_QDOC.

REPOSITORY
  R264 KApiDox

BRANCH
  introduceK_DOXYGEN

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

AFFECTED FILES
  README.md
  src/kapidox/data/Doxyfile.global

To: kossebau, #frameworks
Cc: kde-frameworks-devel, kde-doc-english, LeGast00n, gennad, fbampaloukas, 
GB_2, michaelh, ngraham, bruns, skadinna

D26407: KFileItem: Improve isSlow to not block when a network mount is unresponsive, make SkipMimeTypeFromContent skip only on slow fs

[David Faure]

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

INLINE COMMENTS

> meven wrote in kfileitem.cpp:787
> I did not mean to avoid android, just surfaced the comment in 
> KMountPoint::currentMountPoints regarding its limitation to make it clear why 
> we need an else block here in the first place.

OK, but I still read "can only mean" like a possibly incorrect assumption.

I suggest s/can only mean/for instance/

> kfileitem.cpp:771
> +// refresh cached mountpoints data
> +mountPoints = KMountPoint::currentMountPoints();
> +}

You forgot to update lastMountPointUpdate here.

And to avoid calling currentDateTime() twice (this is a relatively slow 
method), make sure to put the result into a local var used in both places.

> kfileitem.cpp:1246
> +// avoid potential blocking stat on network mount
> +if (d->m_bSkipMimeTypeFromContent && d->isSlow()) {
>  return false;

Did you mean || ?

Otherwise this changes the behaviour also on fast local paths (when 
SkipMimeTypeFromContent is set).

Then again, D19887  (which introduced this 
if) was apparently about network mounts.

> kmountpoint.h:66
> + */
> +Ptr findByPathDirectly(const QString ) const;
> +

I don't really like the name, it's non-obvious until reading the documentation.
Maybe it should be findByPath(path, KMountPoint::DontResolveSymlinks) ?
(with a 2-args findByPath overload and a KF6 TODO to merge the two)

REPOSITORY
  R241 KIO

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

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

D26202: WIP: Refactor KConfigXT

[David Faure]

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

INLINE COMMENTS

> kconfigcompiler_test.cpp:129
> -// use split('\n') to avoid
> -// the whole output shown inline
> -QCOMPARE(content.split('\n'), contentRef.split('\n'));

Why did you remove this? It's just a more user-friendly version of the QVERIFY 
on the next line, so it can't possibly have failed while the next line passes...

> kconfigcompiler_test.cpp:180
> +QFile diffFile(oldFile + QStringLiteral(".diff"));
> +diffFile.open(QIODevice::WriteOnly);
> +

QVERIFY(...)

> kconfigcompiler_test.cpp:183
> +QTextStream writer();
> +writer << out;
> +diffFile.close();

diffFile.write(out); would be simpler than going via QTextStream

> kconfigcompiler_test.cpp:184
> +writer << out;
> +diffFile.close();
>  }

not needed, the dtor closes

> KCFGXmlParser.cpp:20
> +
> +// TODO: Remove those global.
> +extern QStringList allNames;

Yes ;)

> KCFGXmlParser.h:1
> +#ifndef KCFGXMLPARSER_H
> +#define KCFGXMLPARSER_H

missing license header

(repeats)

> KCFGXmlParser.h:15
> +void start();
> +QString errorString();
> +ParseResult getParseResult();

... const;

> KCFGXmlParser.h:16
> +QString errorString();
> +ParseResult getParseResult();
> +

... const;

> KCFGXmlParser.h:28
> +// Those are the Entries in the Xml, that represent a parameter within 
> the   tag.
> +void readParameterFromEntry(CfgEntry , const QDomElement );
> +bool hasDefaultCode(CfgEntry , const QDomElement );

(to pick on one random method as an example) : this is only used internally by 
the class, it should be made private. Please do this for all methods that are 
not used from outside the class. It will make it much clearer what's the actual 
API of the class.

> KCFGXmlParser.h:34
> +
> +// TODO: Fix this function later.
> +CfgEntry *parseEntry(const QString , const QDomElement );

how, why, when?

> KCFGXmlParser.h:45
> +#endif
> \ No newline at end of file


should be fixed

> KConfigCodeGenerator.h:34
> +
> +/* This class manages the base of writting a C - Based code */
> +class KConfigCodeGenerator {

writing takes a single 't'

> KConfigCodeGenerator.h:35
> +/* This class manages the base of writting a C - Based code */
> +class KConfigCodeGenerator {
> +public:

Maybe call it KConfigCodeGeneratorBase ?

It wasn't obvious to me initially that it was "just" a base class.

> KConfigCodeGenerator.h:37
> +public:
> +enum ScopeFinalizer {None, Semicollon};
> +

Semicolon takes a single 'l'

> KConfigCodeGenerator.h:91
> +
> +public:
> +int indentLevel = 0;

urgh, a public variable

> KConfigCommonStructs.h:14
> +
> +// TODO: Remove Signal Arguments for `Param`
> +struct Param

done already?

> KConfigHeaderGenerator.cpp:52
> +
> +for (auto *entry : parseResult.entries) {
> +const QString n = entry->name;

qAsConst()

> KConfigHeaderGenerator.cpp:140
> +stream << endl;
> +// HACK: Original files ended with two last spaces, add them.
> +stream << endl;

s/spaces/newlines/

> KConfigHeaderGenerator.cpp:214
> +{
> +
> +}

nothing here?

> KConfigHeaderGenerator.h:73
> +#endif
> \ No newline at end of file


I thought most editors took care of that, these days...

> KConfigSourceGenerator.cpp:40
> +{
> +qDebug() << "Source Includes" << cfg.sourceIncludes;
> +

remove (or switch to qCDebug)

> KConfigSourceGenerator.h:46
> +
> +// Those are fairly self contained functions.
> +void createHeaders();

Do they all need to be public?

> kconfig_compiler.cpp:745
>  
> -if (!codegenFilename.endsWith(QLatin1String(".kcfgc"))) {
> -cerr << "Codegen options file must have extension .kcfgc" << endl;
> -return 1;
> -}
> -QString baseName = QFileInfo(codegenFilename).fileName();
> -baseName = baseName.left(baseName.length() - 6);
> +KConfigXTParameters cfg = KConfigXTParameters(codegenFilename);
>  

it's shorter and simpler to write KConfigXTParameters cfg(codegenFilename);

> kconfig_compiler.cpp:753
>  
> -QFile input(inputFilename);
> +ParseResult parseResult = xmlParser.getParseResult();
>  

const ... ?

REPOSITORY
  R237 KConfig

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

To: tcanabrava, #frameworks, ervin, bport, dfaure
Cc: bport, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

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

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20FreeBSDQt5.13/264/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Sun, 12 Jan 2020 10:05:57 +
 Build duration:
9 min 47 sec and counting
   JUnit Tests
  Name: projectroot Failed: 4 test(s), Passed: 48 test(s), Skipped: 0 test(s), Total: 52 test(s)Failed: projectroot.autotests.kiocore_jobtestFailed: 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 FreeBSDQt5.13 - Build # 263 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20FreeBSDQt5.13/263/
 Project:
kf5-qt5 FreeBSDQt5.13
 Date of build:
Sun, 12 Jan 2020 09:56:46 +
 Build duration:
8 min 59 sec and counting
   JUnit Tests
  Name: projectroot Failed: 4 test(s), Passed: 48 test(s), Skipped: 0 test(s), Total: 52 test(s)Failed: projectroot.autotests.kiocore_jobtestFailed: 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)

D26588: Simplify usage of QMimeDatabase.

[David Faure]

dfaure closed this revision.

REPOSITORY
  R241 KIO

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

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