Re: [Kde-hardware-devel] Review Request 126096: Fix memory leak in kscreen backend of libkscreen

2015-11-17 Thread Lamarque Souza 


> On Nov. 17, 2015, 4:08 p.m., Martin Gräßlin wrote:
> > backends/xrandr/xrandrscreen.cpp, line 67
> > 
> >
> > Suggestion QScopedPointer
> 
> Daniel Vrátil wrote:
> Yes please, but use XCB::ScopedPointer from xcbwrapper.h instead.

Do you think it is safe to delete the pointer returned by 
xcb_randr_get_screen_resources_current_reply() in XRandR::screenResources()? I 
seems so, just wondering if this patch can side effects in the "hack" used in 
XRandR::screenResources().


- Lamarque


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


On Nov. 17, 2015, 4:31 p.m., Lamarque Souza wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126096/
> ---
> 
> (Updated Nov. 17, 2015, 4:31 p.m.)
> 
> 
> Review request for Plasma, Solid and Daniel Vrátil.
> 
> 
> Repository: libkscreen
> 
> 
> Description
> ---
> 
> libkscreen needs to free memory of xcb_randr_get_screen_resources_reply.
> 
> 
> Diffs
> -
> 
>   backends/xrandr/xrandrscreen.cpp 8df957e 
> 
> Diff: https://git.reviewboard.kde.org/r/126096/diff/
> 
> 
> Testing
> ---
> 
> No memory leak and everything still works.
> 
> 
> Thanks,
> 
> Lamarque Souza
> 
>

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

Re: [Kde-hardware-devel] Review Request 126096: Fix memory leak in kscreen backend of libkscreen

2015-11-17 Thread Lamarque Souza 

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

(Updated Nov. 17, 2015, 4:41 p.m.)


Status
--

This change has been marked as submitted.


Review request for Plasma, Solid and Daniel Vrátil.


Changes
---

Submitted with commit c0141c4b22cc2c336fefa54f331fd1036fdaf3e9 by Lamarque V. 
Souza to branch master.


Repository: libkscreen


Description
---

libkscreen needs to free memory of xcb_randr_get_screen_resources_reply.


Diffs
-

  backends/xrandr/xrandrscreen.cpp 8df957e 

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


Testing
---

No memory leak and everything still works.


Thanks,

Lamarque Souza

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

[Kde-hardware-devel] Review Request 126096: Fix memory leak in kscreen backend of libkscreen

2015-11-17 Thread Lamarque Souza 

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

Review request for Solid and Daniel Vrátil.


Repository: libkscreen


Description
---

libkscreen needs to free memory of xcb_randr_get_screen_resources_reply.


Diffs
-

  backends/xrandr/xrandrscreen.cpp 8df957e 

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


Testing
---

No memory leak and everything still works.


Thanks,

Lamarque Souza

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

Re: [Kde-hardware-devel] Review Request 126101: allow loading backends in-process

2015-11-17 Thread Sebastian Kügler 

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

(Updated Nov. 18, 2015, 2:36 a.m.)


Review request for Plasma, Solid, Àlex Fiestas, Aleix Pol Gonzalez, Daniel 
Vrátil, and Martin Gräßlin.


Changes
---

Address comments.

As I explained, I've left the plugin loading logic in BackendManager, but made 
the method names a bit more verbose. Everything else is taken care of as 
suggested.


Repository: libkscreen


Description
---

This patchset adds in-process operation to libkscreen

purpose:
- allow easier debugging
- for some backends (qscreen, upcoming kwayland) the out of process operation 
is not necessary since these backends are well-shielded

This implementation is backwards compatible and should mean only minimal 
changes to running setups.

If the user exports KSCREEN_BACKEND_INPROCESS=1 before running, all operations 
will be done in process. Otherwise, the out-of-process mode is used.

The idea is that we use 
The changes in the clients to use the in-process mode are to use 
ConfigOperation::create() and ConfigOperation::setOperation() to retrieve the 
get or set config jobs. The rest will be handled inside libkscreen.

Autotests should cover all the cases (and actually a few currently unsupported 
ones, such as using different backends in the same process).

Details on performance, etc.: 
http://vizzzion.org/blog/2015/11/wayland-and-libkscreen-benchmarks/


Diffs (updated)
-

  src/configoperation.h 2405d79 
  autotests/testconfigmonitor.cpp a051226 
  backends/fake/fake.cpp 60264dd 
  src/configmonitor.cpp a14bc70 
  src/backendlauncher/backendloader.cpp 52051df 
  src/backendmanager.cpp ca9c746 
  src/backendmanager_p.h c6418e2 
  src/config.cpp 75d947d 
  src/configmonitor.h b6f1189 
  src/configoperation.cpp 87fe141 
  CMakeLists.txt 86a0965 
  autotests/CMakeLists.txt 69af7f0 
  autotests/testinprocess.cpp PRE-CREATION 
  autotests/testqscreenbackend.cpp da4dbae 
  autotests/testscreenconfig.cpp ecbcedf 
  autotests/testxrandr.cpp b9b838a 
  src/configoperation_p.h afdc462 
  src/getconfigoperation.h c85bfaa 
  src/getconfigoperation.cpp 22f92b4 
  src/output.cpp bd381fa 
  src/setconfigoperation.h 020a990 
  src/setconfigoperation.cpp 6ea944f 

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


Testing
---

Added a ton of autotests, made sure all existing ones pass.

tried "KSCREEN_BACKEND_INPROCESS=1 kcmshell5 kscreen", all working as expected.


Thanks,

Sebastian Kügler

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

Re: [Kde-hardware-devel] Review Request 126101: allow loading backends in-process

2015-11-17 Thread Sebastian Kügler 


> On Nov. 17, 2015, 9:55 p.m., Daniel Vrátil wrote:
> > src/backendmanager_p.h, line 130
> > 
> >
> > Why is this even a QHash? We don't want to run multiple backends at 
> > once, or do we? Wouldn't just holding pointer to the current backend be 
> > enough? Maybe even without the arguments...

The arguments are needed for the Fake backend, otherwise, loading a different 
config doesn't work, which breaks a bunch of tests. Instead of shutting down 
the backend, it's enough to re-init() it, which is what I'm doing here.

This bit could potentially make sense for other cases, for example passing the 
config for a wayland test server (which is what I'm doing for the wayland 
tests), and possibly also the WAYLAND_DISPLAY to use, as to avoid getting in 
the way of a running server.


- Sebastian


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


On Nov. 18, 2015, 12:32 a.m., Sebastian Kügler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126101/
> ---
> 
> (Updated Nov. 18, 2015, 12:32 a.m.)
> 
> 
> Review request for Plasma, Solid, Àlex Fiestas, Aleix Pol Gonzalez, Daniel 
> Vrátil, and Martin Gräßlin.
> 
> 
> Repository: libkscreen
> 
> 
> Description
> ---
> 
> This patchset adds in-process operation to libkscreen
> 
> purpose:
> - allow easier debugging
> - for some backends (qscreen, upcoming kwayland) the out of process operation 
> is not necessary since these backends are well-shielded
> 
> This implementation is backwards compatible and should mean only minimal 
> changes to running setups.
> 
> If the user exports KSCREEN_BACKEND_INPROCESS=1 before running, all 
> operations will be done in process. Otherwise, the out-of-process mode is 
> used.
> 
> The idea is that we use 
> The changes in the clients to use the in-process mode are to use 
> ConfigOperation::create() and ConfigOperation::setOperation() to retrieve the 
> get or set config jobs. The rest will be handled inside libkscreen.
> 
> Autotests should cover all the cases (and actually a few currently 
> unsupported ones, such as using different backends in the same process).
> 
> Details on performance, etc.: 
> http://vizzzion.org/blog/2015/11/wayland-and-libkscreen-benchmarks/
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 86a0965 
>   autotests/CMakeLists.txt 69af7f0 
>   autotests/testconfigmonitor.cpp a051226 
>   autotests/testinprocess.cpp PRE-CREATION 
>   autotests/testqscreenbackend.cpp da4dbae 
>   autotests/testscreenconfig.cpp ecbcedf 
>   backends/fake/fake.cpp 60264dd 
>   src/CMakeLists.txt 4b56b61 
>   src/backendlauncher/backendloader.cpp 52051df 
>   src/backendmanager.cpp ca9c746 
>   src/backendmanager_p.h c6418e2 
>   src/config.cpp 75d947d 
>   src/configmonitor.h b6f1189 
>   src/configmonitor.cpp a14bc70 
>   src/configoperation.h 2405d79 
>   src/configoperation.cpp 87fe141 
>   src/getconfigoperation.h c85bfaa 
>   src/inprocessconfigoperation.h PRE-CREATION 
>   src/inprocessconfigoperation.cpp PRE-CREATION 
>   src/output.cpp bd381fa 
>   src/setconfigoperation.cpp 6ea944f 
>   src/setinprocessoperation.h PRE-CREATION 
>   src/setinprocessoperation.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126101/diff/
> 
> 
> Testing
> ---
> 
> Added a ton of autotests, made sure all existing ones pass.
> 
> tried "KSCREEN_BACKEND_INPROCESS=1 kcmshell5 kscreen", all working as 
> expected.
> 
> 
> Thanks,
> 
> Sebastian Kügler
> 
>

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