D21783: [WIP]Show more details in warning dialog shown before starting a privileged operation

[David Faure]

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


  I'm concerned that you didn't compile this (because of dependency issues, 
from what I gather), which means it's not tested either.
  
  I tried to compile it but it doesn't cleanly apply to git master. Can you 
rebase it?

INLINE COMMENTS

> slaveinterface.cpp:295
>  }
> +} else if (m.contains("privilege_conf_details")) { // KF6 TODO 
> Remove this conditional.
> +d->privilegeConfMetaData = m;

This fails to build for me...

slaveinterface.cpp:291:55: error: ‘QString::QString(const char*)’ is private 
within this context

It needs QStringLiteral like the previous one.

> dfaure wrote in file_unix.cpp:87
> no space before ':' in English

Not done

> dfaure wrote in file_unix.cpp:91
> same

same

> dfaure wrote in file_unix.cpp:121
> weird indentation

still there

REPOSITORY
  R241 KIO

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

To: chinmoyr, #vdg, #frameworks, dfaure
Cc: mreeves, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25793: Rename "Configure Shortcuts" to "Configure Keyboard Shortcuts"

[Luigi Toscano]

ltoscano added a comment.


  Oh sorry, I missed that. It's the kind of information I wouldn't expect in 
the commit message.

REPOSITORY
  R265 KConfigWidgets

BRANCH
  configure-keyboard-shortcuts (branched from master)

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

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

D25698: New class KApplicationTrader, to replace KMimeTypeTrader and KServiceTypeTrader

[David Faure]

dfaure added a comment.


  Feedback on the API question would be welcome.
  
  Also, see the discovery in T12256 . I'm 
thinking of renaming this class to KServiceTrader and add a queryByServiceType 
to it.

REPOSITORY
  R309 KService

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

To: dfaure, broulik, mart, vkrause, nicolasfella
Cc: dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25793: Rename "Configure Shortcuts" to "Configure Keyboard Shortcuts"

[Nathaniel Graham]

ngraham added a comment.


  Yep, in fact I mentioned this in the description section of the patch:
  
  > If accepted, will wait until after tagging to land it so as not to break 
the string freeze.

REPOSITORY
  R265 KConfigWidgets

BRANCH
  configure-keyboard-shortcuts (branched from master)

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

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

D25798: Deprecated allowAsDefault

[Nicolas Fella]

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

REVISION SUMMARY
  Execute upon T12309 . The KServiceOffer 
still takes a 'bool allowedAsDefault', that needs an overload, but in a 
different patch

TEST PLAN
  builds

REPOSITORY
  R309 KService

BRANCH
  allo

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

AFFECTED FILES
  src/CMakeLists.txt
  src/services/kservice.cpp
  src/services/kservice.h
  src/services/kserviceoffer.cpp
  src/services/kserviceoffer.h

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

D25793: Rename "Configure Shortcuts" to "Configure Keyboard Shortcuts"

[Luigi Toscano]

ltoscano added a comment.


  Please commit it after the commit for the new Frameworks is made (so probably 
from Sunday onwards).

REPOSITORY
  R265 KConfigWidgets

BRANCH
  configure-keyboard-shortcuts (branched from master)

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

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

D25798: Deprecated allowAsDefault

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> kservice.cpp:982
>  
> +#if KSERVICE_ENABLE_DEPRECATED_SINCE(5, 65)
>  bool KService::allowAsDefault() const

This should be BUILD, not ENABLE.

*BUILD* macros are controlled by the EXCLUDE_DEPRECATED_BEFORE_AND_AT value, 
which iis what is used at build time of the library itself, and then hardcoded 
with the installed headers.
*ENABLE* macros are controlled by KF_DISABLE_DEPRECATED_BEFORE_AND_AT value, 
which is used when building against the library, and can be controlled by the 
user of the library.
To avoid duplication, the *ENABLE* macros are also reused in the headers during 
the build time of the library itself, to not to have to write

  #if KSERVICE_ENABLE_DEPRECATED_SINCE(5, 65) AND 
KSERVICE_BUILD_DEPRECATED_SINCE(5, 65) 

While it works to use *ENABLE* also in the sources, like the reuse works in the 
headers, it is bad practice as it blurs the purposes of the ENABLE vs the BUILD 
macros, where the latter is only to be set at build time of the library itself. 
So to not give people wrong ideas, the BUILD macros should be used everywhere 
where only in the build of the library itself it is deciced via the 
EXCLUDE_DEPRECATED_BEFORE_AND_AT value whether code should be part of the 
created library.

> kserviceoffer.cpp:97
>  
> +#if KSERVICE_ENABLE_DEPRECATED_SINCE(5, 65)
>  bool KServiceOffer::allowAsDefault() const

KSERVICE_BUILD_DEPRECATED_SINCE

REPOSITORY
  R309 KService

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

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

D25682: [WIP] add initial wsdiscovery support

[Ben Cooksley]

bcooksley added a comment.


  With regards to the Docker/Gitlab CI part, please use the images under 
kdeorg/ on Dockerhub rather than personally maintained images as the wider 
community has no access to your namespace on Gitlab.com
  
  (Additionally please note that Fedora is not permitted to be used on our 
infrastructure)

REPOSITORY
  R320 KIO Extras

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

To: sitter, dfaure, #frameworks, #dolphin
Cc: bcooksley, ngraham, caspermeijn, davidedmundson, kde-frameworks-devel, 
kfm-devel, pberestov, iasensio, fprice, LeGast00n, MrPepe, fbampaloukas, 
alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, 
firef, andrebarros, bruns, emmanuelp, mikesomov

D25682: [WIP] add initial wsdiscovery support

[David Faure]

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


  I guess you were expecting a higher-level review, but I don't know anything 
about these protocols.
  
  I'm glad to see my KDSoap library being useful in KDE too though :-)

INLINE COMMENTS

> discovery.cpp:21
> +
> +#include "discovery.h"

Not sure this .cpp file serves any purpose right now ;)

But actually the virtual destructors could be implemented here out-of-line to 
avoid being generated in every user of the class. The `=default` syntax would 
work here too.

> discovery.h:2
> +/*
> +Copyright 2019 Harald Sitter  +

What happened to the '>' character?

[repeats]

> discovery.h:40
> +virtual ~Discovery() = default;
> +virtual void toEntry(KIO::UDSEntry ) = 0;
> +};

Why don't these `toEntry()` methods *return* a KIO::UDSEntry instead?
It's always empty on incoming anyway.

And UDSEntry supports moving so the "return" statement won't make a copy.

> discovery.h:51
> +virtual void stop() = 0;
> +virtual bool isFinished() = 0;
> +

const?

> dnssddiscoverer.cpp:46
> +//   that assumption will be true all the time. ~sitter, 2018
> +QUrl u(QStringLiteral("smb://"));
> +u.setHost(m_service->hostName());

`u.setScheme(QStringLiteral("smb"))` would be slightly faster; you're 
constructing a URL from its parts, no need to trigger parsing.

> dnssddiscoverer.cpp:71
> +// RemoteService::Ptr is useless here.
> +for (const auto  : m_services) {
> +if (*service == *it) {

qAsConst

> dnssddiscoverer.cpp:103
> +m_browser.disconnect();
> +for (auto service : m_services) {
> +service->resolve(); // Blocks until resolution happened. Our signal 
> handle then jumps in.

qAsConst

> dnssddiscoverer.h:41
> +
> +class DNSSDDiscoverer : public QObject, public virtual Discoverer
> +{

Why `virtual`? I thought this only mattered for diamond-shaped inheritance.

(OTOH I remember it leads to strange order of ctors being called, so I avoid it 
as much as possible)

> .gitlab-ci.yml:2
> +fedora:
> +image: 
> registry.gitlab.com/caspermeijn/docker-images/fedora-build-onvifviewer:latest
> +stage: build

(is this meant to go into kde git? just wondering)

> CMakeLists.txt.user:3
> +
> +
> +

Now this one I'm sure, should NOT go into git.

> CMakeLists.txt.user:205
> +
> + key="ProjectExplorer.BuildConfiguration.BuildDirectory">/home/me/src/git/soapy/build-kdsoap-ws-discovery-client-Desktop-Release-with-Debug-Information
> + key="ProjectExplorer.BuildConfiguration.BuildStepList.0">

... because it's about your config :)

> wsdiscoverytargetservice.cpp:38
> +
> +bool WSDiscoveryTargetService::isMatchingType(const KDQName )
> +{

const

(this will avoid detaching m_typeList)

> wsdiscoverytargetservice.cpp:48
> +
> +bool WSDiscoveryTargetService::isMatchingScope(const QUrl )
> +{

same

> kio_smb_browse.cpp:479
> +   if (normalizedUrl.path().isEmpty()) {
> +#pragma message "refactor this entire function into less of a long pile of 
> madness"
> +   qDebug() << "Trying modern discovery (dnssd/wsdiscovery)";

lol

> kio_smb_browse.cpp:494
> +   connect(, ::timeout, this, [&] {
> +   if (list.size() < 1) {
> +   return;

isEmpty() is more readable.

> kio_smb_browse.cpp:503
> +   auto enterDiscovery = [&](Discovery::Ptr discovery) {
> +   discovery->toEntry(udsentry);
> +   list.append(udsentry);

With my suggestion to return a udsentry, this whole lambda becomes

`list.append(discovery->toEntry())`

(another move, no copy)

> kio_smb_browse.cpp:511
> +
> +   QList discoverers {, };
> +

const QList to avoid detaching in range-for below

> kio_smb_browse.cpp:519
> +   }
> +   allFinished &= discoverer->isFinished();
> +   }

never use &= for booleans, it's not meant for that

(it won't work for '1' and '2' which are both 'true' for booleans)

`allFinished = allFinished && discoverer->isFinished()`

Unfortunately there's no &&= in C++.

> wsdiscoverer.cpp:116
> +qWarning() << response.faultAsString();
> +// No return! We'd disqualify systems that do not implement pbsd.
> +}

OK. But maybe an `else` then ?
Not much point in using response.childValues() on a fault, i.e. iterating over 
the fault details.

> wsdiscoverer.cpp:128
> +QString computer;
> +for (auto section : response.childValues()) {
> +computer = section

Needs a const local var to avoid a detach. Yes, range-for is annoying for Qt 
containers.

> wsdiscoverer.cpp:160
> +
> +const QHostAddress address(m_endpointUrl.toString());
> +qDebug() << "";

unused

> wsdiscoverer.cpp:209
> +// We do set a suitable timeout in the resolver so this doesn't take 
> forever.
> +for (auto future : m_futures) {
> +

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[David Faure]

dfaure added a comment.


  In D23384#571455 , @sitter wrote:
  
  > The opposite extreme is to always pass when X-KDE-Protocols is set and 
assume that the applications are actually working correctly (e.g. vlc ought to 
talk to kiod/KPasswdServerClient to get credentials, otherwise its declaration 
of X-KDE-Protocols is incorrect and you are looking at a bug in vlc. at the 
very least it should throw up its own auth dialog if it doesn't know what to 
do).
  
  
  There's a mistake in this reasoning. The "KDE" in X-KDE-Protocols only means 
that this is a KDE-specific field in desktop files, it wasn't standardized.
  It doesn't however mean that these protocols are implemented using KIO.
  In fact KIO-based apps normally just set X-KDE-Protocols=KIO. Other values 
for that field (i.e. an actual list of protocols) *is* what other apps are 
supposed to use there.
  
  But yes, this doesn't solve the credentials issue.
  I like the suggestion in another comment to implement that in 
SecretService... (which could replace kpasswdserver then, at least for the 
storage part, only the GUI would remain).

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

D21783: [WIP]Show more details in warning dialog shown before starting a privileged operation

[Chinmoy Ranjan Pradhan]

chinmoyr updated this revision to Diff 71041.
chinmoyr added a comment.


  Rebased, compiled and tested. Changes work as expected.
  Double checked file_unix.cpp. Would be a shame if it happens yet again.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21783?vs=71025=71041

BRANCH
  master

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

AFFECTED FILES
  autotests/kiotesthelper.h
  src/core/jobuidelegateextension.h
  src/core/slavebase.cpp
  src/core/slavebase.h
  src/core/slaveinterface.cpp
  src/core/slaveinterface_p.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/jobuidelegate.cpp
  src/widgets/jobuidelegate.h

To: chinmoyr, #vdg, #frameworks, dfaure
Cc: mreeves, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25340: Added background colors to active and inactive icon view

[Nathaniel Graham]

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


  I guess I should change my status to accepted given that I think this is good 
enough and already an improvement. But I think we can do even better, 
@niccolove. :)

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  backrgound-color-iconviewer (branched from master)

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

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

D25794: ArraySourceTest: Use QTEST_GUILESS_MAIN

[David Edmundson]

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

BRANCH
  master

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

To: heikobecker, #frameworks, ahiemstra, davidedmundson

D25795: Rename kf5quickcharts_example to kquickcharts_example

[David Edmundson]

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

BRANCH
  master

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

To: heikobecker, #frameworks, ahiemstra, davidedmundson

D25789: Correctly report if baloo_file is unavailable

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> monitor.cpp:67
>  m_balooRunning = false;
>  QDBusServiceWatcher* balooWatcher = new 
> QDBusServiceWatcher(m_scheduler->service(),
>  m_bus,

The watcher should be installed unconditionally, and first.

Checking state first and the instaling the watcher is racy.

REPOSITORY
  R293 Baloo

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

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

D25767: KAutoSaveFile: add a unit test to check max. filename length

[Dominik Haumann]

dhaumann added inline comments.

INLINE COMMENTS

> dfaure wrote in kautosavefiletest.cpp:98
> Let's hope none of the supported OSes have a smaller PATH_MAX :)
> FreeBSD, Windows, macOS, who knows what limits they have.

What I wonder is: PATH_MAX etc are #defines, right? Since on Windows there is a 
registry flag to enable much longer paths/file names. My intention is not to 
block this series of patches, but we might end up with restrictions in our code 
that do not exist on the filesystem at hand.

REPOSITORY
  R244 KCoreAddons

BRANCH
  l-kautosave-unittest (branched from master)

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

To: ahmadsamir, #frameworks, mpyne, dfaure
Cc: dhaumann, dfaure, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21783: [WIP]Show more details in warning dialog shown before starting a privileged operation

[David Faure]

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

INLINE COMMENTS

> slavebase.cpp:1525
> +
> +#ifndef KIOCORE_NO_DEPRECATED
> +PrivilegeOperationStatus SlaveBase::requestPrivilegeOperation()

`#if KIOCORE_BUILD_DEPRECATED_SINCE(5, 65) `

(notice BUILD, not ENABLE)

> slavebase.h:960
> +
> +#ifndef KIOCORE_NO_DEPRECATED
> +KIOCORE_DEPRECATED PrivilegeOperationStatus requestPrivilegeOperation();

The modern way is `#if KIOCORE_ENABLE_DEPRECATED_SINCE(5, 65)`

> slavebase.h:961
> +#ifndef KIOCORE_NO_DEPRECATED
> +KIOCORE_DEPRECATED PrivilegeOperationStatus requestPrivilegeOperation();
> +#endif

`KIOCORE_DEPRECATED_VERSION(5, 65, "Pass QString action to 
requestPrivilegeOperation")`

> slaveinterface.h:89
>  MSG_PRIVILEGE_EXEC,
> -MSG_SLAVE_STATUS_V2
> +MSG_SLAVE_STATUS_V2,
>  // add new ones here once a release is done, to avoid breaking binary 
> compatibility

unrelated/unneeded anymore

> slaveinterface_p.h:56
> +// Since 5.65 this is used for sending privilege operation details.
> +// KF6 TODO remove this hack.
> +MetaData privilegeConfMetaData;

Please explain how, for when the time comes (in case you're not around to do 
it).

> file_unix.cpp:84
> +QString action, detail;
> +switch(actionType) {
> +case CHMOD:

missing space after `switch` (same rule as `if`, `for` etc.)

> dfaure wrote in file_unix.cpp:81
> `const QVariantList &`

marked as done, but not done

> jobuidelegate.cpp:375
> +const QString details = 
> metaData.value(QStringLiteral("privilege_conf_details"));
> +result =  KMessageBox::warningContinueCancelDetailed(
> +  window(), text, caption, KStandardGuiItem::cont(), 
> KStandardGuiItem::cancel(), dontAskAgainName,

two spaces after `=`, remove one

> jobuidelegate.cpp:377
> +  window(), text, caption, KStandardGuiItem::cont(), 
> KStandardGuiItem::cancel(), dontAskAgainName,
> +  KMessageBox::Options(KMessageBox::Dangerous | 
> KMessageBox::WindowModal), details);
> +break;

I don't think you need the `KMessageBox::Options(` around the value?

> jobuidelegate.h:155
>const QString  = QString(),
> -  const KIO::MetaData  = 
> KIO::MetaData()) override;
> +  const KIO::MetaData  = KIO::MetaData()) 
> override; // KF6 TODO Add proper API
>  

what would be the proper API you have in mind?

REPOSITORY
  R241 KIO

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

To: chinmoyr, #vdg, #frameworks, dfaure
Cc: mreeves, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D25682: [WIP] add initial wsdiscovery support

[Casper Meijn]

caspermeijn added a comment.


  In D25682#570849 , @sitter wrote:
  
  > In D25682#570845 , 
@davidedmundson wrote:
  >
  > > Why do we need to mirror this dsoap-ws-discovery-client lib that seems to 
be copied from somewhere?
  >
  >
  > Testing convenience until @caspermeijn makes a release mostly. The library 
was,or maybe even still is, in flux as API was being shuffled up to kdsoap.
  
  
  Cool than my library can be used for this as well! The interface to KDSoap is 
in master now, so that only waits for a release of KDSoap. However I want to 
change some things in the kdsoap-ws-discovery-client API.
  
  Please be aware of the license of the WSDL files. It explicitly states "this 
document itself may not be modified in any way", so I am not sure what this 
will to with the license of the binary.

REPOSITORY
  R320 KIO Extras

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

To: sitter
Cc: ngraham, caspermeijn, davidedmundson, kde-frameworks-devel, kfm-devel, 
pberestov, iasensio, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, mikesomov

D25753: EBN extra-cmake-modules transport cleanup

[Christophe Giboudeaux]

cgiboudeaux added a comment.


  In D25753#572894 , @winterz wrote:
  
  > please send me a list of urls that don't have https:  and I'll add them to 
the whitelist
  
  
  The x86-64.org domain is dead. I'm not sure about what could be used instead.

REPOSITORY
  R240 Extra CMake Modules

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

To: jhayes, apol, cgiboudeaux
Cc: winterz, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
GB_2, bencreasy, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[Jean-Baptiste Mardelle]

mardelle added a comment.


  The unittest is in another diff because it's a different author I guess. I 
can process the comments but looking closer I found 2 other important issues in 
this class:
  
  1. When using KAutoSaveFile::staleFiles(filename) to check if there is an 
existing stale file for a document, we use the function 
KAutoSaveFile::extractManagedFilePath
  
  This function compares the orginal document path one reconstructed from the 
lock file name. However this logic seems flawed since the lock file name uses a 
trimmed path in some cases it is impossible to rebuild the original document's 
path from it.
  Instead I would propose to replace extractManagedFilePath with a new 
function, comparing filename, then paths like:
  
bool staleMatchesManaged(const QString& staleFileName, const QString 
managedFile)
{
const QStringRef sep = staleFileName.rightRef(3);
int sepPos = staleFileName.indexOf(sep);
if (QFileInfo(managedFile).fileName() != 
staleFileName.leftRef(sepPos).toString()) {
// File names don't match
return false;
}
int pathPos = staleFileName.indexOf(QChar::fromLatin1('_'), sepPos);
const QByteArray encodedPath = staleFileName.midRef(pathPos+1, 
staleFileName.length()-pathPos-1-KAutoSaveFilePrivate::NamePadding).toLatin1();
const QString sourcePathStart = QUrl::fromPercentEncoding(encodedPath);
return 
QFileInfo(managedFile).absolutePath().startsWith(sourcePathStart);
}
  
  
  
  2. The QLockFile class used in KAutoSaveFile creates a temporary file to 
manage locking:
  
  > QLockFile rmlock(d->fileName + QLatin1String(".rmlock"));
  
  https://github.com/qt/qtbase/blob/dev/src/corelib/io/qlockfile.cpp#L259
  
  This means that in our situation where we have a filename that already has 
the maximum length, trying to lock will always fail, preventing usage of the 
autosave file.
  Easier solution is probably to make maximum filename in KAutoSave 
File::tempFileName() a bit shorter.
  
  Thoughts? Should I prepare separate patches ? But changing the fileName 
encoding as suggested:
  
  > Simpler than toPercentEncoding: use QUrl::fileName(QUrl::FullyEncoded).
  
  Might break more the path recovery method extractManagedFilePath..

REPOSITORY
  R244 KCoreAddons

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

To: mardelle, #frameworks, dfaure, mpyne
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25149: Add a new template for KCMs

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 71009.
tcanabrava marked an inline comment as done.
tcanabrava added a comment.


  - Fix indentation

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25149?vs=69942=71009

BRANCH
  arcpatch-D25149

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

AFFECTED FILES
  templates/CMakeLists.txt
  templates/kcm-qml/%{APPNAMELC}settings.kcfg
  templates/kcm-qml/%{APPNAMELC}settings.kcfgc
  templates/kcm-qml/CMakeLists.txt
  templates/kcm-qml/Messages.sh
  templates/kcm-qml/README
  templates/kcm-qml/kcm-qml.kdevtemplate
  templates/kcm-qml/kcm.cpp
  templates/kcm-qml/kcm.h
  templates/kcm-qml/kcm_%{APPNAMELC}.desktop
  templates/kcm-qml/package/contents/ui/main.qml
  templates/kcm-qml/package/metadata.desktop

To: tcanabrava, #plasma, #frameworks, mart, ervin
Cc: #plasma, GB_2, yurchor, davidedmundson, ognarb, ervin, 
kde-frameworks-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25149: Add a new template for KCMs

[Tomaz Canabrava]

tcanabrava marked 6 inline comments as done.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: tcanabrava, #plasma, #frameworks, mart, ervin
Cc: #plasma, GB_2, yurchor, davidedmundson, ognarb, ervin, 
kde-frameworks-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25149: Add a new template for KCMs

[Tomaz Canabrava]

tcanabrava marked 2 inline comments as done.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: tcanabrava, #plasma, #frameworks, mart, ervin
Cc: #plasma, GB_2, yurchor, davidedmundson, ognarb, ervin, 
kde-frameworks-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25296: [RFC] Fix Display Configuration icon margins

[Nathaniel Graham]

ngraham added a subscriber: trickyricky26.
ngraham added a comment.


  @ndavis or @trickyricky26, could I ping you on this?

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D25682: [WIP] add initial wsdiscovery support

[Nathaniel Graham]

ngraham added reviewers: dfaure, Frameworks, Dolphin.

REPOSITORY
  R320 KIO Extras

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

To: sitter, dfaure, #frameworks, #dolphin
Cc: ngraham, caspermeijn, davidedmundson, kde-frameworks-devel, kfm-devel, 
pberestov, iasensio, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, mikesomov

D25753: EBN extra-cmake-modules transport cleanup

[John Hayes]

jhayes added a comment.


  The two links I know of are:
  http://0pointer.de/lennart/projects/libcanberra (works as is in file but
  wont work as https)
  http://www.x86-64.org/documentation/abi.pdf (http://www.x86-64.org is dead)

REPOSITORY
  R240 Extra CMake Modules

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

To: jhayes, apol, cgiboudeaux
Cc: winterz, cgiboudeaux, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
GB_2, bencreasy, michaelh, ngraham, bruns

D25296: [RFC] Fix Display Configuration icon margins

[Noah Davis]

ndavis added a comment.


  In D25296#563291 , @ngraham wrote:
  
  > Never mind, I wasn't deleting the cache files properly after rebuilding. 
When I do that, the monochrome icons don't get used at all and it reverts to 
the colorful one:
  >  F778: Screenshot_20191116_084222.png 

  
  
  The color vs monochrome issue strikes again :(
  
  Maybe remove the size prefix from the 32px version? If that doesn't work, 
I've got nothing.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D25788: Initialise QML monitor values

[David Edmundson]

davidedmundson created this revision.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
davidedmundson requested review of this revision.

REVISION SUMMARY
  m_indexerState would not be initialised and m_totalFiles would be
  garbage values if baloo_file wasn't running. We want to initialise
  everything.

TEST PLAN
  Compiles

REPOSITORY
  R293 Baloo

BRANCH
  master

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

AFFECTED FILES
  src/qml/experimental/monitor.cpp
  src/qml/experimental/monitor.h

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

D25789: Correctly report if baloo_file is unavailable

[David Edmundson]

davidedmundson created this revision.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
davidedmundson requested review of this revision.

REVISION SUMMARY
  Add a new state. Watch for service unregistration as well as
  registration.

REPOSITORY
  R293 Baloo

BRANCH
  master

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

AFFECTED FILES
  src/file/indexerstate.h
  src/qml/experimental/monitor.cpp
  src/qml/experimental/monitor.h

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

D24489: KAutosaveFile not respecting maximum filename length

[Ahmad Samir]

ahmadsamir added a comment.


  In D24489#573152 , @mardelle wrote:
  
  > The unittest is in another diff because it's a different author I guess.
  
  
  [...]
  I didn't mean to step on your toes; feel free to disregard the unit test I 
wrote, or reuse whatever part(s) of it as you see fit. It's just that I was 
interested in the matter and sort of wanted to see if my understanding was 
correct. :)

REPOSITORY
  R244 KCoreAddons

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

To: mardelle, #frameworks, dfaure, mpyne
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21783: [WIP]Show more details in warning dialog shown before starting a privileged operation

[Chinmoy Ranjan Pradhan]

chinmoyr updated this revision to Diff 71025.
chinmoyr marked 10 inline comments as done.
chinmoyr added a comment.


  Addressed the issues.
  Annotated code to be removed in KF6.
  My build env is outdated so I couldn't compile the new depracation macro. 
Copied as it is.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21783?vs=70914=71025

BRANCH
  arcpatch-D21783

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

AFFECTED FILES
  autotests/kiotesthelper.h
  src/core/jobuidelegateextension.h
  src/core/slavebase.cpp
  src/core/slavebase.h
  src/core/slaveinterface.cpp
  src/core/slaveinterface_p.h
  src/ioslaves/file/file_unix.cpp
  src/widgets/jobuidelegate.cpp
  src/widgets/jobuidelegate.h

To: chinmoyr, #vdg, #frameworks, dfaure
Cc: mreeves, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D21783: [WIP]Show more details in warning dialog shown before starting a privileged operation

[Chinmoy Ranjan Pradhan]

chinmoyr added inline comments.

INLINE COMMENTS

> dfaure wrote in slaveinterface_p.h:56
> Please explain how, for when the time comes (in case you're not around to do 
> it).

Removing each of its occurence (of which there are 3) and the surrounding `if`. 
 I will add comments where its needed to be removed.

> dfaure wrote in jobuidelegate.h:155
> what would be the proper API you have in mind?

I think adding a QString parameter for operation details should be sufficient. 
Then the API should be kept in sync with SlaveBase, SalveInterface, and 
JobUiDelegateExtension.

REPOSITORY
  R241 KIO

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

To: chinmoyr, #vdg, #frameworks, dfaure
Cc: mreeves, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

T10262: Integrate KIO Slaves into file system using FUSE gateway

[Nathaniel Graham]

ngraham closed this task as "Resolved".
ngraham added a comment.


  This is currently in progress and I don't think we need this task open 
anymore. The patches are scattered across various bits of infrastructure so it 
won't be useful for linking them here.

TASK DETAIL
  https://phabricator.kde.org/T10262

To: cfeck, ngraham
Cc: chinmoyr, fvogt, feverfew, #frameworks, ngraham, #dolphin, cfeck, 
pberestov, iasensio, fprice, MrPepe, fbampaloukas, alexde, Codezela, meven, 
spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

D25789: Correctly report if baloo_file is unavailable

[Nathaniel Graham]

ngraham added a comment.


  Maybe instead of "Unavailable", it could say "Not Running"?

REPOSITORY
  R293 Baloo

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

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

D25788: Initialise QML monitor values

[Nathaniel Graham]

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

REPOSITORY
  R293 Baloo

BRANCH
  master

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

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

D24489: KAutosaveFile not respecting maximum filename length

[Jean-Baptiste Mardelle]

mardelle added a comment.


  Oh no don't misunderstand me. I am very glad that someone else stepped up! I 
have more than enough work on my plate with Kdenlive so please help :)
  Hopefully David can give us some hints about the best way to move on!

REPOSITORY
  R244 KCoreAddons

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

To: mardelle, #frameworks, dfaure, mpyne
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25340: Added background colors to active and inactive icon view

[Nathaniel Graham]

ngraham added a comment.


  Can we push this forward? I just triaged a bunch of bugs and found that 
https://bugs.kde.org/show_bug.cgi?id=370465 now has five duplicates. There 
seems to be quite a bit of demand for this.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D25791: Fix writeFlags with KConfigCompilerSignallingItem

[David Edmundson]

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

REVISION SUMMARY
  KConfigCompilerSignallingItem both inherits KConfigSkeletonItem and
  internally is powered by a separate KConfigSkeletonItem
  
  The generated code calls setWriteFlags on the outer
  KConfigCompilerSignallingItem instance, but the real writing is
  performed by the internal version. We need to set the flags in the right
  place.
  
  Ideally we would do this in an overload of KConfigSkeletonItem, but
  given we can't, I've shadowed the method. This isn't pretty, but given
  the docs say it should generally only be used from auto generated code,
  should be fine.

TEST PLAN
  Used in workspace KCM

REPOSITORY
  R237 KConfig

BRANCH
  master

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

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

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

D25791: Fix writeFlags with KConfigCompilerSignallingItem

[David Edmundson]

davidedmundson added a dependent revision: D25792: Add notifiers to workspace 
options kcfg.

REPOSITORY
  R237 KConfig

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

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

D25793: Rename "Configure Shortcuts" to "Configure Keyboard Shortcuts"

[Nathaniel Graham]

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

REVISION SUMMARY
  This is a bit clearer, and reinforces the keyboard icon used for the menu 
item.
  
  BUG: 39488
  FIXED-IN: 5.66
  
  If accepted, will wait until after tagging to land it so as not to break the 
string freeze.

TEST PLAN
  F7802662: Configure keyboard shortcuts.png 


REPOSITORY
  R265 KConfigWidgets

BRANCH
  configure-keyboard-shortcuts (branched from master)

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

AFFECTED FILES
  src/kstandardaction_p.h

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

D25793: Rename "Configure Shortcuts" to "Configure Keyboard Shortcuts"

[Noah Davis]

ndavis added a comment.


  This doesn't seem wrong, but why is it needed? Do people get confused about 
the type of shortcuts? Are there non-keyboard shortcuts? If there are, would we 
put their configuration menu under a different menu option?

REPOSITORY
  R265 KConfigWidgets

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

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

D25793: Rename "Configure Shortcuts" to "Configure Keyboard Shortcuts"

[Nathaniel Graham]

ngraham added a comment.


  There was a bug report about it that had some people agreeing with it. I 
think it makes a bit of sense because yes, this dialog is indeed only about 
keyboard shortcuts, and at least to my ears, the phrase "keyboard shortcuts" 
instantly connotes what this is about, while "shortcuts" is a more generic term 
that doesn't have such an instantly recognizable computer-related meaning.

REPOSITORY
  R265 KConfigWidgets

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

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

D25793: Rename "Configure Shortcuts" to "Configure Keyboard Shortcuts"

[Noah Davis]

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


  Welp, there's nothing objectively wrong with making this patch. LGTM

REPOSITORY
  R265 KConfigWidgets

BRANCH
  configure-keyboard-shortcuts (branched from master)

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

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

D25340: Added background colors to active and inactive icon view

[Filip Fila]

filipf added a comment.


  In D25340#563400 , @ndavis wrote:
  
  > This diff is against commit 467d721cc96258b54048c0dd1508d16e03c0cd55, which 
isn't in git master. Do I actually need that commit for this patch to work?
  
  
  A similar issue still exists, but it does apply against master.
  
  Gave this is a quick spin and it definitely looks like an improvement.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D25794: ArraySourceTest: Use QTEST_GUILESS_MAIN

[Heiko Becker]

heikobecker created this revision.
heikobecker added reviewers: Frameworks, ahiemstra.
heikobecker requested review of this revision.

REVISION SUMMARY
  Allows the test to run without a display server.

TEST PLAN
  Builds, test still passes

BRANCH
  master

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

AFFECTED FILES
  autotests/ArraySourceTest.cpp

To: heikobecker, #frameworks, ahiemstra

D25795: Rename kf5quickcharts_example to kquickcharts_example

[Heiko Becker]

heikobecker created this revision.
heikobecker added reviewers: Frameworks, ahiemstra.
heikobecker requested review of this revision.

REVISION SUMMARY
  Matching the project name.

TEST PLAN
  exmaple is installed under the new name, runs fine

BRANCH
  master

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

AFFECTED FILES
  examples/charts/CMakeLists.txt

To: heikobecker, #frameworks, ahiemstra

D24489: KAutosaveFile not respecting maximum filename length

[David Faure]

dfaure added a comment.


  I see. This answers my question about why two merge requests -- no problem, 
keep them separate, but commit the fix before the unittest
  [this is so that bisecting never ends up in the situation where unittests are 
broken]
  
  You can also ignore my suggestion about fileName(QUrl::FullyEncoded) if 
you're afraid it won't be symmetrical, no problem.
  
  The rest of mardelle's comment looks sensible to me as well.

REPOSITORY
  R244 KCoreAddons

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

To: mardelle, #frameworks, dfaure, mpyne
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns