D28349: Fix Warnings

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> bruns wrote in kdirwatch.cpp:946
> m_nfsPreferredMethod defaults to `KDirWatch::FAM`, i.e. it will crash below 
> now on NFS when FAM is disabled.

So either change the default, or add a Q_FALLTHROUH() ?

REPOSITORY
  R244 KCoreAddons

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

To: meven, #frameworks, davidedmundson
Cc: bruns, iasensio, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham

D28274: [KFontChooser] Add a checkbox to toggle showing only monospaced fonts

[Benjamin Port]

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

INLINE COMMENTS

> kfontchooser.cpp:347
>  
>  // Completed the font attributes grid
>  

This comment is still valid ?

> kfontchooser.cpp:381
> +if (flags & ShowDifferences) { // In this mode follow the state of the 
> familyCheckbox
> +connect(familyCheckbox, &QAbstractButton::toggled, q, [this](const 
> bool state) {
> +onlyFixedCheckbox->setEnabled(state);

I think there you can connect directly to setEnabled, you don't need a lambda

REPOSITORY
  R236 KWidgetsAddons

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

To: ahmadsamir, #frameworks, dfaure, cfeck, apol, bport
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28457: kdirwatch: fix a recently introduced crash

[Méven Car]

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

REVISION SUMMARY
  Prevent a crash when browsing nfs without FAM installed introduced in D28349 


REPOSITORY
  R244 KCoreAddons

BRANCH
  master

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

AFFECTED FILES
  src/lib/io/kdirwatch.cpp

To: meven, bruns, #frameworks, iasensio
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28349: Fix Warnings

[Ismael Asensio]

iasensio added a comment.


  In D28349#638395 , @bruns wrote:
  
  > @iasensio - are you using NFS?
  
  
  It's a NTFS partition using `fuseblk`, but anyway D28457 
 fixes the crash

REPOSITORY
  R244 KCoreAddons

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

To: meven, #frameworks, davidedmundson
Cc: bruns, iasensio, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham

D28457: kdirwatch: fix a recently introduced crash

[Ismael Asensio]

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


  Thanks! It fixes `BUG: 419428`

REPOSITORY
  R244 KCoreAddons

BRANCH
  master

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

To: meven, bruns, #frameworks, iasensio
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28372: Added a merged look to the plasmoidheading and remove roundedborders

[Niccolò Venerandi]

niccolove added a subscriber: manueljlin.
niccolove added a comment.


  Generally speaking, I'd still like for all applets to have a row of 
plasmoidheading on the bottom of system tray's own because having just system 
tray plasmoidheading, especially with the new small font, does not feel 
"enough" to me; if you look at every @manueljlin mockups, you'll see they have 
two rows of heading. I think it's because there's a visual minimum 
(plasmoidheading area / normal background area) ratio to look pretty. To 
clarify, I think this is not ideal:
  F8207914: image.png 
  While I think this looks much better:
  F8207917: image.png 
  YMMV. What do you think?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #plasma
Cc: manueljlin, ahiemstra, ndavis, ngraham, mart, davidedmundson, 
kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, bruns

D28388: kio_file: honour KIO::StatResolveSymlink for UDS_DEVICE_ID and UDS_INODE

[Ahmad Samir]

ahmadsamir added a comment.


  In D28388#637419 , @dfaure wrote:
  
  > In D28388#637417 , @ahmadsamir 
wrote:
  >
  > > (Now the other issue, "DEVICE" (from the kproperties patch) is always 
"8", whether I use ~/.bashrc, /usr/bin/file, or some file on a usb stick. But 
that's not really related to this diff).
  >
  >
  > That's really odd. And what does `stat` say?
  >  E.g. I get 
  >  Device: fe01h/65025d
  >  on one partition, and
  >  Device: fe04h/65028d
  >  on another. Those decimal values match the debug output from that 
kpropertiesdialog patch.
  
  
  Sorry for the delay; it turns out statx is available on my system, so 
'stat_dev(buff)' called:
  
inline static uint16_t stat_mode(struct statx &buf) { return 
buf.stx_dev_major; }
  
  which is always 8 on my system.
  
  I propose changing it to combine stx_dev_major and stx_dev_minor:
  
inline static uint32_t stat_dev(struct statx &buf)
{
return (buf.stx_dev_major * 100) + buf.stx_dev_minor;
}
  
  so for example:
  
$ stat /usr/bin/file | grep Device
Device: 804h/2052d  Inode: 9168Links: 1
$ stat ~/.bashrc | grep Device
Device: 803h/2051d  Inode: 97  Links: 1
  
  "DEVICE" (from kpropertiesdialog) would be, respectively:
  
804
803
  
  I am not an expert on these low-level stat calls, but that seems to make 
sense to me anyway :)

REPOSITORY
  R241 KIO

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

To: dfaure, trmdi, ahmadsamir, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28460: Add KCModuleStateProbe as base class for plugin

[Benjamin Port]

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

REVISION SUMMARY
  This class will allow to get state of a module without loading the UI.

REPOSITORY
  R295 KCMUtils

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

AFFECTED FILES
  src/CMakeLists.txt
  src/kcmoduleloader.cpp
  src/kcmoduleloader.h
  src/kcmodulestateprobe.cpp
  src/kcmodulestateprobe.h

To: bport, #plasma, ervin
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28460: Add KCModuleStateProbe as base class for plugin

[Benjamin Port]

bport added a dependent revision: D28461: In sidebar mode show if a module is 
in default state or not.

REPOSITORY
  R295 KCMUtils

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

To: bport, #plasma, ervin
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28460: Add KCModuleStateProbe as base class for plugin

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> kcmoduleloader.cpp:165
> +
> +if (!mod.library().isEmpty()) {
> +QString error;

Use an early return here

> kcmoduleloader.cpp:176
> +if (factory) {
> +auto p = factory->create(nullptr, args2);
> +if (p) {

This looks like it leaks

> kcmoduleloader.cpp:182
> +
> +auto p = mod.service()->createInstance(nullptr, 
> args2, &error);
> +if (p) {

Error can be `nullptr`. Either print a warning or remove it

REPOSITORY
  R295 KCMUtils

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

To: bport, #plasma, ervin
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28463: do not install testengine

[Harald Sitter]

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

REVISION SUMMARY
  by default users have to opt out of BUILD_TESTING meaning everyone would
  by default build and get testengine installed even though it serves no
  purpose in production. this also includes distros as I've noticed :O
  
  do not install the engine, same as testplugin isn't getting installed.
  
  (I am actually thinking throwing it away as a whole may make sense; it
   serves no real purpose over any other engine)

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

AFFECTED FILES
  tests/testengine/CMakeLists.txt

To: sitter, mart
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28457: kdirwatch: fix a recently introduced crash

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> kdirwatch.cpp:956
>  #else
> -case KDirWatch::FAM: Q_UNREACHABLE(); break;
> +case KDirWatch::FAM: break;
>  #endif

Can you change that to `case KDirWatch::FAM: entryAdded = false; break` to make 
it a little bit more explicit?

> kdirwatch.cpp:959
>  #if HAVE_SYS_INOTIFY_H
>  case KDirWatch::INotify: entryAdded = useINotify(e); break;
>  #endif

bonus points for adding

  #else
  case KDirWatch::INotify: entryAdded = false; break;

here (otherwise the switch may be incomplete), dito for QFSWatch below.

REPOSITORY
  R244 KCoreAddons

BRANCH
  master

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

To: meven, bruns, #frameworks, iasensio
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28460: Add KCModuleStateProbe as base class for plugin

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> kcmodulestateprobe.h:21
> +
> +#ifndef KCMODULEDEFAULT_H
> +#define KCMODULEDEFAULT_H

`KCMODULESTATEPROBE_H`

> kcmodulestateprobe.h:25
> +#include 
> +#include 
> +#include 

Forward-declare

> kcmodulestateprobe.h:39
> +virtual bool isDefaults() const = 0;
> +};
> +

Please add a `virtual_hook` so we can extend this class in the future without 
breaking ABI should we have the need to extract more data out of a settings 
module:

  virtual void virtual_hook(int id, void *data)

REPOSITORY
  R295 KCMUtils

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

To: bport, #plasma, ervin
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28220: Switch to using Kirigami's ShadowedRectangle

[Dan Leinir Turthra Jensen]

leinir added a comment.


  Ping? Not super critical, i guess, but it just feels sad to have things 
pending for weeks...

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #frameworks, #plasma, ahiemstra, broulik, mart, #vdg
Cc: davidedmundson, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, bruns

D28274: [KFontChooser] Add a checkbox to toggle showing only monospaced fonts

[Ahmad Samir]

ahmadsamir updated this revision to Diff 78979.
ahmadsamir added a comment.


  Don't use a lambda in connetc() where it's not needed

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28274?vs=78441&id=78979

BRANCH
  l-monospace-checkbox

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

AFFECTED FILES
  src/kfontchooser.cpp

To: ahmadsamir, #frameworks, dfaure, cfeck, apol, bport
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28274: [KFontChooser] Add a checkbox to toggle showing only monospaced fonts

[Ahmad Samir]

ahmadsamir marked an inline comment as done.
ahmadsamir added inline comments.

INLINE COMMENTS

> bport wrote in kfontchooser.cpp:347
> This comment is still valid ?

Yes, that's after setting the font family/style/size list views and the size 
DoubleSpinBox.

REPOSITORY
  R236 KWidgetsAddons

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

To: ahmadsamir, #frameworks, dfaure, cfeck, apol, bport
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28271: [KFontChooser] More code cleanup

[Ahmad Samir]

ahmadsamir planned changes to this revision.

REPOSITORY
  R236 KWidgetsAddons

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

To: ahmadsamir, #frameworks, dfaure, cfeck, apol, bport
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28271: [KFontChooser] More code cleanup

[Ahmad Samir]

ahmadsamir updated this revision to Diff 78983.
ahmadsamir added a comment.


  - Simplify connect() calls
  - Move the connect() calls of the various checkboxes, in ShowDifferences 
mode, after the widgets they enable/disable have been created

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28271?vs=78433&id=78983

BRANCH
  l-kfontchooser-3 (branched from master)

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

AFFECTED FILES
  src/kfontchooser.cpp

To: ahmadsamir, #frameworks, dfaure, cfeck, apol, bport
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28463: do not install testengine

[Aleix Pol Gonzalez]

apol added a comment.


  How is it going to be tested then?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: sitter, mart
Cc: apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28463: do not install testengine

[Kai Uwe Broulik]

broulik added a comment.


  It's not used by any tests either

REPOSITORY
  R242 Plasma Framework (Library)

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

To: sitter, mart
Cc: broulik, apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D28463: do not install testengine

[Aleix Pol Gonzalez]

apol 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/D28463

To: sitter, mart, apol
Cc: broulik, apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D28271: [KFontChooser] More code cleanup

[Ahmad Samir]

ahmadsamir updated this revision to Diff 78986.
ahmadsamir added a comment.


  Move family checkbox and listview code next to each other

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28271?vs=78983&id=78986

BRANCH
  l-kfontchooser-3 (branched from master)

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

AFFECTED FILES
  src/kfontchooser.cpp

To: ahmadsamir, #frameworks, dfaure, cfeck, apol, bport
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28444: WIP/RFC: Add ECMCargo module

[Carson Black]

cblack added a comment.


  In D28444#638540 , @IlyaBizyaev 
wrote:
  
  > In D28444#638430 , @cblack wrote:
  >
  > > more than half of Ikona's CMakeLists.txt is boilerplate dedicated to 
integrating Rust with the rest of the project
  >
  >
  > I don't understand why your function needs a list of Rust files, even 
though those are already handled by Cargo. Looks boilerplate-ish...
  
  
  CMake needs to know when the Rust source has changed so it can rebuild it

REPOSITORY
  R240 Extra CMake Modules

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

To: cblack, #frameworks, #build_system
Cc: apol, IlyaBizyaev, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
cblack, GB_2, bencreasy, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[David Edmundson]

davidedmundson added a comment.


  What do we want to happen for released code that gets a bugfix update?

INLINE COMMENTS

> kconfigdialogmanager.cpp:609
> +const auto defaultValue = [item] {
> +item->swapDefault();
> +const auto value = item->property();

Why not item->readDefault()?

> kconfigdialogmanager.cpp:625
> +q->setProperty(widget, defaultValue);
> +updateWidgetIndicator(configId, widget);
> +emit q->widgetModified();

won't it do it itself when the property changes?

> settingsstatusindicator.cpp:75
> +setFocusPolicy(Qt::NoFocus);
> +show();
> +}

> This is equivalent to calling showFullScreen(), showMaximized(), or 
> setVisible(true), depending on the platform's default behavior for the window 
> flags.

For X and wayland it's setVisible(true)

but we shouldn't count on it.

> settingsstatusindicator.cpp:175
> +const auto leftToRight = m_trackedWidget->isLeftToRight();
> +auto x = leftToRight ? m_trackedWidget->pos().x() + 
> m_trackedWidget->width()
> + : m_trackedWidget->pos().x() - width();

unused?

> settingsstatusindicator.cpp:184
> +const auto re = QRegularExpression("^kcfg_");
> +const auto children = 
> m_trackedWidget->parentWidget()->findChildren(re, 
> Qt::FindDirectChildrenOnly);
> +const auto xValues = [=] {

Can we be sure the tracked widget always has a parent widget?

If someone doesn't use layouts a widget might not have a parent.

> settingsstatusindicator.cpp:192
> +   const auto localX = leftToRight ? 
> widgetExpectedWidth(w) : -width();
> +   return w->pos().x() + localX;
> +   });

that's not true for the RTL case where the widget is expected to resize.

It would be   w->pos().x() + w->width() - widgetExpectedWidth(w)

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D28466: Added Page element

[Niccolò Venerandi]

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

REVISION SUMMARY
  Page element was missing. I added it.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/plasmacomponents3/Page.qml
  src/declarativeimports/plasmacomponents3/qmldir

To: niccolove
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28466: Added Page element

[Niccolò Venerandi]

niccolove added a reviewer: Plasma.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #plasma
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28466: Added Page element

[Niccolò Venerandi]

niccolove added a dependent revision: D28467: Converted to Page with a 
PlasmodHeading in the heading.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #plasma
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28388: kio_file: honour KIO::StatResolveSymlink for UDS_DEVICE_ID and UDS_INODE

[Méven Car]

meven added a comment.


  In D28388#638722 , @ahmadsamir 
wrote:
  
  > ...
  
  
  Open a diff to have a focused conversation.

REPOSITORY
  R241 KIO

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

To: dfaure, trmdi, ahmadsamir, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27540: KCModule: Indicate when a setting has been changed from the default or previous value

[Noah Davis]

ndavis added a comment.


  Somehow I missed the notification that this was updated.
  
  Thanks for the horizontal alignment. Could you also add a left margin to the 
column of reset buttons? It should be the same as the spacing between the 
labels and the controls, which is `Kirigami.Units.smallSpacing`, IIRC.

REPOSITORY
  R265 KConfigWidgets

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

To: ervin, ngraham, davidedmundson, meven, crossi, bport, #vdg, ndavis, broulik
Cc: alexde, ndavis, iasensio, davidre, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D28457: kdirwatch: fix a recently introduced crash

[Méven Car]

meven edited the summary of this revision.

REPOSITORY
  R244 KCoreAddons

BRANCH
  master

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

To: meven, bruns, #frameworks, iasensio
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28444: WIP/RFC: Add ECMCargo module

[Ilya Bizyaev]

IlyaBizyaev added a comment.


  In D28444#638877 , @cblack wrote:
  
  > CMake needs to know when the Rust source has changed so it can rebuild it
  
  
  Changes to Rust code need to be tracked by Cargo, not by CMake

REPOSITORY
  R240 Extra CMake Modules

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

To: cblack, #frameworks, #build_system
Cc: apol, IlyaBizyaev, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
cblack, GB_2, bencreasy, michaelh, ngraham, bruns

D28457: kdirwatch: fix a recently introduced crash

[Méven Car]

meven updated this revision to Diff 79002.
meven marked 2 inline comments as done.
meven added a comment.


  Add entryAdded = false to make things explicit and prevent warnings

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28457?vs=78958&id=79002

BRANCH
  master

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

AFFECTED FILES
  src/lib/io/kdirwatch.cpp

To: meven, bruns, #frameworks, iasensio
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28388: kio_file: honour KIO::StatResolveSymlink for UDS_DEVICE_ID and UDS_INODE

[Ahmad Samir]

ahmadsamir added a comment.


  In D28388#638986 , @meven wrote:
  
  > In D28388#638722 , @ahmadsamir 
wrote:
  >
  > > ...
  >
  >
  > Open a diff to have a focused conversation.
  
  
  Good point. Thanks. :)

REPOSITORY
  R241 KIO

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

To: dfaure, trmdi, ahmadsamir, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28466: Added Page element

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> Page.qml:2
> +/*
> + *   Copyright 2016 Niccolò Venerandi 
> + *

when did you add it?

> Page.qml:24
> +T.Page {
> +implicitWidth: Math.max(background ? background.implicitWidth : 0,
> +(contentItem ? contentItem.implicitWidth : 0) + 
> leftPadding + rightPadding)

what background?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #plasma
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D28466: Added Page element

[Niccolò Venerandi]

niccolove marked 2 inline comments as done.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #plasma
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D28466: Added Page element

[Niccolò Venerandi]

niccolove updated this revision to Diff 79004.
niccolove added a comment.


  Used correct year and implicitWidth/Height code

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28466?vs=78994&id=79004

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/plasmacomponents3/Page.qml
  src/declarativeimports/plasmacomponents3/qmldir

To: niccolove, #plasma
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D28457: kdirwatch: fix a recently introduced crash

[Stefan Brüns]

bruns accepted this revision.
bruns added a comment.


  LGTM, thanks

REPOSITORY
  R244 KCoreAddons

BRANCH
  master

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

To: meven, bruns, #frameworks, iasensio
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28466: Added Page element

[Nathaniel Graham]

ngraham added a comment.


  Could you add some explanation regarding what this is for? Ideally, both in 
the phab patch and also inline, as API docs.
  
  Also shouldn't this be in PlasmaExtras?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #plasma
Cc: ngraham, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, bruns

D28466: Added Page element

[Niccolò Venerandi]

niccolove added a comment.


  In D28466#639011 , @ngraham wrote:
  
  > Could you add some explanation regarding what this is for? Ideally, both in 
the phab patch and also inline, as API docs.
  >
  > Also shouldn't this be in PlasmaExtras?
  
  
  I'll explain here first to make sure it's correct: from what I was able to 
understand talking to Marco, plasmacomponents3 are QtQuick components with 
plasma style. In this case there is not a svg for a page in the desktoptheme, 
so this effectively just behaves like a normal Page component.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #plasma
Cc: ngraham, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, bruns

D28466: Added Page element

[Nathaniel Graham]

ngraham added a comment.


  In D28466#639018 , @niccolove 
wrote:
  
  > In D28466#639011 , @ngraham 
wrote:
  >
  > > Could you add some explanation regarding what this is for? Ideally, both 
in the phab patch and also inline, as API docs.
  > >
  > > Also shouldn't this be in PlasmaExtras?
  >
  >
  > I'll explain here first to make sure it's correct: from what I was able to 
understand talking to Marco, plasmacomponents3 are QtQuick components with 
plasma style. In this case there is not a svg for a page in the desktoptheme, 
so this effectively just behaves like a normal Page component.
  
  
  Ah right, yeah. PC3 is indeed correct then. My bad!

REPOSITORY
  R242 Plasma Framework (Library)

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

To: niccolove, #plasma
Cc: ngraham, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, bruns

D28444: WIP/RFC: Add ECMCargo module

[Carson Black]

cblack added a comment.


  In D28444#638988 , @IlyaBizyaev 
wrote:
  
  > In D28444#638877 , @cblack wrote:
  >
  > > CMake needs to know when the Rust source has changed so it can rebuild it
  >
  >
  > Changes to Rust code need to be tracked by Cargo, not by CMake
  
  
  CMake needs to know what files Cargo wants to build in order to invoke it 
only when Rust files change. There's no reason to invoke Cargo every time 
`make` is ran when CMake has the ability to keep track of dirty files.

REPOSITORY
  R240 Extra CMake Modules

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

To: cblack, #frameworks, #build_system
Cc: apol, IlyaBizyaev, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
cblack, GB_2, bencreasy, michaelh, ngraham, bruns

D28444: WIP/RFC: Add ECMCargo module

[Ilya Bizyaev]

IlyaBizyaev added a comment.


  In D28444#639020 , @cblack wrote:
  
  > CMake needs to know what files Cargo wants to build in order to invoke it 
only when Rust files change. There's no reason to invoke Cargo every time 
`make` is ran when CMake has the ability to keep track of dirty files.
  
  
  I disagree here; I'd rather invoke the tool that is meant to do the job than
  a) reimplement tracking in CMake;
  b) duplicate file lists for no real win.
  Extra concerns for the developer, barely any difference for the machine.

REPOSITORY
  R240 Extra CMake Modules

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

To: cblack, #frameworks, #build_system
Cc: apol, IlyaBizyaev, kde-frameworks-devel, kde-buildsystem, LeGast00n, 
cblack, GB_2, bencreasy, michaelh, ngraham, bruns

D28466: Added Page element

[Nathaniel Graham]

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


  LGTM but make sure @davidedmundson or another #plasma 
 person agrees.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

To: niccolove, #plasma, ngraham
Cc: ngraham, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, bruns

D28470: WIP: IconItem: Refactor source handling for different types

[Konrad Materka]

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

REVISION SUMMARY
  There are 3 possible strategies: QIcon, QImage and SVG. This change moves 
logic of each of these strategies into separate class.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

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

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: kde-frameworks-devel, #plasma, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28470: WIP: IconItem: Refactor source handling for different types

[Konrad Materka]

kmaterka added a comment.


  Do you think it is a good direction?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: kmaterka, #plasma, broulik, apol, davidedmundson
Cc: kde-frameworks-devel, #plasma, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns