D27577: [KCM]Fix content below scrollbars

[George Vogiatzis]

gvgeo added a comment.


  I don't see any objections, will push to stable.

REPOSITORY
  R115 Plasma Audio Volume Applet

BRANCH
  advanced2 (branched from master)

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

To: gvgeo, #plasma, #vdg, drosca, ngraham
Cc: ngraham, apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D27734: [Applet/TaskManager]Don't create extra PulseAudio component

[George Vogiatzis]

gvgeo created this revision.
gvgeo added reviewers: Plasma, hein.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
gvgeo requested review of this revision.

REVISION SUMMARY
  Use the component already created.

TEST PLAN
  Functionality must stay the same.
  Test with D27684 's Test Plan.
  Also test without plasma-pa, no options or icon should appear,
  without any warning.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  fix (branched from master)

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

AFFECTED FILES
  applets/taskmanager/package/contents/ui/main.qml

To: gvgeo, #plasma, hein
Cc: broulik, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27723: feat(kwayland): enable the auto-rotation and tablet mode

[Bhushan Shah]

This revision was automatically updated to reflect the committed changes.
Closed by commit R110:65585290f109: feat(kwayland): enable the auto-rotation 
and tablet mode (authored by bshah).

REPOSITORY
  R110 KScreen Library

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27723?vs=76640=7

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

AFFECTED FILES
  backends/kwayland/waylandconfig.cpp

To: bshah, romangg, davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27509: Introduce ProcessDataModel

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> broulik wrote in extended_process_list.cpp:394
> This could have used a `reserve` call :)

not a meaningful one, each provider can add provide many attributes

REPOSITORY
  R111 KSysguard Library

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

To: davidedmundson, #plasma
Cc: ahiemstra, broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D27509: Introduce ProcessDataModel

[David Edmundson]

davidedmundson updated this revision to Diff 76653.
davidedmundson marked 15 inline comments as done.
davidedmundson added a comment.


  some but not all review comments

REPOSITORY
  R111 KSysguard Library

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27509?vs=76011=76653

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  processcore/CMakeLists.txt
  processcore/extended_process_list.cpp
  processcore/extended_process_list.h
  processcore/process_attribute.cpp
  processcore/process_attribute.h
  processcore/process_attribute_model.cpp
  processcore/process_attribute_model.h
  processcore/process_data_model.cpp
  processcore/process_data_model.h
  processui/ProcessModel.cpp

To: davidedmundson, #plasma
Cc: ahiemstra, broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D27509: Introduce ProcessDataModel

[Arjen Hiemstra]

ahiemstra added inline comments.

INLINE COMMENTS

> broulik wrote in process_data_model.cpp:278
> Superfluous; or have it return `name()` instead

Oops. Both this one and the one in data() should return name() if shortName() 
is empty.

REPOSITORY
  R111 KSysguard Library

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

To: davidedmundson, #plasma
Cc: ahiemstra, broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D27509: Introduce ProcessDataModel

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> broulik wrote in process_data_model.h:53
> should this just be `Qt::DisplayRole` given you return for both in `data`?

It breaks our roleNames as you can't have two names for the same value.

Then on the QML side we can't interchange with display and formattedValue. 
which is what we were trying to achieve

REPOSITORY
  R111 KSysguard Library

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

To: davidedmundson, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27509: Introduce ProcessDataModel

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> extended_process_list.cpp:62
> +if (m_changeFlag != 0) {
> +connect(parent, ::processChanged, this, 
> [=](KSysGuard::Process *process) {
> +if (!process->changes().testFlag(m_changeFlag)) {

Capture only `[this]`

> extended_process_list.cpp:93
>  
> +auto pidSensor = new ProcessSensor(this, 
> QStringLiteral("pid"), i18n("PID"), ::Process::pid, 
> KSysGuard::Process::Status, ForwardFirstEntry);
> +pidSensor->setDescription(i18n("The unique Process ID that identifies 
> this process."));

Isn't the `typename` auto-deduced/implied? Doesn't hurt though.

> extended_process_list.cpp:109
> +this, QStringLiteral("username"), i18n("Username"), 
> [](KSysGuard::Process *p) {
> +KUser user(p->uid());
> +QString username;

Does this need caching?

> extended_process_list.cpp:122
> +K_UID uid = p->uid();
> +if (uid == 65534) {
> +return false;

Add a comment that this magic number is `nobody`

> extended_process_list.cpp:354
> +ioCharactersActuallyReadRateSensor->setUnit(KSysGuard::UnitByteRate);
> +ioCharactersActuallyReadRateSensor->setShortName(i18n("Read"));
> +ioCharactersActuallyReadRateSensor->setDescription(i18n("The rate of 
> data being read from disk."));

Do any of the sensor names need / used to have i18n context?

> extended_process_list.cpp:394
>  {
>  QVector rc;
>  for (auto p : qAsConst(d->m_providers)) {

This could have used a `reserve` call :)

> process_data_model.cpp:1
> +#include "process_data_model.h"
> +#include "formatter.h"

Missing copyright header

> process_data_model.cpp:56
> +{
> +connect(m_processes, ::Processes::beginAddProcess, this, 
> ::beginInsertRow);
> +connect(m_processes, ::Processes::endAddProcess, this, 
> ::endInsertRow);

Does `ProcessDataModelPrivate` really need to be a `QObject`? Can't you connect 
it to `q` with a lambda?

> process_data_model.cpp:63
> +for (auto attr : attributes) {
> +m_availableAttributes[attr->id()] = attr;
> +}

Add `reserve()`

> process_data_model.cpp:145
> +void ProcessDataModel::setEnabledAttributes(const QStringList 
> )
> +{
> +beginResetModel();

if (d->enabledAttributes == enabledAttributes) {
   return;
  }

?

> process_data_model.cpp:183
> +{
> +if (row < 0)
> +return QModelIndex();

Coding style

> process_data_model.cpp:190
> +return QModelIndex();
> +if (d->m_processes->processCount() <= row)
> +return QModelIndex();

Remove equals, or better turn it around to match everone else around it:

  if (row >= d->m_processes->processCount()) {
  return QModelIndex();
  }

> process_data_model.cpp:254
> +
> +QMetaEnum e = 
> metaObject()->enumerator(metaObject()->indexOfEnumerator("AdditionalRoles"));
> +

There is:

  QMetaEnum::fromType();

> process_data_model.cpp:278
> +case ShortName: {
> +if (!attribute->shortName().isEmpty()) {
> +return attribute->shortName();

Superfluous; or have it return `name()` instead

> process_data_model.h:33
> +/**
> + * This class contains contains a model of all running processes
> + * Rows represent processes

Remove second "contains"

> process_data_model.h:48
> +Q_PROPERTY(QStringList enabledAttributes READ enabledAttributes WRITE 
> setEnabledAttributes NOTIFY enabledAttributesChanged)
> +Q_PROPERTY(QObject *attributesModel READ attributesModel CONSTANT)
> +

would it hurt to make this a `QAbstractItemModel *`?

> process_data_model.h:53
> +Value = Qt::UserRole, /// The raw value of the attribute. This is 
> unformatted and could represent an int, real or string
> +FormattedValue, /// A string containing the value in a locale 
> friendly way with appropriate suffix "eg. 5Mb" "20%"
> +

should this just be `Qt::DisplayRole` given you return for both in `data`?

> process_data_model.h:66
> +
> +ProcessDataModel(QObject *parent = nullptr);
> +~ProcessDataModel() override;

`explicit`

> process_data_model.h:104
> +private:
> +QScopedPointer d;
> +friend class ProcessDataModelPrivate;

Why is this not a nested `Private` class like in the extended processes list?

REPOSITORY
  R111 KSysguard Library

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

To: davidedmundson, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27723: feat(kwayland): enable the auto-rotation and tablet mode

[Bhushan Shah]

bshah created this revision.
bshah added reviewers: romangg, davidedmundson.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
bshah requested review of this revision.

REVISION SUMMARY
  Related kwin_wayland patches have been landed for rotation, so enable
  the relevant features.
  
  Depends on: D26036 

TEST PLAN
  tested on Dell Inspiron 13 7xxx series laptop with mentioned kwin change 
applied

REPOSITORY
  R110 KScreen Library

BRANCH
  bshah/autorotate

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

AFFECTED FILES
  backends/kwayland/waylandconfig.cpp

To: bshah, romangg, davidedmundson
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24706: [RFC] Change button style

[Noah Davis]

ndavis added a comment.


  In D24706#619482 , @mart wrote:
  
  > hmm, buttons look quite.. flat now? there was a lot of discussion on what 
should be flat and what not. while i think having things a bit flatter doesn't 
actually hurt, could this be considered an usability issue?
  
  
  I think it's only style issue as long as we have either good enough contrast 
on the background or good enough contrast on the outline. I'm personally not 
100% into the flat look, but it's hard to get the gradient right and I don't 
like the current gradient.

REPOSITORY
  R31 Breeze

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

To: ndavis, #vdg, #breeze
Cc: mart, ahiemstra, cfeck, The-Feren-OS-Dev, cblack, bodoeggert, ngraham, 
plasma-devel, manueljlin, Orage, LeGast00n, konkinartem, ian, jguidon, Ghost6, 
jraleigh, zachus, MrPepe, fbampaloukas, squeakypancakes, alexde, 
IohannesPetros, GB_2, trickyricky26, ragreen, crozbo, ndavis, ZrenBot, firef, 
skadinna, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, aaronhoneycutt, 
abetts, sebas, apol, mbohlender

D24706: [RFC] Change button style

[Marco Martin]

mart added a comment.


  hmm, buttons look quite.. flat now? there was a lot of discussion on what 
should be flat and what not. while i think having things a bit flatter doesn't 
actually hurt, could this be considered an usability issue?

REPOSITORY
  R31 Breeze

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

To: ndavis, #vdg, #breeze
Cc: mart, ahiemstra, cfeck, The-Feren-OS-Dev, cblack, bodoeggert, ngraham, 
plasma-devel, manueljlin, Orage, LeGast00n, konkinartem, ian, jguidon, Ghost6, 
jraleigh, zachus, MrPepe, fbampaloukas, squeakypancakes, alexde, 
IohannesPetros, GB_2, trickyricky26, ragreen, crozbo, ndavis, ZrenBot, firef, 
skadinna, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, aaronhoneycutt, 
abetts, sebas, apol, mbohlender

D27458: Set a better position for Krunner in wayland

[Tranter Madi]

trmdi closed this revision.

REPOSITORY
  R120 Plasma Workspace

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

To: trmdi, #plasma, davidedmundson
Cc: meven, apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D27458: Set a better position for Krunner in wayland

[Tranter Madi]

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


  Rename m_realVisible to m_requestedVisible

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27458?vs=76597=76629

BRANCH
  arcpatch-D27458 (branched from master)

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

AFFECTED FILES
  krunner/view.cpp
  krunner/view.h
  shell/strutmanager.cpp
  shell/strutmanager.h

To: trmdi, #plasma, davidedmundson
Cc: meven, apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D25324: Display a warning, if xsettingsd is not found

[Mikhail Zolotukhin]

gikari updated this revision to Diff 76627.
gikari added a comment.


  Add XSettingsd find module

REPOSITORY
  R99 KDE Gtk Configuration Tool

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25324?vs=69795=76627

BRANCH
  xsettingsd-dependency (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  cmake/modules/FindXSettingsd.cmake

To: gikari
Cc: eszlari, broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27721: globalaccel: allow sleep and hibernate keyboard shortcuts

[Bhushan Shah]

bshah created this revision.
bshah added a reviewer: Plasma.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
bshah requested review of this revision.

REVISION SUMMARY
  This is useful for the phone specially. You can quickly turn on-off your
  phone from lockscreen without actually unlocking it.

TEST PLAN
  I need help testing it as my prefix session is broken currently.

REPOSITORY
  R133 KScreenLocker

BRANCH
  bshah/allow-sleep-hibernate

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

AFFECTED FILES
  globalaccel.cpp

To: bshah, #plasma
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27458: Set a better position for Krunner in wayland

[David Edmundson]

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


  Lets go for it.

INLINE COMMENTS

> view.cpp:411
> +{
> +m_realVisible = visible;
> +

this is a very confusing name.

It refers to the visible state krunner has set, but most importantly NOT the 
state of the QWindow (which is arguably the real state)

REPOSITORY
  R120 Plasma Workspace

BRANCH
  arcpatch-D27458 (branched from master)

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

To: trmdi, #plasma, davidedmundson
Cc: meven, apol, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D27711: [Applet]Use gridLayout for details

[George Vogiatzis]

gvgeo abandoned this revision.
gvgeo added a comment.


  In favor of kirigami form.

REPOSITORY
  R116 Plasma Network Management Applet

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

To: gvgeo, #plasma, jgrulich, ngraham
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart