D6513: Add support for Attica tags support

[Laurent Montel]

mlaurent accepted this revision.

REPOSITORY
  R304 KNewStuff

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

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

D15213: Provide icons for 2x scaling

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  So if I apply this patch, use a 2x scale factor, and launch Dolphin (from 
18.08.2 or git master), I should see all line-art icons in my places panel, 
right? It works for 2x, and 1.5x, but at 1.7x and 1.3x I hit 
https://bugs.kde.org/show_bug.cgi?id=396990. But that's not your fault, so... 
approved!
  
  Does this mean that every new 16px, 22px, 24px, and 32px monochrome icon will 
need an `@2x` symlink?

REPOSITORY
  R266 Breeze Icons

BRANCH
  2xIcons

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

To: rizzitello, #vdg, #plasma, broulik, ngraham
Cc: ngraham, broulik, kde-frameworks-devel, michaelh, bruns

D8708: [WIP] Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau updated this revision to Diff 41078.
kossebau added a comment.


  Dump update with some more changes (still TODOs left to handle)

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8708?vs=40990=41078

BRANCH
  addAnnotationItemDelegate

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

AFFECTED FILES
  src/CMakeLists.txt
  src/include/CMakeLists.txt
  src/include/ktexteditor/abstractannotationitemdelegate.h
  src/include/ktexteditor/annotationinterface.h
  src/view/kateannotationitemdelegate.cpp
  src/view/kateannotationitemdelegate.h
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewhelpers.cpp
  src/view/kateviewhelpers.h
  src/view/kateviewinternal.h

To: kossebau
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D6513: Add support for Attica tags support

[Christoph Feck]

cfeck added a comment.


  @dfaure, @mlaurent okey?

REPOSITORY
  R304 KNewStuff

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

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

D6513: Add support for Attica tags support

[Christoph Feck]

cfeck accepted this revision.

REPOSITORY
  R304 KNewStuff

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

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

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

[Nathaniel Graham]

ngraham added a comment.


  Any progress on this, @emateli?

REPOSITORY
  R241 KIO

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

To: emateli, #frameworks, dfaure
Cc: asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, michaelh, bruns

D12545: Set focus on the filename line edit when a file is selected

[Nathaniel Graham]

ngraham added a comment.


  @anemeth, any updates on this? Do you need a hand with anything?

REPOSITORY
  R241 KIO

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

To: anemeth, #frameworks, #vdg, ngraham
Cc: kde-frameworks-devel, rkflx, ngraham, #frameworks, michaelh, bruns

KDE CI: Frameworks plasma-framework kf5-qt5 SUSEQt5.10 - Build # 208 - Still Unstable!

[CI System]

BUILD UNSTABLE
 Build URL
https://build.kde.org/job/Frameworks%20plasma-framework%20kf5-qt5%20SUSEQt5.10/208/
 Project:
Frameworks plasma-framework kf5-qt5 SUSEQt5.10
 Date of build:
Wed, 05 Sep 2018 19:23:17 +
 Build duration:
3 min 26 sec and counting
   JUnit Tests
  Name: (root) Failed: 6 test(s), Passed: 9 test(s), Skipped: 0 test(s), Total: 15 test(s)Failed: TestSuite.dialognativetestFailed: TestSuite.plasma-configmodeltestFailed: TestSuite.plasma-fallbackpackagetestFailed: TestSuite.plasma-iconitemtestFailed: TestSuite.plasma-packagestructuretestFailed: TestSuite.plasma-storagetest
   Cobertura Report
  
   Project Coverage Summary
  
   Name
  PackagesFilesClassesLinesConditionalsCobertura Coverage Report33%
(6/18)35%
(44/126)35%
(44/126)27%
(3599/13112)19%
(1821/9436)Coverage Breakdown by Package
Name
   FilesClassesLinesConditionalsautotests86%
(12/14)86%
(12/14)55%
(607/1113)29%
(313/1084)src.declarativeimports.calendar0%
(0/6)0%
(0/6)0%
(0/463)0%
(0/231)src.declarativeimports.core31%
(5/16)31%
(5/16)13%
(299/2217)7%
(96/1434)src.declarativeimports.plasmacomponents0%
(0/6)0%
(0/6)0%
(0/497)0%
(0/187)src.declarativeimports.plasmaextracomponents0%
(0/3)0%
(0/3)0%
(0/42)0%
(0/22)src.declarativeimports.platformcomponents0%
(0/3)0%
(0/3)0%
(0/58)0%
(0/14)src.declarativeimports.platformcomponents.utils0%
(0/2)0%
(0/2)0%
(0/14)0%
(0/2)src.plasma64%
(14/22)64%
(14/22)41%
(1420/3491)30%
(794/2633)src.plasma.packagestructure0%
(0/7)0%
(0/7)0%
(0/135)0%
(0/12)src.plasma.private47%
(9/19)47%
(9/19)43%
(663/1542)30%
(301/1003)src.plasma.scripting0%
(0/3)0%
(0/3)0%
(0/161)0%
(0/128)src.plasmapkg0%
(0/1)0%
(0/1)0%
(0/45)0%
(0/40)src.plasmaquick27%
(3/11)27%
(3/11)29%
(579/1976)18%
(312/1702)src.plasmaquick.private50%
(1/2)50%
(1/2)29%
(31/107)36%
(5/14)src.scriptengines.qml.plasmoid0%
(0/6)0%
(0/6)0%
(0/1096)0%
(0/906)tests.dpi0%
(0/2)0%
(0/2)0%
(0/21)0%
(0/2)tests.kplugins0%
(0/2)0%
  

D15071: Don't draw frames and shadows around images with transparency

[Nathaniel Graham]

ngraham closed this revision.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck, abetts
Cc: anthonyfieroni, markg, abetts, bruns, kde-frameworks-devel, michaelh, 
ngraham

D15180: kioexecd: watch for creations or modifications of the temporary files

[Elvis Angelaccio]

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

INLINE COMMENTS

> kioexecd.cpp:129
> +// check if the deleted (and not recreated) files where deleted 30s ago 
> or more
> +for (auto it = m_deleted.begin(); it != m_deleted.end();) {
> +if (it.value().msecsTo(currentDateTime) >= predefinedTimeout) {

Maybe `constBegin()`/`constEnd()` here?

REPOSITORY
  R241 KIO

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

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: bruns, anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, 
ngraham

D7010: KSqueezedTextLabel: call updateGeometry() when text changes

[Dominik Haumann]

dhaumann added a subscriber: rkflx.
dhaumann added a comment.


  @dfaure: writing "Henrik" when he already unsubscribeb will never reach him. 
Ping @rkflx

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

To: brauch, cfeck, dfaure
Cc: rkflx, dfaure, dhaumann, aacid, #frameworks, michaelh, ngraham, bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate updated this revision to Diff 41066.
jtamate added a comment.


  Use QDateTime::currentDateTimeUtc() instead of QDateTime::currentDateTime().

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15180?vs=41035=41066

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

AFFECTED FILES
  src/kioexec/kioexecd.cpp
  src/kioexec/kioexecd.h

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: bruns, anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, 
ngraham

D15074: [server] Do not try to create data offers without source

[Eike Hein]

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

REPOSITORY
  R127 KWayland

BRANCH
  offerWithSourceOnly

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

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

D15071: Don't draw frames and shadows around images with transparency

[Kai Uwe Broulik]

broulik accepted this revision.
broulik added a comment.


  Alright let's go with this

REPOSITORY
  R241 KIO

BRANCH
  thumbnail-frame-refinement (branched from master)

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

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck, abetts
Cc: anthonyfieroni, markg, abetts, bruns, kde-frameworks-devel, michaelh, 
ngraham

D15180: kioexecd: watch for creations or modifications of the temporary files

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> dfaure wrote in kioexecd.cpp:133
> Move the call to currentDateTime() outside of the loop, so it happens only 
> once.
> It's much more costly than one might think (because of timezone conversion, 
> which uses tzset() etc.)

makes me wonder, wouldn't it be better to use currentDateTimeUtc() throughout? 
We are not interested in absolute time here anyway.

REPOSITORY
  R241 KIO

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

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: bruns, anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, 
ngraham

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate added a comment.


  May I commit or should I wait for @elvisangelaccio to accept the changes? 
This time I have read the arc message, that is usually something about non 
tracked files:
  Revision 'D15180 : kioexecd: watch for 
creations or modifications of the temporary files' has not been accepted. 
Continue anyway? [y/N]
  
  > Even better use the std::remove_if/std::erase pattern, which avoids O(n^2) 
complexity during erase.
  > Or in this specific case, std::partition/{iterate over removed}/std::erase
  
  I assume QMap is like a std::map. I've seen in this cppreference page 
 that these algorithms 
cannot be used with associative containers such as std::set and std::map.

REPOSITORY
  R241 KIO

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

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: bruns, anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, 
ngraham

D15283: Move SMB KCM to Network Settings category

[Nathaniel Graham]

ngraham accepted this revision as: VDG.
ngraham added a comment.


  OK, for now let's move it to the proper place then.

REPOSITORY
  R241 KIO

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

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

D15283: Move SMB KCM to Network Settings category

[Kai Uwe Broulik]

broulik added a comment.


  I investigated once but the SMB kio slave actually still reads these 
properties. Why it doesn't work, I have no idea. SMB browsing is completely 
broken in newer distros thanks to switch to SMB >= 2 ..

REPOSITORY
  R241 KIO

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

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

D15283: Move SMB KCM to Network Settings category

[Nathaniel Graham]

ngraham added a comment.


  Makes sense, but can we just delete this KCM entirely instead? Apparently it 
doesn't, um, work. https://bugs.kde.org/show_bug.cgi?id=164283

REPOSITORY
  R241 KIO

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

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

D15240: Create a default wallet when user refuses to do so

[Nathaniel Graham]

ngraham added a comment.


  In D15240#320867 , @bruns wrote:
  
  > Can't we use pam_kwallet to receive the password and automatically create 
the wallet when the user logs in for the first time?
  >
  > Of course, some caveats apply, passwordless login, smart cards, trivial 
password ...
  
  
  If that's technically feasible, that would work too. However keep in mind 
that some distros like openSUSE deliberately do not ship with `pam_kwallet` 
(see https://bugzilla.suse.com/show_bug.cgi?id=1034347).
  
  Ultimately I don't have strong opinions on implementation, but I do think we 
should come up with some way to avoid presenting the user with a surprise 
wizard full of nerdy options.

REPOSITORY
  R311 KWallet

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

To: McPain, #frameworks, ngraham
Cc: bruns, ngraham, abetts, kde-frameworks-devel, michaelh

D15240: Create a default wallet when user refuses to do so

[Stefan Brüns]

bruns added a comment.


  > In D15240#320224 , @abetts wrote:
  > 
  >> I can see this argument as very valuable. I have also been a little 
startled by setting this up when you first start using Plasma or create your 
first password. I have not seen this in other systems. I know also we care for 
security and transparency. Maybe we should reconsider the wizard and use a more 
automated approach. Thoughts?
  > 
  > 
  > I would favor automatically creating a default wallet with the user's 
current password using a "good enough" cipher that we can hopefully all agree 
on. This would probably require changes to user-manager, or whatever it is that 
receives the string used for a new account's password. At the moment when a new 
user account is created, it would not only create the new user account, but it 
would also create a wallet using the same password.
  
  Can't we use pam_kwallet to receive the password and automatically create the 
wallet when the user logs in for the first time?
  
  Of course, some caveats apply, passwordless login, smart cards, trivial 
password ...

REPOSITORY
  R311 KWallet

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

To: McPain, #frameworks, ngraham
Cc: bruns, ngraham, abetts, kde-frameworks-devel, michaelh

D15180: kioexecd: watch for creations or modifications of the temporary files

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in kioexecd.cpp:134
> Also for loop should looks like:
> 
>   for (it = begin(); it != end();) {
>   if () {
>   it = erase(it);
>   } else {
>   ++it;
>   }
>   }

Even better use the std::remove_if/std::erase pattern, which avoids O(n^2) 
complexity during erase.
Or in this specific case, std::partition/{iterate over removed}/std::erase

REPOSITORY
  R241 KIO

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

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: bruns, anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, 
ngraham

D15071: Don't draw frames and shadows around images with transparency

[Andres Betts]

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

REPOSITORY
  R241 KIO

BRANCH
  thumbnail-frame-refinement (branched from master)

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

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck, abetts
Cc: anthonyfieroni, markg, abetts, bruns, kde-frameworks-devel, michaelh, 
ngraham

D15071: Don't draw frames and shadows around images with transparency

[Stefan Brüns]

bruns added a comment.


  +1 from me ...

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, #dolphin, #vdg, broulik, cfeck
Cc: anthonyfieroni, markg, abetts, bruns, kde-frameworks-devel, michaelh, 
ngraham

D15240: Create a default wallet when user refuses to do so

[Oleg Solovyov]

McPain added a comment.


  In D15240#320542 , @ngraham wrote:
  
  > I would favor automatically creating a default wallet with the user's 
current password using a "good enough" cipher that we can hopefully all agree 
on. This would probably require changes to user-manager, or whatever it is that 
receives the string used for a new account's password. At the moment when a new 
user account is created, it would not only create the new user account, but it 
would also create a wallet using the same password.
  
  
  What if we create a user in AD? (Active Directory or something similar, not 
local user)

REPOSITORY
  R311 KWallet

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

To: McPain, #frameworks, ngraham
Cc: ngraham, abetts, kde-frameworks-devel, michaelh, bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[David Faure]

dfaure accepted this revision.
dfaure added a comment.


  Thanks!

REPOSITORY
  R241 KIO

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

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15213: Provide icons for 2x scaling

[Kai Uwe Broulik]

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


  Alright, Dolphin patches merged to stable. This can go in now, thanks!

REPOSITORY
  R266 Breeze Icons

BRANCH
  2xIcons

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

To: rizzitello, #vdg, #plasma, broulik
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate updated this revision to Diff 41035.
jtamate added a comment.


  Tested, dirty is not signaled when created (at least I didn't saw the dialog 
for uploading the modified file).
  Removed the header.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15180?vs=41030=41035

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

AFFECTED FILES
  src/kioexec/kioexecd.cpp
  src/kioexec/kioexecd.h

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D6513: Add support for Attica tags support

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 41034.
leinir marked an inline comment as done.
leinir added a comment.


  More codestyle fixes. I also notice some which were there before this work, 
but fixing that seems distinctly out of scope for this patch...

REPOSITORY
  R304 KNewStuff

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6513?vs=40981=41034

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

AFFECTED FILES
  autotests/knewstuffentrytest.cpp
  src/attica/atticaprovider.cpp
  src/core/CMakeLists.txt
  src/core/engine.cpp
  src/core/engine.h
  src/core/entryinternal.cpp
  src/core/entryinternal.h
  src/core/provider.h
  src/core/tagsfilterchecker.cpp
  src/core/tagsfilterchecker.h
  src/staticxml/staticxmlprovider.cpp
  tests/CMakeLists.txt
  tests/khotnewstuff_test.cpp
  tests/khotnewstuff_test.knsrc.in
  tests/knewstuff2_test.cpp
  tests/knewstuff2_test.h
  tests/knewstuff2_test.knsrc
  tests/testdata/entry.xml
  tests/testdata/provider.xml

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

D6513: Add support for Attica tags support

[Dan Leinir Turthra Jensen]

leinir marked 16 inline comments as done.
leinir added a comment.


  i guess uncrustify isn't a magic bullet either, eh? ;) Thanks for the 
findings!

INLINE COMMENTS

> cfeck wrote in khotnewstuff_test.cpp:66
> Any rationale for using `fromLocal8Bit()` for fixed strings? If, for whatever 
> reason, you do not want to use QStringLiteral or QLatin1String, please use 
> fromUtf8(). This is what we ship for source files.

None apart from this being a modified version of an old test which used that 
function rather than the proper one. Fixed :)

REPOSITORY
  R304 KNewStuff

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

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

D15180: kioexecd: watch for creations or modifications of the temporary files

[David Faure]

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


  No worries, that's what code review is for :-)
  
  Now this is starting to look good ;)

INLINE COMMENTS

> kioexecd.cpp:88
> +
> +slotDirty(path);
> +}

Doesn't kdirwatch emit dirty when emitting created anyway?
(because historically, there was only "dirty" initially)

But I could be confusing watching files and watching directories, so better 
test rather than trusting me ;)

> kioexecd.h:28
>  #include 
> +#include 
> +#include 

remove

REPOSITORY
  R241 KIO

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

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate updated this revision to Diff 41030.
jtamate added a comment.


  Why the mutex? I interpreted that QTimer work as an interrumpt (wrong) 
instead of generating events in the event queue (right).
  So I've gone through all the possible mistakes one can do here:
  
  - Design mistakes
  - Security mistakes
  - Misunderstanding the API.
  
  Definitely, I need more vacation.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15180?vs=41026=41030

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

AFFECTED FILES
  src/kioexec/kioexecd.cpp
  src/kioexec/kioexecd.h

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[David Faure]

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

INLINE COMMENTS

> kioexecd.cpp:36
>  
> +const int predefinedTimeout = 3; // 30s
> +

static const int...

> kioexecd.cpp:86
> +{
> +m_deleted_mutex.lock();
> +m_deleted.remove(path);

Why is there a mutex at all, in this single-threaded code? This doesn't make 
sense to me.

> kioexecd.cpp:133
> +for (auto it = m_deleted.begin(); it != m_deleted.end();) {
> +if (it.value().msecsTo(QDateTime::currentDateTime()) >= 
> predefinedTimeout) {
> +qCDebug(KIOEXEC) << "Going to forget" << it.key();

Move the call to currentDateTime() outside of the loop, so it happens only once.
It's much more costly than one might think (because of timezone conversion, 
which uses tzset() etc.)

REPOSITORY
  R241 KIO

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

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate updated this revision to Diff 41026.
jtamate edited the summary of this revision.
jtamate edited the test plan for this revision.
jtamate added a comment.


  Do not delete recursively.
  Do not delete the file after 30s (we know it is already deleted).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15180?vs=40961=41026

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

AFFECTED FILES
  src/kioexec/kioexecd.cpp
  src/kioexec/kioexecd.h

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D7010: KSqueezedTextLabel: call updateGeometry() when text changes

[Sven Brauch]

brauch added a comment.


  For the record, I tried writing a test for this but didn't succeed and 
eventually put it aside, although the difference is easily visible in a test 
application. There must be a reason why the naive test case behaves differently 
from an interactive application ... I could take another look, I guess.

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

To: brauch, cfeck, dfaure
Cc: dfaure, dhaumann, aacid, #frameworks, michaelh, ngraham, bruns

D15283: Move SMB KCM to Network Settings category

[Kai Uwe Broulik]

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

REVISION SUMMARY
  The "Connectivity" category is empty and makes it show up on the root level 
as non-descript "Connectivity" entry

TEST PLAN
  Before
  F6236479: Screenshot_20180905_093102.png 

  After
  F6236480: Screenshot_20180905_093159.png 

  Every other KIO KCM is also in that category

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/kcms/kio/smb.desktop

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

D15180: kioexecd: watch for creations or modifications of the temporary files

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> jtamate wrote in kioexecd.cpp:65
> There is a slightly problem:  QStandardPaths::CacheLocation is application 
> dependent, and their values doesn't match here:
> kioexec: /home/jtorres/.cache/kioexec/
> kioexecd: /home/jtorres/.cache/kiod5/
> Can we assume that replacing kiod5 by kioexec will always work?
> 
> We could use QStandardPaths::GenericCacheLocation instead, but this is not 
> guaranteed to be non empty.
> 
> Or another solution: keep it as it was (delete only the file and the 
> directory if it is possible).

You can remove only file and then if dir is empty to delete it. Same in 
slotCheckDeletedFiles

REPOSITORY
  R241 KIO

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

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in kioexecd.cpp:65
> The problem with using `QDir::removeRecursively()` is that the folder we are 
> going to delete recursively is an input from dbus. What happens if some 
> malicious software calls `watch("~/dummy.txt")` ?
> 
> At the very least we need to check whether this folder starts with 
> `QStandardPaths::writableLocation(QStandardPaths::CacheLocation) + 
> QStringLiteral("/krun")` (the path used by `kioexec`).

There is a slightly problem:  QStandardPaths::CacheLocation is application 
dependent, and their values doesn't match here:
kioexec: /home/jtorres/.cache/kioexec/
kioexecd: /home/jtorres/.cache/kiod5/
Can we assume that replacing kiod5 by kioexec will always work?

We could use QStandardPaths::GenericCacheLocation instead, but this is not 
guaranteed to be non empty.

Or another solution: keep it as it was (delete only the file and the directory 
if it is possible).

> anthonyfieroni wrote in kioexecd.cpp:85-88
> Now it's not needed 'remove' will do the work

You're right. In this case it isn't possible to be notified of a file creation 
unless it has been deleted first.

REPOSITORY
  R241 KIO

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

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15240: Create a default wallet when user refuses to do so

[Oleg Solovyov]

McPain added a comment.


  In D15240#320542 , @ngraham wrote:
  
  > Stuff that doesn't work with KWallet should be fixed. But the point would 
be moot if we create a default wallet in a more user-friendly manner...
  
  
  I never told that there are stuff that doesn't work with KWallet
  I'm saying about stuff that doesn't work _without_ KWallet :)

REPOSITORY
  R311 KWallet

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

To: McPain, #frameworks, ngraham
Cc: ngraham, abetts, kde-frameworks-devel, michaelh, bruns