Re: Review Request 126651: Fix wrong documentation for KIconTheme::iconPath

[Holger Kaelberer]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126651/
---

(Updated Jan. 6, 2016, 8:28 a.m.)


Review request for KDE Frameworks.


Repository: kiconthemes


Description (updated)
---

name has to be passed \*with\* extension


Diffs
-

  src/kicontheme.h ca04879 

Diff: https://git.reviewboard.kde.org/r/126651/diff/


Testing
---

kapidox output looks fine


Thanks,

Holger Kaelberer

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 126651: Fix wrong documentation for KIconTheme::iconPath

[Holger Kaelberer]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126651/
---

Review request for KDE Frameworks.


Repository: kiconthemes


Description
---

name has to be passed *with* extension


Diffs
-

  src/kicontheme.h ca04879 

Diff: https://git.reviewboard.kde.org/r/126651/diff/


Testing
---

kapidox output looks fine


Thanks,

Holger Kaelberer

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126650: [WIP] Add PM/ScreenSaver Inhibition capabilities to KIdleTime

[Kai Uwe Broulik]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126650/#review90697
---


Nice!

I'm wondering how could add additional platform support to this API later.


autotests/fakePMServer.cpp (line 23)


PowerDevil never returns the same cookie more than once, ie. it's just 
++m_cookieId;



autotests/inhibition_test.cpp (line 60)


Does this test work in a normal Plasma session? There's already PowerDevil 
claiming those services



autotests/inhibition_test.cpp (line 89)


Perhaps we should split the enum Requested into Activating and 
Deactivating, or at least "Busy"/BeingProcessed or so, Requsted for me implies 
an "activation requested"



autotests/inhibition_test.cpp (line 134)


Please also add a "delete i;" test to verify that automatic unregistration 
on deletion also works



src/inhibition.h (line 31)


I think we should reserve 0x00 as "no type" or similar



src/inhibition.h (lines 32 - 34)


Please use proper doxygen comments for describing enum values:
InhibitScreen = 0x01, ///< Prevents screen turning off, ...



src/inhibition.h (line 33)


I think they should build upon each other

Screen = Suspend | 0x02,
PM = Suspend | Screen | 0x04,
All = Suspend | Screen | PM



src/inhibition.h (line 49)


Or just comment here that "this destroys the object and releases the 
inhibition"



src/inhibition.h (line 54)


This is standard QObject behavior but I would still mention that when the 
object is destroyed, the inhibition will be released



src/inhibition.h (line 66)


but deleting it will still release the inhibition, right?



src/inhibition.h (lines 105 - 106)


Then you'll have two inhibitions, one of which will no longer be managed by 
this object. You need to deactivate first.



src/inhibition.h (line 122)


Do we use Q_NULLPTR in Frameworks?



src/inhibition.cpp (lines 38 - 40)


initializer list?

Private()
: status(Inhibition::Inactive)
, ...

Also, you're leaving most member variables uninitialized



src/inhibition.cpp (line 83)


This blocks, doesn't it?



src/inhibition.cpp (line 144)


If I didn't change anything but status was Revoked or Failed I would not be 
able to activate it again?



src/inhibition.cpp (line 156)


"reason" is not a good name for this



src/inhibition.cpp (line 160)


You would need to set both states

1 ("InterruptSession") and 4 ("ChangeScreenSettings") which would be 5 in 
the end.

I think there's no point in allowing to keep the screen on but then have 
the system auto suspend after a while.



src/inhibition.cpp (line 186)


What about PM? See above, if they were cumulative this check would be fine.

Perhaps we need a way to communicate to the user whether fine-grained types 
control is supported or not, so I could refrain from posting an inhibition if I 
know it'll be overkill for the situation I want, or so.



src/inhibition.cpp (line 211)


Oh, so this is a separate call? Then we should probably also split this in 
the API, so we have a "keep screen on" and "lock screen" type? Martin G?

However if I watch a movie I want neither the screen to turn off nor the 
screen to lock.



src/inhibition.cpp (line 228)


Depending on which call (the PM or ScreenSaver) call returns later this 
status will be returned by the API.



src/inhibition.cpp (line 238)


We need to handle the case of "activate but then the object goes away", we 
will have a spurious inhibition in that case. PowerDevil tracks when an 
application closes but it obviously cannot detect this case.

Re: Review Request 126385: PartManager: stop tracking a widget even if it is no longer a top level window

[Jonathan Marten]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126385/
---

(Updated Jan. 6, 2016, 6:04 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Changes
---

Submitted with commit 5ac5dfb28014035e3704ae8bf8076001e5955ceb by Jonathan 
Marten to branch master.


Bugs: 355711
https://bugs.kde.org/show_bug.cgi?id=355711


Repository: kparts


Description
---

This appears to be the cause of a crash when exiting System Settings.  More 
information in the bug report, but basically what happens is that the manager 
keeps track of widgets that it is managing in d->m_managedTopLevelWidgets.  If 
a widget is a top level widget when it is added, but is no longer top level 
when it is destroyed, it is not removed from the list which results in an 
access-to-deleted-object in the destructor.

This change unconditionally removes the widget even if it is no longer top 
level.  Removing the widget from the list has no ill effects, the list is only 
actually used in Partmanager::eventFilter() which will never get an event for a 
deleted widget anyway.

Aside:  The problematic 'foreach (const QWidget *w, 
d->m_managedTopLevelWidgets)' loop in PartManager::PartManager() is really 
superfluous, since all signal connections to 'this' are removed on destruction 
anyway.


Diffs
-

  src/partmanager.cpp 81bf73f 

Diff: https://git.reviewboard.kde.org/r/126385/diff/


Testing
---

Built KParts with this change, and also systemsettings5 with the associated 
change (see associated review).  Observed no crash when exiting the 
application.  Also checked correct operation of Konqueror and Kate.


Thanks,

Jonathan Marten

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126403: Get rid of QApplication dependency

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126403/
---

(Updated Jan. 6, 2016, 5:34 p.m.)


Review request for KDE Frameworks, kwin and Albert Astals Cid.


Bugs: 354811
https://bugs.kde.org/show_bug.cgi?id=354811


Repository: kwindowsystem


Description
---

summarized, alternative to https://git.reviewboard.kde.org/r/126397/

NOTICE: this compiles but is otherwise *completely* untested!


Diffs
-

  src/platforms/xcb/kwindowsystem.cpp 9d28704 

Diff: https://git.reviewboard.kde.org/r/126403/diff/


Testing (updated)
---

Albert performed a successful test on the bug.


Thanks,

Thomas Lübking

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126403: Get rid of QApplication dependency

[Thomas Lübking]

> On Jan. 4, 2016, 7:42 a.m., Martin Gräßlin wrote:
> > anyone tested this on an "affected system"?
> 
> Albert Astals Cid wrote:
> I have not, I thought you had made it clear in the past you thought it 
> was a bad idea since it was all supposed to be widget based anyway.
> 
> Thomas Lübking wrote:
> We're trying to resolve this ;-)
> 
> KWindowSystem could otherwise never be used by QGuiApplication's
> As bonus, we can probably unlink Qt6Widgets in "it's certainly not called 
> KF6 - that would be far too simple" :-P
> (I'm not sure whether we can/should that right now - while you should 
> explicitly link stuff you need, we don't live in a should-land)
> 
> => If you can, give it a try (compiz isn't provided by my distro)
> 
> Albert Astals Cid wrote:
> I can be your tester if you tell me what to test.
> 
> Thomas Lübking wrote:
> Make kscreenlocker_greet crash when using compiz as WM, see bug #354811
> 
> Albert Astals Cid wrote:
> Running /usr/lib/x86_64-linux-gnu/libexec/kscreenlocker_greet on Ubuntu 
> Xenial under Unity7 crashes before applying this patch to kwindowsystem and 
> seems to work fine after applying the patch.

That's the desired result, many thanks for your effort.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126403/#review90522
---


On Jan. 2, 2016, 9:57 a.m., Thomas Lübking wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126403/
> ---
> 
> (Updated Jan. 2, 2016, 9:57 a.m.)
> 
> 
> Review request for KDE Frameworks, kwin and Albert Astals Cid.
> 
> 
> Bugs: 354811
> https://bugs.kde.org/show_bug.cgi?id=354811
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> summarized, alternative to https://git.reviewboard.kde.org/r/126397/
> 
> NOTICE: this compiles but is otherwise *completely* untested!
> 
> 
> Diffs
> -
> 
>   src/platforms/xcb/kwindowsystem.cpp 9d28704 
> 
> Diff: https://git.reviewboard.kde.org/r/126403/diff/
> 
> 
> Testing
> ---
> 
> no, not the least. esp. not on the bug.
> 
> 
> Thanks,
> 
> Thomas Lübking
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126648: Allow to disable KArchive for internal usage

[Luigi Toscano]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126648/
---

(Updated Gen. 6, 2016, 10:33 p.m.)


Review request for Build System, KDE Frameworks, Alex Merry, and David Faure.


Changes
---

Update the patch with the final version.


Repository: kdoctools


Description
---

Most of the code was already prepared to compile without the KArchive 
dependency (including the MEINPROC_NO_KARCHIVE flag).
This mode should be only enabled on KDE infrastructure, as the compressed cache 
(the only added feature coming from KArchive) is not used by the generator of 
the documentation website.
A warning is printed when the mode is enabled.
The saveToCache function is preserved, even if without code, to minimize the 
changes required.


Diffs (updated)
-

  CMakeLists.txt 2ec3027 
  src/CMakeLists.txt 136fbfb 
  src/xslt_kde.cpp 7069a28 

Diff: https://git.reviewboard.kde.org/r/126648/diff/


Testing
---

Normal compilation, compilation with MEINPROC_NO_KARCHIVE=ON


Thanks,

Luigi Toscano

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126648: Allow to disable KArchive for internal usage

[Luigi Toscano]

> On Gen. 6, 2016, 8:33 a.m., David Faure wrote:
> > src/xslt_kde.cpp, line 17
> > 
> >
> > I would suggest to add a qWarning() under this line, to warn at runtime 
> > in case this is ever called in a non-karchive build.

Thanks; I'm going to update the patch for reference, then push it after the 
release of Frameworks 5.18.


- Luigi


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126648/#review90682
---


On Gen. 6, 2016, 1:58 a.m., Luigi Toscano wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126648/
> ---
> 
> (Updated Gen. 6, 2016, 1:58 a.m.)
> 
> 
> Review request for Build System, KDE Frameworks, Alex Merry, and David Faure.
> 
> 
> Repository: kdoctools
> 
> 
> Description
> ---
> 
> Most of the code was already prepared to compile without the KArchive 
> dependency (including the MEINPROC_NO_KARCHIVE flag).
> This mode should be only enabled on KDE infrastructure, as the compressed 
> cache (the only added feature coming from KArchive) is not used by the 
> generator of the documentation website.
> A warning is printed when the mode is enabled.
> The saveToCache function is preserved, even if without code, to minimize the 
> changes required.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 2ec3027 
>   src/CMakeLists.txt 136fbfb 
>   src/xslt_kde.cpp 7069a28 
> 
> Diff: https://git.reviewboard.kde.org/r/126648/diff/
> 
> 
> Testing
> ---
> 
> Normal compilation, compilation with MEINPROC_NO_KARCHIVE=ON
> 
> 
> Thanks,
> 
> Luigi Toscano
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126650: [WIP] Add PM/ScreenSaver Inhibition capabilities to KIdleTime

[Martin Klapetek]

> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > autotests/fakePMServer.cpp, line 23
> > 
> >
> > PowerDevil never returns the same cookie more than once, ie. it's just 
> > ++m_cookieId;

Well I don't think that's an actual problem here, it's a mock/fake server and 
the client does not care *what* cookie data it will receive.


> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > autotests/inhibition_test.cpp, line 60
> > 
> >
> > Does this test work in a normal Plasma session? There's already 
> > PowerDevil claiming those services

The qtest_dbus.h has a code that launces a separate dbus instance. So yeah this 
is fine.


> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > autotests/inhibition_test.cpp, line 89
> > 
> >
> > Perhaps we should split the enum Requested into Activating and 
> > Deactivating, or at least "Busy"/BeingProcessed or so, Requsted for me 
> > implies an "activation requested"

RequestSent? WaitingForReply?


> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > autotests/inhibition_test.cpp, line 134
> > 
> >
> > Please also add a "delete i;" test to verify that automatic 
> > unregistration on deletion also works

Yeah that's in the plan.


> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > src/inhibition.h, line 31
> > 
> >
> > I think we should reserve 0x00 as "no type" or similar

I can sure start the flags from a higher value, but imo "NoType" makes no 
sense, I mean, requesting to inhibit "nothing"?


> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > src/inhibition.h, line 33
> > 
> >
> > I think they should build upon each other
> > 
> > Screen = Suspend | 0x02,
> > PM = Suspend | Screen | 0x04,
> > All = Suspend | Screen | PM

Yeah they really should.


> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > src/inhibition.h, line 54
> > 
> >
> > This is standard QObject behavior but I would still mention that when 
> > the object is destroyed, the inhibition will be released

It says that right there, no? "Parenting the inhibition to any object will 
deactivate ... when that object is destroyed"


> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > src/inhibition.h, line 66
> > 
> >
> > but deleting it will still release the inhibition, right?

If it was activated first, yes. I'll expand that comment.


> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > src/inhibition.h, lines 105-106
> > 
> >
> > Then you'll have two inhibitions, one of which will no longer be 
> > managed by this object. You need to deactivate first.

Ah yes, I was hoping this could be done in an easier way but needs deactivate() 
first.


> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > src/inhibition.h, line 122
> > 
> >
> > Do we use Q_NULLPTR in Frameworks?

Iii...think so?


> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > src/inhibition.cpp, lines 38-40
> > 
> >
> > initializer list?
> > 
> > Private()
> > : status(Inhibition::Inactive)
> > , ...
> > 
> > Also, you're leaving most member variables uninitialized

Is there any actual difference (performance or something) between setting the 
values in ctor and setting them before ctor?


> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > src/inhibition.cpp, line 83
> > 
> >
> > This blocks, doesn't it?

Yes, but I'm not sure if we actually don't want blocking here? If you use the 
big static method, it may get into activate() before the solid reply has 
returned. Alternatively I could make a kind of semaphore thingy and trigger 
activate() only after the solid reply has arrived.


> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > src/inhibition.cpp, line 144
> > 
> >
> > If I didn't change anything but status was Revoked or Failed I would 
> > not be able to activate it again?

Good point, needs fixing.


> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > 

Re: Draft split for qpt plugin from frameworkintegration

[Albert Astals Cid]

El Friday 18 December 2015, a les 10:03:39, Martin Graesslin va escriure:
> On Thursday, December 17, 2015 5:48:47 PM CET Martin Graesslin wrote:
> > On Thursday, December 17, 2015 4:32:48 PM CET Aleix Pol wrote:
> > > On Wed, Dec 16, 2015 at 4:12 PM, Martin Graesslin 
> > 
> > wrote:
> > > > Hi all,
> > > > 
> > > > following up on [1] I have prepared a split of frameworkintegration to
> > > > move
> > > > the QPT plugin into a dedicated repository. You can find it in [2].
> > > > Please
> > > > have a look at the split repo to verify that it looks fine. If
> > > > everything
> > > > is OK, I'll request a new repo from sysadmins.
> > > > 
> > > > Some general notes
> > > > * new repo name: plasma-integration
> > > > * new plugin name: PlasmaDesktopPlatformTheme
> > > > * new src folder name: src/plasma-desktop-platformtheme
> > > > 
> > > > Explaining the name changes:
> > > > The plugin is renamed to not conflict on install with the existing
> > > > plugin
> > > > from frameworkintegration and also incorporating future changes Marco
> > > > pointed out: we need a different QPT plugin for mobile.
> > > > 
> > > > How to remove the plugin from frameworkintegration:
> > > > After the split we cannot remove it from frameworkintegration yet as
> > > > that
> > > > would break setups on the next framework release. Given that I plan to
> > > > only
> > > > phase it out: Introduce a cmake option to build and install it, which
> > > > defaults to true till at least Plasma 5.6 is released. Afterward
> > > > swapping
> > > > the default to false, with a big fat warning and keeping it like that
> > > > for
> > > > at least half a year. Then remove it. We don't provide any guarantees
> > > > for
> > > > it, so we can remove it, but I want to keep the impact small.
> > > > 
> > > > Cheers
> > > > Martin
> > > > 
> > > > [1] https://mail.kde.org/pipermail/kde-frameworks-devel/2015-December/
> > > > 029234.html
> > > > [2] kde:scratch/graesslin/platform-integration
> > > > (https://quickgit.kde.org/?
> > > > p=scratch%2Fgraesslin%2Fplasma-integration.git )
> > > 
> > > I have my doubts that we want a different QPT for desktop and mobile.
> > > If we want to provide any convergence we need to strive to have the
> > > same on both platforms.
> > 
> > Good point! Marco is right: we need different presets whether it's desktop
> > or mobile. You're right that different plugins would result in making
> > switching impossible. Sounds like the plugin would have to learn to switch
> > at runtime.
> > 
> > Given that feedback I'm considering to not rename the directory.
> 
> pushed a new variant without the rename of the source folder and the plugin
> renamed to KDEPlasmaPlatformTheme.

Can you guys please have a look at the Messages.sh? We have now two codebases 
(Which i guess are different at this point?) creating the same 
frameworksintegration5.pot file

Cheers,
  Albert

> 
> Cheers
> Martin

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel