D13988: Use subseq matching for service runner

[Nathaniel Graham]

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


  Fantastic. This works great for me, but let's get some more opinions too.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  feat/app-name-subseq

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

To: michaeleden, rthomas, #plasma_workspaces, #plasma, broulik, ngraham
Cc: cfeck, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14493: fontinst quits after KJob is done

[Nathaniel Graham]

ngraham added a comment.


  Does this fix 379524 and 379324? See 
https://community.kde.org/Infrastructure/Phabricator#Add_special_keywords

REPOSITORY
  R119 Plasma Desktop

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

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

D14437: Fix QFileDialog not remembering the last visited directory.

[Aleix Pol Gonzalez]

apol added a comment.


  +1 from me on the patch.
  I'd prefer without the comment and just have it investigated, I'm not sure 
the comment will help.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: dfaure, anthonyfieroni, elvisangelaccio, plasma-devel, broulik, arichardson
Cc: apol, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, mart

D13988: Use subseq matching for service runner

[Christoph Feck]

cfeck added a comment.


  According to https://community.kde.org/Schedules/Plasma_5 the Plasma 5.14 
release is planned to depend on Frameworks 5.50.

REPOSITORY
  R120 Plasma Workspace

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

To: michaeleden, rthomas, #plasma_workspaces, #plasma, broulik
Cc: cfeck, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D12708: Only include QtQuick support in Breeze KStyle if QtQuick is available

[Nathaniel Graham]

ngraham added a comment.


  I'm raid this patch no longer applies cleanly on `master`:
  
Created and checked out branch arcpatch-D12708.
Checking patch kstyle/config-breeze.h.cmake...
Checking patch kstyle/breezewindowmanager.h...
Checking patch kstyle/breezewindowmanager.cpp...
Checking patch kstyle/breezestyle.cpp...
Hunk #1 succeeded at 7158 (offset 74 lines).
Checking patch kstyle/CMakeLists.txt...
Hunk #1 succeeded at 25 (offset -15 lines).
Hunk #2 succeeded at 33 (offset -15 lines).
Hunk #3 succeeded at 41 (offset -15 lines).
error: while searching for:

  kconfig_add_kcfg_files(breeze_PART_SRCS breezestyleconfigdata.kcfgc)
  add_library(breeze MODULE ${breeze_PART_SRCS})
  target_link_libraries(breeze Qt5::Core Qt5::Gui Qt5::Widgets Qt5::DBus 
Qt5::Quick)
  target_link_libraries(breeze KF5::ConfigCore KF5::ConfigWidgets 
KF5::GuiAddons KF5::WindowSystem)

  if( KF5FrameworkIntegration_FOUND )

error: patch failed: kstyle/CMakeLists.txt:166
Applied patch kstyle/config-breeze.h.cmake cleanly.
Applied patch kstyle/breezewindowmanager.h cleanly.
Applied patch kstyle/breezewindowmanager.cpp cleanly.
Applied patch kstyle/breezestyle.cpp cleanly.
Applying patch kstyle/CMakeLists.txt with 1 reject...
Hunk #1 applied cleanly.
Hunk #2 applied cleanly.
Hunk #3 applied cleanly.
Rejected hunk #4.

 Patch Failed! 
Usage Exception: Unable to apply patch!
  
  Can you re-base it on master? (`git pull --rebase origin/master`). Thanks!

REPOSITORY
  R31 Breeze

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

To: tundracomp, mart
Cc: ngraham, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14489: [ProcessModel] Support high dpi for window pixmaps

[David Edmundson]

davidedmundson added a comment.


  KWindowSystem::icon doesn't set a DPR on the returned pixmap  which sounds 
wrong.

REPOSITORY
  R111 KSysguard Library

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

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

D14489: [ProcessModel] Support high dpi for window pixmaps

[Kai Uwe Broulik]

broulik added a comment.


  Interesting. Without this patch, I had some icons come out too small, will 
investigate.

REPOSITORY
  R111 KSysguard Library

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

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

D14493: fontinst quits after KJob is done

[Mathias Tillman]

mathiastillman created this revision.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
mathiastillman requested review of this revision.

REVISION SUMMARY
  As the summary says, when installing/removing multiple fonts to system the 
expected behaviour would be for fontinst to keep running after the first font 
has been installed or removed, instead it quits which causes a bunch of issues. 
fontinst uses KJob to authorize and internally KJob uses a QEventLoopLocker 
which causes the main event loop to quit when it's done.
  I'm not entirely sure why the event loop locker is enabled by default for 
KJob, but the patch I have attached works around this by completely disabling 
that functionality for fontinst. There's a timer that runs in the background 
which checks for any connected clients, so it will quit after a little while 
regardless.
  See #379524 and #379324 at bugs.kde.org.

TEST PLAN
  Make sure fonts are still installed and removed properly.

REPOSITORY
  R119 Plasma Desktop

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

AFFECTED FILES
  kcms/kfontinst/dbus/Main.cpp

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

D10040: Add serial number and EISA ID to OutputDevice interface

[Roman Gilg]

romangg added a comment.


  In D10040#300588 , @davidedmundson 
wrote:
  
  > I just removed handling of dynamically updating eisa/serialNumber it 
doesn't seem to be something that makes sense for it to change at runtime.
  >
  > Also I don't want to copy the current setEdid pattern, which is broken ATM. 
Whenever any new client connects it broadcasts a change to every connected 
client...
  >  I need to follow that up in another patch, possibly by making it static 
like these two.
  
  
  Can you explain both points a bit more? Maybe when updating the Summary.

REPOSITORY
  R127 KWayland

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

To: davidedmundson, graesslin, sebas, #kwin, dvratil
Cc: romangg, kde-frameworks-devel, davidedmundson, plasma-devel, ragreen, 
Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D14487: [Klipper] Only create KHelpMenu when used

[David Edmundson]

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


  KHelpMenu looks to lazy load internally inside KHelpMenu::menu.
  
  What issue did this cause?

REPOSITORY
  R120 Plasma Workspace

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

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

D14489: [ProcessModel] Support high dpi for window pixmaps

[David Edmundson]

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


QPixmap KWindowSystem::icon(WId win, int width, int height, bool scale, int 
flags, NETWinInfo *info)
{
Q_D(KWindowSystem);
width *= qApp->devicePixelRatio();
height *= qApp->devicePixelRatio();

REPOSITORY
  R111 KSysguard Library

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

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

D14489: [ProcessModel] Support high dpi for window pixmaps

[David Edmundson]

davidedmundson added a comment.


  Not here, no.

REPOSITORY
  R111 KSysguard Library

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

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

D14489: [ProcessModel] Support high dpi for window pixmaps

[Nathaniel Graham]

ngraham added a comment.


  Should this be `devicePixelRatioF()` to support fractional scale factors?

REPOSITORY
  R111 KSysguard Library

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

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

D14490: [System Monitor] Enable high dpi pixmaps

[Kai Uwe Broulik]

broulik created this revision.
broulik added reviewers: Plasma, davidedmundson.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
broulik requested review of this revision.

TEST PLAN
  - Needs D14489  for proper window pixmaps
  - Clear icon in search field is crisp now
  - Icons in filter and More menus are crisp now

REPOSITORY
  R120 Plasma Workspace

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

AFFECTED FILES
  systemmonitor/main.cpp

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

D14489: [ProcessModel] Support high dpi for window pixmaps

[Kai Uwe Broulik]

broulik created this revision.
broulik added reviewers: Plasma, davidedmundson.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
broulik requested review of this revision.

REVISION SUMMARY
  Request larger pixmaps and set `devicePixelRatio` accordingly

REPOSITORY
  R111 KSysguard Library

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

AFFECTED FILES
  processui/ProcessModel.cpp

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

D14488: [Positioner] Be a flat list model

[Kai Uwe Broulik]

broulik created this revision.
broulik added reviewers: Plasma, hein.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
broulik requested review of this revision.

REVISION SUMMARY
  Return rowCount of `0` for sub items

TEST PLAN
  Noticed GammaRay thought the desktop positioner model was a tree
  Items still show up fine on desktop

REPOSITORY
  R119 Plasma Desktop

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

AFFECTED FILES
  containments/desktop/plugins/folder/positioner.cpp

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

D14487: [Klipper] Only create KHelpMenu when used

[Kai Uwe Broulik]

broulik created this revision.
broulik added reviewers: Plasma, graesslin.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
broulik requested review of this revision.

REVISION SUMMARY
  The entry is only shown when run standalone anyway.

TEST PLAN
  Still works when run standalone

REPOSITORY
  R120 Plasma Workspace

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

AFFECTED FILES
  klipper/klipperpopup.cpp

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

D14480: Don't unintentionally change font rendering when rendering preview images

[Julian Wolff]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:79a4bbc36cee: Dont unintentionally change font 
rendering when rendering preview images (authored by progwolff).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14480?vs=38767=38777

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

AFFECTED FILES
  kcms/fonts/previewimageprovider.cpp

To: progwolff, ngraham
Cc: ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D14480: Don't unintentionally change font rendering when rendering preview images

[Nathaniel Graham]

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


  Good catch.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  Plasma/5.13

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

To: progwolff, ngraham
Cc: ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D12925: Parse global config files. Remove 'Vendor default' option. Fix changes not recognized.

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a reviewer: Plasma.
ngraham added a comment.
This revision now requires changes to proceed.


  Nice! Two nitpicks:
  
  - `Anti-aliasing: [x] Use antialiasing` Either change the checkbox's label to 
"Enable", or remove the left label and give this whole section its own header.
  - We've still got a "Vendor Default" option for "Sub-pixel rendering type". 
Instead, there should really be a "none" option there to complement the options 
for the different types.

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, ngraham, #plasma
Cc: rkflx, ngraham, mart, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D10040: Add serial number and EISA ID to OutputDevice interface

[David Edmundson]

davidedmundson added a comment.


  I just removed handling of dynamically updating eisa/serialNumber it doesn't 
seem to be something that makes sense for it to change at runtime.
  
  Also I don't want to copy the current setEdid pattern, which is broken ATM. 
Whenever any new client connects it broadcasts a change to every connected 
client...
  I need to follow that up in another patch, possibly by making it static like 
these two.

REPOSITORY
  R127 KWayland

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

To: davidedmundson, graesslin, sebas, #kwin, dvratil
Cc: romangg, kde-frameworks-devel, davidedmundson, plasma-devel, ragreen, 
Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D10040: Add serial number and EISA ID to OutputDevice interface

[David Edmundson]

davidedmundson updated this revision to Diff 38772.
davidedmundson added a comment.


  Remove unrelated changes

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10040?vs=38734=38772

BRANCH
  output_changes

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

AFFECTED FILES
  autotests/client/test_wayland_outputdevice.cpp
  src/client/outputdevice.cpp
  src/client/outputdevice.h
  src/client/protocols/outputdevice.xml
  src/server/outputdevice_interface.cpp
  src/server/outputdevice_interface.h

To: davidedmundson, graesslin, sebas, #kwin, dvratil
Cc: romangg, kde-frameworks-devel, davidedmundson, plasma-devel, ragreen, 
Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D14474: Stop enforcing spacings

[Aleix Pol Gonzalez]

This revision was automatically updated to reflect the committed changes.
Closed by commit R134:b648825801a3: Stop enforcing spacings (authored by apol).

REPOSITORY
  R134 Discover Software Store

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14474?vs=38736=38770

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

AFFECTED FILES
  discover/qml/UpdatesPage.qml

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

D10040: Add serial number and EISA ID to OutputDevice interface

[Roman Gilg]

romangg added inline comments.

INLINE COMMENTS

> outputdevice.cpp:161
> +  int32_t subPixel, const char *make, const char 
> *model,
> +  int32_t transform)
>  {

Unrelated change.

> outputdevice_interface.cpp:153
> +connect(this, ::serialNumberChanged,   this, 
> [this, d] { d->updateGeometry(); });
> +connect(this, ::eisaIdChanged, this, 
> [this, d] { d->updateGeometry(); });
>  connect(this, ::scaleChanged,  this, 
> [this, d] { d->updateScale(); });

needs fix

> outputdevice_interface.cpp:410
> +wl_resource_post_event(resource,
> +ORG_KDE_KWIN_OUTPUTDEVICE_GEOMETRY,
>  globalPosition.x(),

Why change? Unrelated.

> outputdevice_interface.h:50
> +//KF6 TODO - This class sends absolute garbage over the wire constantly.
> +//sendDone needs to be explicit, anything related to the geometry event 
> needs to be in a single method
> +

What do you mean with "needs to be explicit"?

REPOSITORY
  R127 KWayland

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

To: davidedmundson, graesslin, sebas, #kwin, dvratil
Cc: romangg, kde-frameworks-devel, davidedmundson, plasma-devel, ragreen, 
Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

D14480: Don't unintentionally change font rendering when rendering preview images

[Julian Wolff]

progwolff created this revision.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
progwolff requested review of this revision.

REVISION SUMMARY
  Font rendering settings are changed temporarily to render preview images. The 
changed settings were
  reverted, but never reapplied. This way rendering preview images could alter 
the user's font rendering 
  configuration.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  Plasma/5.13

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

AFFECTED FILES
  kcms/fonts/previewimageprovider.cpp

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

D12925: Parse global config files. Remove 'Vendor default' option. Fix changes not recognized.

[Julian Wolff]

progwolff updated this revision to Diff 38764.
progwolff added a comment.


  - split changes to previewimageprovider.cpp to a separate commit

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12925?vs=38757=38764

BRANCH
  arcpatch-D12925

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

AFFECTED FILES
  kcms/fonts/fonts.cpp
  kcms/fonts/fonts.h
  kcms/fonts/kxftconfig.cpp
  kcms/fonts/kxftconfig.h
  kcms/fonts/package/contents/ui/main.qml

To: progwolff
Cc: rkflx, ngraham, mart, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D14444: Use nullptr/override

[Alex]

al1xz updated this revision to Diff 38762.
al1xz marked an inline comment as done.
al1xz added a comment.


  Update

REPOSITORY
  R106 KSysguard

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D1?vs=38674=38762

BRANCH
  arcpatch-D1

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

AFFECTED FILES
  gui/SensorBrowser.cpp
  gui/SensorBrowser.h
  gui/SensorDisplayLib/BarGraph.h
  gui/SensorDisplayLib/DancingBars.h
  gui/SensorDisplayLib/DummyDisplay.h
  gui/SensorDisplayLib/FancyPlotter.cpp
  gui/SensorDisplayLib/FancyPlotter.h
  gui/SensorDisplayLib/ListView.cpp
  gui/SensorDisplayLib/ListView.h
  gui/SensorDisplayLib/ListViewSettings.cpp
  gui/SensorDisplayLib/LogFile.h
  gui/SensorDisplayLib/MultiMeter.h
  gui/SensorDisplayLib/MultiMeterSettings.cpp
  gui/SensorDisplayLib/ProcessController.h
  gui/SensorDisplayLib/SensorDisplay.h
  gui/SensorDisplayLib/SensorLogger.cpp
  gui/SensorDisplayLib/SensorLogger.h
  gui/SensorDisplayLib/SensorLoggerDlg.cpp
  gui/SensorDisplayLib/SensorLoggerSettings.cpp
  gui/SensorDisplayLib/SensorModel.h
  gui/WorkSheet.cpp
  gui/WorkSheet.h
  gui/Workspace.cpp
  gui/ksortfilterproxymodel.cpp
  gui/ksortfilterproxymodel.h
  gui/ksysguard.cpp
  gui/ksysguard.h

To: al1xz, apol
Cc: zzag, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D14436: SwitchDesktop mousewheel options with config dialog added

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> desktop.cpp:172
> +namespace {
> +void switchDesktop(bool next, bool rollover, bool invert) {
>  const int numDesktops = KWindowSystem::numberOfDesktops();

if you turn this into a method you don't need the rollover/invert args

> totto wrote in desktop.h:54
> Or, even simpler: I'll just drop the duplication between the in-class bool 
> (i.e. the the cached value), and the `m_options` config bool  and always look 
> it up in `m_options`, should be fast enough.
> 
> Btw, would a `std::shared_ptr` have been Ok for once? That does not have the 
> bug prone non-explicit bool problem and would work as expected.

> Btw, would a std::shared_ptr have been Ok for once?

TBH, I still would have considered it overkill. It's only two checkboxes, it 
doesn't need any custom design patterns.

If you did want to reduce the few lines of duplication, KConfigXT is the 
framework to look at for doing this sort of thing.

REPOSITORY
  R120 Plasma Workspace

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

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

D14478: Do not leak all instances of QMLOutput when QMLScreen is destroyed

[David Edmundson]

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


  It's good to go.

REPOSITORY
  R104 KScreen

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

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

D14478: Do not leak all instances of QMLOutput when QMLScreen is destroyed

[Roman Gilg]

romangg added a comment.


  I also thought the call to setParentItem would clear the output. But as 
@broulik says it does not delete automatically. So is this good to go? Or 
should we make QMLScreen really a Qt Object parent of the outputs?

REPOSITORY
  R104 KScreen

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

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

D12925: Parse global config files. Remove 'Vendor default' option. Fix changes not recognized.

[Julian Wolff]

progwolff added a comment.


  Big sorry for being away for so long. I had some things to do that took much 
more time than initally planned.
  Many thanks to those who fixed the bugs I introduced or created workarounds...
  
  I tried to get this patch working now. First of all, I want to say a few 
words about how this patch will remove the "vendor default" option.
  Previously "vendor default" ment that no anti-aliasing entry is written to 
the local config file. There could however be a global config file that has 
such an entry.
  As it was already said in some older comments, this is nothing special and 
many settings exist in other kcms or other applications where we can have both 
local and global config files. So, we could drop the "vendor default" option 
here, if we are able to parse the global configs.
  
  This patch tries to do this by using all config files given by 
`FcConfigGetConfigFiles`. 
  Ater parsing all the config files we know if there is a local 
anti-aliasing-entry. If there is, any changes will be written to this entry in 
the local config file. 
  If there is not a local anti-aliasing-entry, none will be created unless the 
option is actually changed in the kcm. If it is changed, a new entry is created 
in the local config file.
  If the user resets the defaults, the local entry is removed.
  
  I tested this on my machine without any problems. We should however test this 
carefully on different systems. There might be some cases I did not think about 
yet.

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff
Cc: rkflx, ngraham, mart, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D12925: Parse global config files. Remove 'Vendor default' option. Fix changes not recognized.

[Julian Wolff]

progwolff updated this revision to Diff 38757.
progwolff added a comment.


  Rebase on master. Partial rewrite.

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12925?vs=34315=38757

BRANCH
  arcpatch-D12925

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

AFFECTED FILES
  kcms/fonts/fonts.cpp
  kcms/fonts/fonts.h
  kcms/fonts/kxftconfig.cpp
  kcms/fonts/kxftconfig.h
  kcms/fonts/package/contents/ui/main.qml
  kcms/fonts/previewimageprovider.cpp

To: progwolff
Cc: rkflx, ngraham, mart, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D14478: Do not leak all instances of QMLOutput when QMLScreen is destroyed

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> davidedmundson wrote in qmlscreen.cpp:96
> this should clear them already?

`parentItem` is only visual parent, it doesn't communicate ownership

REPOSITORY
  R104 KScreen

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

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

D14478: Do not leak all instances of QMLOutput when QMLScreen is destroyed

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> qmlscreen.cpp:96
>  
>  qmloutput->setParentItem(this);
>  qmloutput->setZ(m_outputMap.count());

this should clear them already?

REPOSITORY
  R104 KScreen

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

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

D14353: Improve alignment of types

[Roman Gilg]

romangg added a comment.


  As already said on IRC and also how Vlad sees it: this optimization like for 
most of our classes is not worth it.
  
  Since we are an open source project, which strives to motivate people to 
contribute, good readability is much more important. So when ordering the class 
members we should think of our code as technical documents, whose structure 
should make it easy for new contributors to learn what the code is doing.
  
  This means that when describing a class general and more important properties 
should be at the top (e.g. here id, name, type) and more specific or less 
important ones are at the bottom (e.g. here rotation, scale). Also properties 
should be grouped if they show similarities (e.g. here pos, size, rotation, 
scale).
  
  If we have a class in our code where tighter packing really makes sense (much 
more instances), a comment in the class description should indicate this. 
Otherwise the default ordering mode should be better readability.

REPOSITORY
  R110 KScreen Library

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

To: gladhorn, #plasma, romangg
Cc: zzag, romangg, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D14437: Fix QFileDialog not remembering the last visited directory.

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in kdeplatformfiledialoghelper.cpp:118
> It works without comment, i think these connections are not needed.

Yes for this problem my patch is enough, but I was surprised to find that those 
signals were not connected. Yes it's a TODO, but unrelated to this bug.

REPOSITORY
  R135 Integration for Qt applications in Plasma

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

To: dfaure, anthonyfieroni, elvisangelaccio, plasma-devel, broulik, arichardson
Cc: apol, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, mart

D14476: QMLScreen: do not declare the engine a member

[Frederik Gladhorn]

gladhorn added inline comments.

INLINE COMMENTS

> broulik wrote in qmlscreen.h:109
> This entire custom engine handling could be removed, with `QMLScreen` using 
> `qmlEngine(this)` when creating an `QMLOutput`

I had briefly tried and got a nullptr from qmlEngine, but I didn't really spend 
time on it.

REPOSITORY
  R104 KScreen

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

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

D14426: [Folder View] Create KFilePlacesModel only when needed and listen for changes

[Eike Hein]

hein added a comment.


  Danke :)

REPOSITORY
  R119 Plasma Desktop

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

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

D14477: Do not leak Widget::ui

[Frederik Gladhorn]

This revision was automatically updated to reflect the committed changes.
Closed by commit R104:749ab17d5e48: Do not leak Widget::ui (authored by 
gladhorn).

REPOSITORY
  R104 KScreen

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14477?vs=38746=38749

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

AFFECTED FILES
  kcm/src/widget.cpp

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

D14426: [Folder View] Create KFilePlacesModel only when needed and listen for changes

[Kai Uwe Broulik]

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


  Good idea, will do

REPOSITORY
  R119 Plasma Desktop

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

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

D14476: QMLScreen: do not declare the engine a member

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> qmlscreen.h:109
>  
>  QQmlEngine* m_engine = nullptr;
>  QMLOutput *m_leftmost = nullptr;

This entire custom engine handling could be removed, with `QMLScreen` using 
`qmlEngine(this)` when creating an `QMLOutput`

REPOSITORY
  R104 KScreen

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

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

D14476: QMLScreen: do not declare the engine a member

[Roman Gilg]

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


  Note, that m_engine is set in widget.cpp. But it's just a Cpp call to 
setEngine, which is defined extra. So removing the property macro should be 
fine.

REPOSITORY
  R104 KScreen

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

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

D14426: [Folder View] Create KFilePlacesModel only when needed and listen for changes

[Eike Hein]

hein added a comment.


  Can we make the KFilePlaces model static on top of this? It could be shared 
by all FVs that need it. Look at how tasksmodel.cpp keeps a refcount of when 
ActivityInfo is in use and deletes it when it drops to 0 for example ...

REPOSITORY
  R119 Plasma Desktop

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

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

D14478: Do not leak all instances of QMLOutput when QMLScreen is destroyed

[Frederik Gladhorn]

gladhorn created this revision.
gladhorn added a reviewer: Plasma.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
gladhorn requested review of this revision.

REVISION SUMMARY
  In practice this is most likely only a leak on exit which does not
  matter, but it makes using Valgrind etc impossible.

REPOSITORY
  R104 KScreen

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

AFFECTED FILES
  kcm/src/declarative/qmlscreen.cpp
  kcm/src/declarative/qmlscreen.h

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

D14477: Do not leak Widget::ui

[Frederik Gladhorn]

gladhorn created this revision.
gladhorn added a reviewer: Plasma.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
gladhorn requested review of this revision.

REPOSITORY
  R104 KScreen

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

AFFECTED FILES
  kcm/src/widget.cpp

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

D14436: SwitchDesktop mousewheel options with config dialog added

[Thomas Otto]

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


  > disabled checkbox for rollover
  
  That was partially meant to overcome the shortcoming that changing the kwin 
value does not change this value until some settings dialog is opened, but with 
your patch this is no longer required.
  
  Plus I wanted to avoid two options for the same thing and add some kind of 
hint where this behavior could be changed. Should I still mention it as plain 
text (though translating that might be complicated) or just leave users to 
figure it out themselves? Again, "go to first config gui of this key" would be 
nice :)

INLINE COMMENTS

> davidedmundson wrote in desktop.cpp:36
> I have this coming: 
> https://phabricator.kde.org/D13034

Great, will keep an eye on that.

> davidedmundson wrote in desktop.h:54
> or just use a regular bool and copy the value in configurationAccepted
> 
> Long term, having simpler code tends to be much better than clever code.

Or, even simpler: I'll just drop the duplication between the in-class bool 
(i.e. the the cached value), and the `m_options` config bool  and always look 
it up in `m_options`, should be fast enough.

Btw, would a `std::shared_ptr` have been Ok for once? That does not have the 
bug prone non-explicit bool problem and would work as expected.

REPOSITORY
  R120 Plasma Workspace

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

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

D14476: QMLScreen: do not declare the engine a member

[Frederik Gladhorn]

gladhorn created this revision.
gladhorn added a reviewer: Plasma.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
gladhorn requested review of this revision.

REVISION SUMMARY
  There is no need for this, the only use of the engine is when creating 
QQMLOutputComponent.

REPOSITORY
  R104 KScreen

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

AFFECTED FILES
  kcm/src/declarative/qmlscreen.h

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