Modernize the signal-slot syntax
ClosedPublic

Authored by Petross404 on Sep 12 2019, 1:51 PM.

Details

Reviewers
mlaurent
Group Reviewers
Gwenview
Summary

This patch is about modernizing the signal - slot syntax to use the function pointer method. Unfortunatelly I couldn't make this work with qOverload, although I was using GCC 9.2 with C++14 standard option.

Let me know if there is anything I am missing. Thank you.

Test Plan

I applied the patch with portage and kamera installed and showed up fine. I don't have the needed hardware to further test it though.

Diff Detail

Repository
R370 Kamera
Lint
Lint Skipped
Unit
Unit Tests Skipped
Petross404 requested review of this revision.Sep 12 2019, 1:51 PM
Petross404 created this revision.
cfeck added a reviewer: Gwenview.

You need to add
"set(CMAKE_CXX_STANDARD 14)" if you want to use C++14 in CMakeLists.txt (at toplevel)

You need to add
"set(CMAKE_CXX_STANDARD 14)" if you want to use C++14 in CMakeLists.txt (at toplevel)

Hmm, I thought that setting C++ parser from project's settings in KDevelop was sufficient but it was not after all. In that case what should I do now?

adding "set(CMAKE_CXX_STANDARD 14)" and port to qOverload :)

Petross404 updated this revision to Diff 66010.Sep 13 2019, 6:53 PM

In the top level CMakelists.txt I added build system support for C++14 which was necessary for the qOverload macro.

Also CMAKE_CXX_STANDARD_REQUIRED to also set it as a requirement

Also CMAKE_CXX_STANDARD_REQUIRED to also set it as a requirement

Like this?

set(CMAKE_CXX_STANDARD 14)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

Also CMAKE_CXX_STANDARD_REQUIRED to also set it as a requirement

Like this?

set(CMAKE_CXX_STANDARD 14)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

Exactly!

Petross404 updated this revision to Diff 66022.Sep 14 2019, 9:05 AM
set(CMAKE_CXX_STANDARD_REQUIRED ON)

is added.

mlaurent requested changes to this revision.Sep 14 2019, 10:05 AM
mlaurent added inline comments.
kcontrol/kamera.cpp
131

Remove extra tab. Same for other lines

216

Fix indent here too

This revision now requires changes to proceed.Sep 14 2019, 10:05 AM
Petross404 updated this revision to Diff 66045.Sep 14 2019, 1:15 PM

Some wrong tabs and spaces are fixed.

tommo added a subscriber: tommo.Sep 14 2019, 2:05 PM

Pls. note that the usage of CMAKE_CXX_STANDARD requires bumping cmake_minimum_required to 3.1.0 .

After changing cmake version +2 for me.

Petross404 updated this revision to Diff 66320.Sep 17 2019, 5:17 PM
Petross404 marked 2 inline comments as done.

Minimum cmake version is bumped to 3.1.0.

mlaurent accepted this revision.Sep 18 2019, 7:43 AM

Thanks.

This revision is now accepted and ready to land.Sep 18 2019, 7:43 AM

Did you have commit access?

Did you have commit access?

No, I don't think I have commit access.

kcontrol/kameradevice.cpp
304

I didn't notice this earlier. Should I fix this?

mlaurent updated this revision to Diff 66642.Sep 23 2019, 5:10 AM

Fix merge conflict

I rebased it and fixed conflict but it changed owner... why ??? How I can change it ?

I rebased it and fixed conflict but it changed owner... why ??? How I can change it ?

I don't know, but if the commit is using your name, it's fine with me.

mlaurent closed this revision.Sep 29 2019, 8:42 AM