D18653: [PreviewJob] Also pass along that we're the thumbnailer when stat'ing file

[Kai Uwe Broulik]

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

REVISION SUMMARY
  Enables the KIO to react differently when being stat'd by the thumbnail KIO 
slave
  
  CCBUG: 234754

TEST PLAN
  In preparation to fix font thumbnails in `fonts:/`

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/widgets/previewjob.cpp

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

D18612: Cache the default KColorScheme configuration

[Kai Uwe Broulik]

broulik added a comment.


  I also noticed that on application startup two default color schemes are 
created: One by plasma-integration set on `QApplication` and the other by 
`QStyle::standardPalette()` in the widget style.
  I couldn't figure out a way to avoid this (have QStyle use the default app 
palette set by p-i or something without dirty hacks like using dynamic 
properties to communicate that we already have created the proper palette 
elsewhere). Perhaps this is something you could also profile/investigate :)

REPOSITORY
  R265 KConfigWidgets

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

To: mwolff, #kate, #kdevelop, dfaure
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D18630: Fix warning

[Kai Uwe Broulik]

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

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

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

D18499: [KCM Controls GridView] Add remove animation

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R296:d8505b807d55: [KCM Controls GridView] Add remove 
animation (authored by broulik).

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18499?vs=50188&id=50190

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

AFFECTED FILES
  src/qmlcontrols/kcmcontrols/qml/GridView.qml

To: broulik, #plasma, mart, davidedmundson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D18499: [KCM Controls GridView] Add remove animation

[Kai Uwe Broulik]

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

REVISION SUMMARY
  This is what all the grid KCMs have been doing, so it makes sense to put it 
into the component for consistency and maintainability.
  It makes for a much nicer user experience when the view has items that can be 
deleted.

TEST PLAN
  Animation looks exactly the same as it did before in the KCMs but allows us 
to remove 15 lines of boilerplate code from each grid kcm in plasma-desktop

REPOSITORY
  R296 KDeclarative

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

AFFECTED FILES
  src/qmlcontrols/kcmcontrols/qml/GridView.qml

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

D18487: Reparse background contrast settings when colors changed

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> theme_p.cpp:754
>  }
>  backgroundContrast = cg.readEntry("contrast", _contrast);
>  backgroundIntensity = cg.readEntry("intensity", _intensity);

An alternative approach would be to store those values and then do the default 
fallback and `qGray(color(Plasma::Theme::BackgroundColor).rgb()) < 127` stuff 
on the fly on demand.
It's runtime cpu cycles vs parsing a config file once on color change (with 
some awful copy paste)

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D18487: Reparse background contrast settings when colors changed

[Kai Uwe Broulik]

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

REVISION SUMMARY
  When switching between light and dark system colors, the background contrast 
might change which we didn't do at runtime leading to washed out Plasma popup 
backgrounds.
  Also, while at it, update complimentary colorscheme as well, which was 
forgotten here.
  
  BUG: 401142

TEST PLAN
  - Used Breeze Plasma theme (the one that follows system colors), switched 
from Breeze light color scheme to Breeze dark, popups immediately looked as 
they should have rather than only after a plasmashell restart

REPOSITORY
  R242 Plasma Framework (Library)

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

AFFECTED FILES
  src/plasma/private/theme_p.cpp

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

Re: Power Management and Inhibition by Applications

[Kai Uwe Broulik]

Hi,


I had thought that would only inhibit the screensaver instead of automatic
sleep.


KScreenLocker (previously KSMServer) that owns the 
org.freedesktop.ScreenSaver interface tells PowerDevil to keep the 
screen on when screensaver is inhibited. It makes no sense to prevent 
the screen from locking (ie. keep the desktop visible) but then allow 
the screen to turn off, having the same effect: hiding the desktop.



I did a quick test and in case I inhibit through org.freedesktop.ScreenSaver
interface, the battery applet does not indicate any inhibit whereas through
org.freedesktop.PowerManagement.Inhibit indicates that an inhibition is valid.


It should and it does here. Note that PowerDevil only enforces 
inhibitions after five seconds to prevent short transient inhibitions 
(e.g. web browsers block suspend when playing audio which could also 
lead to short notification sounds prolonging the time until suspend)



I wonder if I should not instead make usage of the Inhibit mechanism from
logind coupled with powerdevil. I am not sure if this is desired.


It is and I would love that, I just haven't had the time to implement it.

Also I'm a bit tired of adding yet another inhibition interface (we 
already have like three or four of them) of the "but this time, I 
promise, it will be perfect!!" kind. Last time I checked logind didn't 
offer signals for when an inhibition was added/removed, so the "xyz is 
curently blocking PM" in Battery Monitor might not be possible anymore 
this way.


Cheers
Kai Uwe

D18450: Add extractor for AppImage files

[Kai Uwe Broulik]

broulik added a comment.


  Cool!

REPOSITORY
  R286 KFileMetaData

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

To: kossebau, #baloo
Cc: broulik, kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18425: [KUrlNavigatorPlacesSelector] Properly identify teardown action

[Kai Uwe Broulik]

broulik planned changes to this revision.
broulik added a comment.


  There seems to be one issue that when you open a mounted drive directly, you 
get an "Unmount" entry at the top.
  Likely because the places are populated after the entry was created.
  Perhaps the remove codepath needs to iterate all actions and check rather 
than assuming it's always the last place.

REPOSITORY
  R241 KIO

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

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

D18425: [KUrlNavigatorPlacesSelector] Properly identify teardown action

[Kai Uwe Broulik]

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

REVISION SUMMARY
  Rather than relying on the model count (which broke when we added places 
categories), just identify the action by the data it is given.
  
  BUG: 403454

TEST PLAN
  - Hid Places panel in Dolphin and navigated around some mounts: Only one 
Unmount entry that came and went as it should
  - Unmount still works

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/filewidgets/kurlnavigatorplacesselector.cpp

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

D18356: [Plasma Theme] Use new connect syntax

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:8c5099140751: [Plasma Theme] Use new connect syntax 
(authored by broulik).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18356?vs=49804&id=49805

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

AFFECTED FILES
  src/plasma/theme.cpp

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

D18356: [Plasma Theme] Use new connect syntax

[Kai Uwe Broulik]

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

REVISION SUMMARY
  Saves some cycles

TEST PLAN
  Saves like 0.6ms on startup here, given the amount of `Theme` objects created.
  `ThemePrivate` is also a `QObject`, I don't understand why it checks for 
`qApp` or why it's not done in the `Private`'s constructor though..

REPOSITORY
  R242 Plasma Framework (Library)

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

AFFECTED FILES
  src/plasma/theme.cpp

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

D18149: Share Plasma::Theme instances between multiple ColorScope

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:eac69e04690b: Share Plasma::Theme instances between 
multiple ColorScope (authored by broulik).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18149?vs=49537&id=49803

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

AFFECTED FILES
  src/declarativeimports/core/colorscope.cpp
  src/declarativeimports/core/colorscope.h

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

Re: KDE file dialog column resize no longer possible?

[Kai Uwe Broulik]

Has resizing support been turned off in KF5 maybe, and if so, where and why?


Yes, to ensure sane default sizing of the columns, especially as you 
resize the dialog, which were often too narrow or too wide or otherwise 
unfitting.


Cheers
Kai Uwe

D18308: Ignore all non-storage deviceAdded signals from Solid

[Kai Uwe Broulik]

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

INLINE COMMENTS

> storagedevices.cpp:79
>  Entry entry(dev);
>  if (dev.udi().isEmpty())
>  return nullptr;

I think you can also move this check up

REPOSITORY
  R293 Baloo

BRANCH
  submit

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

To: bruns, #baloo, #frameworks, ngraham, broulik
Cc: broulik, kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D18299: ComboBox: fix default delegate

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> ComboBox.qml:58
> +controlRoot.currentIndex = index;
> +controlRoot.popup.visible = false;
> +}

Are you sure this is needed? Qt docs say for that using `ItemDelegate` for a 
`ComboBox` is recommended as:
"This ensures that the interaction works as expected, and the popup will 
automatically close when appropriate. "
The `currentIndex` change makes sense, though

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

To: apol, #frameworks, davidedmundson
Cc: broulik, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D18149: Share Plasma::Theme instances between multiple ColorScope

[Kai Uwe Broulik]

broulik updated this revision to Diff 49537.
broulik retitled this revision from "Share Plasma::Theme instances between 
multiple Svg and ColorScope" to "Share Plasma::Theme instances between multiple 
ColorScope".
broulik edited the test plan for this revision.
broulik added a comment.


  Focus on `ColorScope` for now, `Svg` is a tad more difficult.
  
  - Use `QSharedPointer`

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18149?vs=49139&id=49537

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

AFFECTED FILES
  src/declarativeimports/core/colorscope.cpp
  src/declarativeimports/core/colorscope.h

To: broulik, #plasma
Cc: apol, anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns

D18149: Share Plasma::Theme instances between multiple Svg and ColorScope

[Kai Uwe Broulik]

broulik added a comment.


  > And QSharedDataPointer?
  
  For that `Theme` would need to be `QSharedData`, no?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: broulik, #plasma
Cc: apol, anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns

D14661: Force reevaluation of Predicates if interfaces are removed

[Kai Uwe Broulik]

broulik added a comment.


  So, looks like adding the device causes us to probe it and that prompts 
UDisks to close the tray for examination? I lack a CD drive, so I cannot try 
this. Can you perhaps investigate this issue? :)
  
  Easiest could be to check for device type and then in doubt just don't do the 
remove-add-dance in case it's an optical drive.

REPOSITORY
  R245 Solid

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

To: bruns, #frameworks, broulik, ngraham, apol
Cc: volkov, apol, anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns

D18263: [CopyJob] Provide more descriptive notification header

[Kai Uwe Broulik]

broulik added a comment.


  Isn't that done by `emitCopying` et al?

REPOSITORY
  R241 KIO

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

To: chinmoyr, dfaure, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17851: Add Android notification backend

[Kai Uwe Broulik]

broulik added a comment.


  Pretty cool!

INLINE COMMENTS

> notifybyandroid.cpp:79
> +{
> +s_instance = this;
> +#if __ANDROID_API__ >= 23

Can there be multiple instances of this `NotifyByAndroid`?

> notifybyandroid.cpp:125
> +pixmap.save(&buffer, "PNG");
> +auto jIconData = env->NewByteArray(iconData.length());
> +env->SetByteArrayRegion(jIconData, 0, iconData.length(), 
> reinterpret_cast(iconData.constData()));

Does this need a `FreeLocalRef` call?

REPOSITORY
  R289 KNotifications

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

To: vkrause, apol
Cc: broulik, apol, nicolasfella, kde-frameworks-devel, michaelh, ngraham, bruns

D18149: Share Plasma::Theme instances between multiple Svg and ColorScope

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> apol wrote in colorscope.h:134
> How about using a QSharedPointer?

Complained with "QSharedPointer: cannot create a QSharedPointer from a 
QObject-tracking QWeakPointer"

REPOSITORY
  R242 Plasma Framework (Library)

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

To: broulik, #plasma
Cc: apol, anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns

D17528: Refactor SlaveInterface::calcSpeed

[Kai Uwe Broulik]

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


  We're getting some `SIGFPE` crash reports in this area: Bug 402665
  I failed to reproduce them, though. Best is probably to revert this patch 
because Frameworks release is imminent.

INLINE COMMENTS

> slaveinterface.cpp:113
> +const SlaveInterfacePrivate::TransferInfo last = {elapsed_time, 
> (d->filesize - d->offset)};
> +KIO::filesize_t lspeed = 1000 * (last.size - first.size) / 
> (last.time - first.time);
>  if (!lspeed) {

We likely get a division by zero here for some reason

REPOSITORY
  R241 KIO

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

To: chinmoyr, dfaure
Cc: broulik, bruns, kde-frameworks-devel, michaelh, ngraham

D18149: Share Plasma::Theme instances between multiple Svg and ColorScope

[Kai Uwe Broulik]

broulik added a comment.


  > Now in multi-thread environment will have problems especially on 
actualTheme(), no?
  
  `ColorScope` is QML-only and should be fine? Not sure about `Svg`, it never 
mentioned being thread-safe.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D17543: [Dialog] Don't alter mainItem's visibility

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:b257029afc3b: [Dialog] Don't alter mainItem's 
visibility (authored by broulik).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D17543?vs=47458&id=49147#toc

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17543?vs=47458&id=49147

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

AFFECTED FILES
  src/plasmaquick/dialog.cpp

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

D17542: Reset parentItem when mainItem changes

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:f4bd4a613187: Reset parentItem when mainItem changes 
(authored by broulik).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17542?vs=47457&id=49146

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

AFFECTED FILES
  src/plasmaquick/dialog.cpp

To: broulik, #plasma, mart
Cc: davidedmundson, kde-frameworks-devel, michaelh, ngraham, bruns

D18149: Share Plasma::Theme instances between multiple Svg and ColorScope

[Kai Uwe Broulik]

broulik edited the test plan for this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D18149: Share Plasma::Theme instances between multiple Svg and ColorScope

[Kai Uwe Broulik]

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

REVISION SUMMARY
  Especially since the `Theme` isn't modified in any way but just used to read 
some data.
  While the `Private` part of `Theme` is already shared, creating a `Theme` 
instance still has some setup cost.

TEST PLAN
  Noticed in GammaRay there was a tonne of `Theme` objects.
  Just the usage in `Svg` alone accounted for 5ms startup time, and 
`ColorScope` is also widely used.
  After this patch there's merely 25 `Theme` objects being created.
  Wanted to use `QSharedPointer` for this but failed..
  Doesn't reduce refcount and cleanup when you assign a different theme using 
`Svg::setTheme` but this is hardly used anyway.

REPOSITORY
  R242 Plasma Framework (Library)

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

AFFECTED FILES
  src/declarativeimports/core/colorscope.cpp
  src/declarativeimports/core/colorscope.h
  src/plasma/private/svg_p.h
  src/plasma/svg.cpp

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

D17681: RFC: Cache translated strings in Job::emit functions

[Kai Uwe Broulik]

broulik added a comment.


  > That 6% goes to 0% i guess?
  
  Pretty much, goes down to 0.07%

REPOSITORY
  R241 KIO

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

To: broulik, davidedmundson, dfaure, chinmoyr
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D17681: RFC: Cache translated strings in Job::emit functions

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:a48a277a8296: Cache translated strings in Job::emit 
functions (authored by broulik).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17681?vs=47828&id=47874

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

AFFECTED FILES
  src/core/job.cpp

To: broulik, davidedmundson, dfaure, chinmoyr
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D17680: Port connect of subjob in copyNextFile to new connect syntax

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:a4905c5994a7: Port connect of subjob in copyNextFile to 
new connect syntax (authored by broulik).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17680?vs=47827&id=47873

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

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

To: broulik, davidedmundson, dfaure, chinmoyr
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17678: Use qobject_cast instead of dynamic_cast

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:1d8e4e6fa29c: Use qobject_cast instead of dynamic_cast 
(authored by broulik).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17678?vs=47824&id=47834

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

AFFECTED FILES
  src/core/copyjob.cpp
  src/core/job.cpp

To: broulik, davidedmundson, dfaure, chinmoyr
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17681: RFC: Cache translated strings in Job::emit functions

[Kai Uwe Broulik]

broulik edited the test plan for this revision.

REPOSITORY
  R241 KIO

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

To: broulik, davidedmundson, dfaure, chinmoyr
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17680: Port connect of subjob in copyNextFile to new connect syntax

[Kai Uwe Broulik]

broulik edited the test plan for this revision.

REPOSITORY
  R241 KIO

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

To: broulik, davidedmundson, dfaure, chinmoyr
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17681: RFC: Cache translated strings in Job::emit functions

[Kai Uwe Broulik]

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

REVISION SUMMARY
  These are called repeatedly for every single file processed and we end up 
spending non-trivial amounts of time doing translation work.

TEST PLAN
  Not sure about implications on thread-safety and we don't evcit those strings 
anymore, but they're not that many..

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/core/job.cpp

To: broulik, davidedmundson, dfaure, chinmoyr
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17680: Port connect of subjob in copyNextFile to new connect syntax

[Kai Uwe Broulik]

broulik edited the test plan for this revision.

REPOSITORY
  R241 KIO

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

To: broulik, davidedmundson, dfaure, chinmoyr
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17680: Port connect of subjob in copyNextFile to new connect syntax

[Kai Uwe Broulik]

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

TEST PLAN
  Copied files, total and processed size still correct

REPOSITORY
  R241 KIO

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

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

To: broulik, davidedmundson, dfaure, chinmoyr
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17678: Use qobject_cast instead of dynamic_cast

[Kai Uwe Broulik]

broulik edited the test plan for this revision.

REPOSITORY
  R241 KIO

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

To: broulik, davidedmundson, dfaure, chinmoyr
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17678: Use qobject_cast instead of dynamic_cast

[Kai Uwe Broulik]

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

REVISION SUMMARY
  Since `KJob` is a `QObject` we can use the more efficient `qobject_cast` here

TEST PLAN
  Copied 1000 files, saved half a millisecond

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/core/copyjob.cpp
  src/core/job.cpp

To: broulik, davidedmundson, dfaure, chinmoyr
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D16643: Correct the accept flag of the event object on DragMove

[Kai Uwe Broulik]

broulik added a comment.


  I'm not blaming anyone, I'm merely stating that there is a potential issue 
with this patch that might not have been addressed and needs @bruns further 
input.

REPOSITORY
  R296 KDeclarative

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

To: trmdi, mart, broulik, #plasma, hein, davidedmundson
Cc: anthonyfieroni, bruns, dkorth, ngraham, kde-frameworks-devel, michaelh

D16643: Correct the accept flag of the event object on DragMove

[Kai Uwe Broulik]

broulik added a comment.


  Only if that potential issue with inhibition has been addressed

REPOSITORY
  R296 KDeclarative

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

To: trmdi, mart, broulik, #plasma, hein, davidedmundson
Cc: anthonyfieroni, bruns, dkorth, ngraham, kde-frameworks-devel, michaelh

D17554: [KFileItem] Fix isLocal check in checkDesktopFile

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:86e3142f8588: [KFileItem] Fix isLocal check in 
checkDesktopFile (authored by broulik).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17554?vs=47493&id=47498

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

AFFECTED FILES
  src/core/kfileitem.cpp

To: broulik, dfaure, davidedmundson, shubham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17554: [KFileItem] Fix isLocal check in checkDesktopFile

[Kai Uwe Broulik]

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

REVISION SUMMARY
  This was accidentally removed in 16edef0dca46f4292bf83d22874a02d8e6fe15e7 
 and 
then cleaned up improperly in c08ee4577168070896a4a46220cd39df4e4e32cf 

  
  BUG: 401947

TEST PLAN
  "Empty Trash" now shows up for Trash on desktop again, as `desktop:/` URL is 
correctly treated as local again

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/core/kfileitem.cpp

To: broulik, dfaure, davidedmundson, shubham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17548: Fixed link to the coding style wiki page

[Kai Uwe Broulik]

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

REPOSITORY
  R293 Baloo

BRANCH
  fixed_typo_in_readme (branched from master)

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

To: janpr, #baloo, broulik
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D17173: Add error value for job owner dying

[Kai Uwe Broulik]

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

REPOSITORY
  R241 KIO

BRANCH
  master

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

To: davidedmundson, dfaure, broulik
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D17173: Add error value for job owner dying

[Kai Uwe Broulik]

broulik added a reviewer: dfaure.

REPOSITORY
  R241 KIO

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

To: davidedmundson, dfaure
Cc: broulik, kde-frameworks-devel, michaelh, ngraham, bruns

D17173: Add error value for job owner dying

[Kai Uwe Broulik]

broulik added a comment.


  Now that `ERR_FILE_TOO_LARGE_FOR_FAT32` is in you can rebase this

REPOSITORY
  R241 KIO

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

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

D17545: Do not stat move/copy job if the destination file system does not support writing

[Kai Uwe Broulik]

broulik added a comment.


  The bug report is about checking whether it's writable, you're checking the 
protocol here, and `file` will always "support writing". This patch could still 
make sense, though.

INLINE COMMENTS

> copyjob.cpp:864
> +QPointer that = q;
> +emit q->warning(q, buildErrorString(ERR_CYCLIC_COPY, 
> m_currentDestURL.toDisplayString()));
> +if (that) {

Not sure `ERR_CYCLIC_COPY` is the correct error for this?

REPOSITORY
  R241 KIO

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

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

D17505: Add preferences-system-bluetooth-battery to preferences.svgz

[Kai Uwe Broulik]

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


  Thanks!

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  bluetooth-battery-icon (branched from master)

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

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

D17543: [Dialog] Don't alter mainItem's visibility

[Kai Uwe Broulik]

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

REVISION SUMMARY
  The item's `visible` property is independent of the window visibility, which 
can be checked using `Window.visible`.

TEST PLAN
  Needs D17542  to fix tooltips
  Other than that, kickoff, systray, panelcontroller, notification popups, task 
manager group dialogs, visibility of pager depending on panelcontroller 
opening, still works.
  dialogqmltest and dialogstatetest still pass, dialognativetest is broken 
before and after
  
  Notifications no longer get bogus `containsMouse` and properly times out:
  When you closed notification popup by clicking the X button inside the popup, 
and the popup shows again later, because we manually set `visible` to `true`, 
`QQuickMouseArea` enters the following code
  
case ItemVisibleHasChanged:
   if (acceptHoverEvents() && d->hovered != (isVisible() && 
isUnderMouse())) {
   if (!d->hovered) {
   QPointF cursorPos = 
QGuiApplicationPrivate::lastCursorPosition;
   d->lastScenePos = 
d->window->mapFromGlobal(cursorPos.toPoint());
   d->lastPos = mapFromScene(d->lastScenePos);
   }
   setHovered(!d->hovered);
   }
  
  `QGuiApplicationPrivate::lastCursorPosition` is only updated when a 
`plasmashell` window is hovered, which is usually not the case when you just 
dismissed a notification popup ontop of another application. Hence, the 
position is incorrect and opening the dialog again causes it to think it's 
hovered.
  `isUnderMouse()` basically checks 
`contains(QGuiApplicationPrivate::lastCursorPosition)`.
  If we don't mess with visibility, none of this happens.

REPOSITORY
  R242 Plasma Framework (Library)

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

AFFECTED FILES
  src/plasmaquick/dialog.cpp

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

D17542: Reset parentItem when mainItem changes

[Kai Uwe Broulik]

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

REVISION SUMMARY
  Otherwise we'll keep piling up custom tooltip items in the dialog. They're 
invisible but they're still there.

TEST PLAN
  Kept the contentItem visible, moved between TM and kickoff, tooltip no longer 
ends up with multiple items in it:
  Before:
  F6473103: Screenshot_20181212_151924.png 

  They're not "leaked" as it doesn't change ownership (`parent` vs 
`parentItem`) but it still adds unnccessary invisible items to the window

REPOSITORY
  R242 Plasma Framework (Library)

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

AFFECTED FILES
  src/plasmaquick/dialog.cpp

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

D17518: Adapt favicon test to www.kde.org changes

[Kai Uwe Broulik]

broulik added a comment.


  I also thought the favicon was too large for `FavIconJob` to download

REPOSITORY
  R241 KIO

BRANCH
  favicon_test

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

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

D17505: Add preferences-system-bluetooth-battery to preferences.svgz

[Kai Uwe Broulik]

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


  The stylesheet stuff doesn't work, ie. the icon stays black in Breeze dark.
  Also, should it maybe have a "bolt" or something next to it as that's what we 
normally have for the batteries (optional)

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D17479: Fix build without phonon

[Kai Uwe Broulik]

broulik added a reviewer: sitter.

REPOSITORY
  R305 KNotifyConfig

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

To: heikobecker, sitter
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17480: Fix warnings

[Kai Uwe Broulik]

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

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

BRANCH
  master

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

To: apol, #frameworks, mart, broulik
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D16299: fallback to dnssd service discovery if smb listDir failed on root

[Kai Uwe Broulik]

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

REPOSITORY
  R320 KIO Extras

BRANCH
  smb-discovery

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

To: sitter, #frameworks, #dolphin, broulik
Cc: acrouthamel, alexde, bcooksley, ngraham, kde-frameworks-devel, kfm-devel, 
sourabhboss, feverfew, michaelh, spoorun, navarromorales, firef, andrebarros, 
bruns, emmanuelp, mikesomov

D17345: Support Bluetooth batteries

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R245:b983836f05cd: Support Bluetooth batteries (authored by 
broulik).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D17345?vs=46834&id=46837#toc

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17345?vs=46834&id=46837

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

AFFECTED FILES
  src/solid/devices/backends/fakehw/fakebattery.cpp
  src/solid/devices/backends/upower/upowerbattery.cpp
  src/solid/devices/backends/upower/upowerdevice.cpp
  src/solid/devices/frontend/battery.h

To: broulik, #plasma, bruns, davidedmundson, bshah
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17345: Support Bluetooth batteries

[Kai Uwe Broulik]

broulik added a dependent revision: D17346: Support Bluetooth batteries.

REPOSITORY
  R245 Solid

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

To: broulik, #plasma, bruns, davidedmundson, bshah
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17345: Support Bluetooth batteries

[Kai Uwe Broulik]

broulik added a reviewer: bshah.

REPOSITORY
  R245 Solid

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

To: broulik, #plasma, bruns, davidedmundson, bshah
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17345: Support Bluetooth batteries

[Kai Uwe Broulik]

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

REVISION SUMMARY
  There's currently no generic Bluetooth device type in UPower, so we get them 
of "unknown" type.
  Check for whether it comes from Bluez to determine it's a Bluetooth battery.

TEST PLAN
  Using upower and bluez git master and a patch to plasma-workspace I now have 
my headset show report its battery level.
  Without plasma-workspace patch it shows up as generic battery but at least it 
shows up
  F6454744: Screenshot_20181204_151718.png 

  "Kopfhörer" is the actual device name, not type, very creative :)

REPOSITORY
  R245 Solid

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

AFFECTED FILES
  src/solid/devices/backends/fakehw/fakebattery.cpp
  src/solid/devices/backends/upower/upowerbattery.cpp
  src/solid/devices/backends/upower/upowerdevice.cpp
  src/solid/devices/frontend/battery.h

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

D17260: Port some core Q_PRIVATE_SLOTS to new connect syntax

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> filecopyjob.cpp:225
>  KIO_ARGS << m_src << m_dest << m_permissions << (qint8)(m_flags & 
> Overwrite);
> -m_copyJob = new DirectCopyJob(slave_url, packedArgs);
> +auto job = new DirectCopyJob(slave_url, packedArgs);
> +m_copyJob = job;

Why this change?

> filecopyjob.cpp:259
> +
> +q->connect(job, &KJob::processedSize, [this](KJob *job, qulonglong 
> processedSize) {
> +   slotProcessedSize(job, processedSize);

Add `q` context

> filecopyjob.cpp:260
> +q->connect(job, &KJob::processedSize, [this](KJob *job, qulonglong 
> processedSize) {
> +   slotProcessedSize(job, processedSize);
> +});

All `slotProcessedSize` does is call `q->setProcessedAmount` so you can 
probably call this directly

> filecopyjob.cpp:263
>  
> -q->connect(job, SIGNAL(processedSize(KJob*,qulonglong)),
> -   SLOT(slotProcessedSize(KJob*,qulonglong)));
> +q->connect(job, QOverload::of(&KJob::percent), [this](KJob 
> *job, ulong percent) {
> +slotPercent(job, percent);

Same as `processedSize`

> job.cpp:88
>  // Forward information from that subjob.
> -connect(job, SIGNAL(speed(KJob*,ulong)),
> -SLOT(slotSpeed(KJob*,ulong)));
> -
> +connect(job, &KJob::speed, this, [=](KJob *job, ulong speed) {
> +Q_UNUSED(job);

Capture only `this`

REPOSITORY
  R241 KIO

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

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

D17238: Explicitly create QDateTime with UTC time

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:a9592e0b69c9: Explicitly create QDateTime with UTC time 
(authored by broulik).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17238?vs=46485&id=46489

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

AFFECTED FILES
  src/core/copyjob.cpp

To: broulik, dfaure, davidedmundson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17238: Explicitly create QDateTime with UTC time

[Kai Uwe Broulik]

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

REVISION SUMMARY
  Otherwise it spends half of the time doing timezone conversions to local time.

TEST PLAN
  - Copied a million files. Time spent in `QDateTime::setMSecsSinceEpoch` went 
from 44% to 0.1%, total time of `addCopyInfoFromUDSEnty` went from 57% to 36%, 
now the bottleneck is all that path concatenation causing URL decoding.
  - Verified that copied files still have correct mtime
  - Verified that "overwrite?" dialog still shows correct mtime

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/core/copyjob.cpp

To: broulik, dfaure, davidedmundson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17167: Provide a method of assigning custom default icons for non-XDG dirs

[Kai Uwe Broulik]

broulik added a reviewer: dfaure.

REPOSITORY
  R241 KIO

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

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

D17206: Rename NotifyByFlatpak to NotifyByPortal

[Kai Uwe Broulik]

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

REPOSITORY
  R289 KNotifications

BRANCH
  snap-support

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

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

D17188: Notification portal: support pixmaps in notifications

[Kai Uwe Broulik]

broulik added a comment.


  +1

INLINE COMMENTS

> notifybyflatpak.h:40
>  public:
> +struct PortalIcon {
> +QString str;

Does this need to be in the header file?

REPOSITORY
  R289 KNotifications

BRANCH
  master

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

To: jgrulich, broulik, apol
Cc: apol, kde-frameworks-devel, michaelh, ngraham, bruns

D17173: Add error value for job owner dying

[Kai Uwe Broulik]

broulik added a comment.


  +1

INLINE COMMENTS

> global.h:249
> +ERR_CANNOT_CREATE_SLAVE = KJob::UserDefinedError + 73, ///< used by 
> Slave::createSlave, @since 5.30
> +ERR_OWNER_DIED = KJob::UserDefinedError + 75, ///< Value used between 
> kuiserver and views when the job owner disappears unexpectedly. It should not 
> be emitted by slaves. @since 5.53
>  };

Why not 74?

REPOSITORY
  R241 KIO

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

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

D17088: [thumbnailer appimage] Fix building with libappimage not in system path

[Kai Uwe Broulik]

broulik added a comment.


  Don't know much cmake but still builds fine here with this change, so +1 from 
me

REPOSITORY
  R320 KIO Extras

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

To: kossebau, broulik, TheAssassin, azubieta
Cc: kde-frameworks-devel, kfm-devel, alexde, sourabhboss, feverfew, michaelh, 
spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, 
mikesomov

D17078: Make it possible to use KAboutData/License/Person from QML

[Kai Uwe Broulik]

broulik added a comment.


  You can change default values just fine, it requires re-compilation to use 
them but no ABI implications.

REPOSITORY
  R244 KCoreAddons

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

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

D17068: actually install the version header

[Kai Uwe Broulik]

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

REPOSITORY
  R272 KDNSSD

BRANCH
  install-version

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

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

D17067: do not leak resolver in remoteservice

[Kai Uwe Broulik]

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

REPOSITORY
  R272 KDNSSD

BRANCH
  master

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

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

D16298: prevent avahi signal racing

[Kai Uwe Broulik]

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

REPOSITORY
  R272 KDNSSD

BRANCH
  master

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

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

D17013: Show menu bar, how to re-enable, common shortcut dialog

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> ktoggleshowmenubaraction.cpp:26
> +
> +#define i18n QString::fromLatin1
> +

What's this for?

> ktoggleshowmenubaraction.h:49
> + */
> +KToggleShowMenuBarAction(QWidget *window, QObject *parent);
> +

I know we typically use `window` here for legacy reasons, but given there's 
`QWindow` in Qt 5, might be better to use `widget` for all of this now?
Just asking, you don't have to change this now, unless someone else comments :)

> ktoggleshowmenubaraction.h:78
> +
> +
> +private:

There's an extra empty line

REPOSITORY
  R236 KWidgetsAddons

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

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

D16301: Remove ComponentInstaller

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:38098e96e3d8: Remove ComponentInstaller (authored by 
broulik).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16301?vs=43874&id=45796

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

AFFECTED FILES
  autotests/CMakeLists.txt
  src/plasma/CMakeLists.txt
  src/plasma/private/componentinstaller.cpp
  src/plasma/private/componentinstaller_p.h
  src/plasma/private/dataenginemanager.cpp
  src/plasma/scripting/scriptengine.cpp

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

D16108: When re-using runners when reloading, reload their configuration

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R308:770f8d64a495: When re-using runners when reloading, 
reload their configuration (authored by broulik).

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16108?vs=43321&id=45784

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

AFFECTED FILES
  src/runnermanager.cpp

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

D16885: Delete the correct item in removeDesktop

[Kai Uwe Broulik]

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

REPOSITORY
  R127 KWayland

BRANCH
  master

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

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

D16684: Updated with petroleum industry units

[Kai Uwe Broulik]

broulik added a comment.


  Looking good now, +1

REPOSITORY
  R292 KUnitConversion

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

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

D16684: Updated with petroleum industry units

[Kai Uwe Broulik]

broulik added a comment.


  The header file for `class Permeability` should probably be named 
`permeability_p.h` to match the other category classes since it's a not 
exported implementation detail

INLINE COMMENTS

> unit.h:335
> +/** @since 5.53 */
> +Darcy,
> +/** @since 5.53 */

Given this is a new category, assign `Darcy = 33000` so that one can still add 
new values after `Yoctoohm` to electrical resistance category without breaking 
compatibility

REPOSITORY
  R292 KUnitConversion

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

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

D16678: Fix root disk icon change so that it doesn't erroneously change other icons

[Kai Uwe Broulik]

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

REPOSITORY
  R245 Solid

BRANCH
  fix-root-icon-change (branched from master)

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

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

D16678: Fix root disk icon change so that it doesn't erroneously change other icons

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> udisksdevice.cpp:874
> +{
> +if (queryDeviceInterface(Solid::DeviceInterface::StorageAccess)) {
> +const UDisks2::StorageAccess accessIface(const_cast(this));

Can you check whether using `isStorageAccess()` is more desirable/performant?

> udisksdevice.h:84
>  bool isEncryptedCleartext() const;
> +bool isRoot() const; // @since 5.53
>  bool isSwap() const;

This isn't public API, you don't need this comment, also perhaps `isRootVolume`

REPOSITORY
  R245 Solid

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

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

D16620: add const to TypeInfo compare operater

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> typeinfo.h:41
>  TypeInfo& operator=(const TypeInfo& rhs);
> -bool operator==(const TypeInfo& rhs);
> +bool operator==(const TypeInfo& rhs) const;
>  

This method is exported so you cannot change cv qualifiers

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #frameworks, bruns
Cc: broulik, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D16594: Add context to kcmodule connection to lambdas

[Kai Uwe Broulik]

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

REPOSITORY
  R295 KCMUtils

BRANCH
  master

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

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

D16529: make libssh module default to the "new" libssh config by default

[Kai Uwe Broulik]

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

REPOSITORY
  R320 KIO Extras

BRANCH
  master

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

To: sitter, broulik, asn
Cc: kde-frameworks-devel, kfm-devel, alexde, sourabhboss, feverfew, michaelh, 
spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp

D16522: [Extractor] Do not check QFile::exists for an empty url

[Kai Uwe Broulik]

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

REPOSITORY
  R293 Baloo

BRANCH
  extractor

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

To: bruns, #baloo, #frameworks, dfaure, apol, broulik
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D16464: [KSambaShare] Trim trailing / from share path

[Kai Uwe Broulik]

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

REPOSITORY
  R241 KIO

BRANCH
  net_usershare

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

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

D16427: [solid-hardware5] List icon in device details

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R245:9910ad1473f4: [solid-hardware5] List icon in device 
details (authored by broulik).

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16427?vs=44214&id=44215

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

AFFECTED FILES
  src/tools/solid-hardware/solid-hardware.cpp

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

D16427: [solid-hardware5] List icon in device details

[Kai Uwe Broulik]

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

TEST PLAN
udi = 
'/org/kde/solid/udev/sys/devices/pci:00/:00:01.1/:01:00.0/usb1/1-7'
  parent = '/org/kde/solid/udev'  (string)
  vendor = 'LGE'  (string)
  product = 'Nexus 5'  (string)
  description = 'Nexus 5'  (string)
  icon = 'multimedia-player'  (string)

REPOSITORY
  R245 Solid

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

AFFECTED FILES
  src/tools/solid-hardware/solid-hardware.cpp

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

D16166: Pass the FileIndexerConfig as const to the individual indexers

[Kai Uwe Broulik]

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

REPOSITORY
  R293 Baloo

BRANCH
  indexer_cleanup

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

To: bruns, #baloo, #frameworks, poboiko, ngraham, dfaure, broulik
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D16387: [sftp] put request chunk debugging into own category

[Kai Uwe Broulik]

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

REPOSITORY
  R320 KIO Extras

BRANCH
  sftp-trace-level

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

To: sitter, asn, broulik, ngraham
Cc: kde-frameworks-devel, kfm-devel, sourabhboss, feverfew, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp

D16313: Use QMenu::addSeparator

[Kai Uwe Broulik]

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

REPOSITORY
  R241 KIO

BRANCH
  arc_addseparator (branched from master)

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

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

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

[Kai Uwe Broulik]

broulik retitled this revision from "[KFilePlacesView] Use asynchronous 
KIO::FileSystemFreeSpaceJob" to "RFC: [KFilePlacesView] Use asynchronous 
KIO::FileSystemFreeSpaceJob".

REPOSITORY
  R241 KIO

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

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

D16311: [KFilePlacesView] Use asynchronous KIO::FileSystemFreeSpaceJob

[Kai Uwe Broulik]

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

REVISION SUMMARY
  This ensure hovering a broken mount will not freeze the UI.
  I already optimized it in D11088  to 
avoid doing blocking calls for when the bar isn't shown but this patch makes it 
fully async now.
  Moreover, refresh only every 60 seconds (it's what Dolphin as well as 
Plasma's free space warning do), currently it would refresh every frame

TEST PLAN
  Capacity bar shows up for a mounted drive and updates after a while when you 
hover it again.
  It doesn't update "live" but it never did that, only happened to because it 
was querying free space like a mad man.
  
  In theory this would also enable free space bars for network shares but 
`CapacityBarRecommendedRole` is only for mounted local storage sans CD rom.

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/filewidgets/kfileplacesview.cpp

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

D16301: Remove ComponentInstaller

[Kai Uwe Broulik]

broulik added a comment.


  > Method InstallPackages
  
  Thanks for the insight. In any case, the way the code "works" right now is 
probably not what we want. It has been unused for years and is unlikely to come 
back in that form, so we can rm it. If we were to support something like this 
we need to modernize the architecture using AppStream and what not anyway.

REPOSITORY
  R242 Plasma Framework (Library)

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

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

D16301: Remove ComponentInstaller

[Kai Uwe Broulik]

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

REVISION SUMMARY
  When a dataengine failed to load it would ask PackageKit to install it but 
it's generally not how we want to distribute plasmoid infrastructure these days 
and didn't work.

TEST PLAN
  The interface didn't exist here, and I couldn't find it in the docs, and it's 
ancient anyway. Even if it worked, it would still fail to load the engine, have 
you install it (assuming it even finds it), and then you have to restart plasma 
to load it.

REPOSITORY
  R242 Plasma Framework (Library)

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

AFFECTED FILES
  autotests/CMakeLists.txt
  src/plasma/CMakeLists.txt
  src/plasma/private/componentinstaller.cpp
  src/plasma/private/componentinstaller_p.h
  src/plasma/private/dataenginemanager.cpp
  src/plasma/scripting/scriptengine.cpp

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

D16295: Remove PLASMA_NO_KIO

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:e92ed94d9d1a: Remove PLASMA_NO_KIO option (authored by 
broulik).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16295?vs=43857&id=43872

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

AFFECTED FILES
  autotests/CMakeLists.txt
  src/plasma/CMakeLists.txt
  src/plasma/config-plasma.h.cmake
  src/plasma/containment.cpp
  src/plasma/pluginloader.cpp
  src/plasma/private/associatedapplicationmanager.cpp
  src/scriptengines/qml/CMakeLists.txt
  src/scriptengines/qml/plasmoid/containmentinterface.cpp

To: broulik, #plasma, mart, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, michaelh, ngraham, bruns

D16300: Remove double underscore (__) from header include guards

[Kai Uwe Broulik]

broulik added a comment.


  It could have just gone `#pramga once`

REPOSITORY
  R39 KTextEditor

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

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

D16298: prevent avahi signal racing

[Kai Uwe Broulik]

broulik added a comment.


  Not sure if evil or genius ;)
  Looks sane, as sane as it gets, obviously.

INLINE COMMENTS

> avahi_listener_p.h:29
> +
> +// Assits with listening to Avahi for all signals and then checking if the
> +// a given dbus message is meant for us or not.

*Assists

> avahi_listener_p.h:41
> +// This method gets called a lot but doesn't do much. Suggest inlining.
> +inline bool isOurMsg(const QDBusMessage &msg)
> +{

Make `const`

REPOSITORY
  R272 KDNSSD

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

To: sitter, mdawson, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D16295: Remove PLASMA_NO_KIO

[Kai Uwe Broulik]

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

REVISION SUMMARY
  It is a remnant of the Active days and didn't even compile.
  This patch only removes those ifdefs, it doesn't fix any issues I found 
during (e.g. double lookups or the fact that `KRun::autoDelete` is the default 
now)

TEST PLAN
  I think the entire `KDE_PLATFORM_FEATURE_BINARY_COMPATIBLE_FEATURE_REDUCTION` 
could be removed, but one step at a time
  
  Compiles, dropping files on desktop still asks what do to and adding a widget 
or setting as wallpaper works

REPOSITORY
  R242 Plasma Framework (Library)

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

AFFECTED FILES
  autotests/CMakeLists.txt
  src/plasma/CMakeLists.txt
  src/plasma/config-plasma.h.cmake
  src/plasma/containment.cpp
  src/plasma/pluginloader.cpp
  src/plasma/private/associatedapplicationmanager.cpp
  src/scriptengines/qml/CMakeLists.txt
  src/scriptengines/qml/plasmoid/containmentinterface.cpp

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

D16291: Center icons properly if size doesn't fit

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R302:2699d36365fa: Center icons properly if size doesn't 
fit (authored by broulik).

REPOSITORY
  R302 KIconThemes

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16291?vs=43847&id=43848

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

AFFECTED FILES
  src/kiconengine.cpp

To: broulik, dfaure, cfeck, ngraham, davidedmundson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns