Merge lp:~vthompson/ubuntu-clock-app/resolve-build-warning into lp:ubuntu-clock-app

[Victor Thompson]

Resolve build warning

Currently, a warning is produced when building the app. An unused parameter "valid" is part of the signature for AlarmSettings::onSettingsChanged. Can this variable be removed or is it needed for a future functionality?

[ 30%] Building CXX object backend/CMakeFiles/alarmsettings.dir/modules/Alarm/Settings/alarmsettings.cpp.o
/home/victor/Development/reboot/backend/modules/Alarm/Settings/alarmsettings.cpp:40:6: warning: unused parameter ‘valid’ [-Wunused-parameter]
 void AlarmSettings::onSettingsChanged(const QString &interface,
      ^

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

FAILED: Continuous integration, rev:32
http://91.189.93.70:8080/job/ubuntu-clock-dev-ubuntu-clock-app-utopic-3.0-ci/76/
Executed test runs:
    FAILURE: http://91.189.93.70:8080/job/generic-mediumtests-utopic/1164/console
    FAILURE: http://91.189.93.70:8080/job/ubuntu-clock-dev-ubuntu-clock-app-utopic-3.0-utopic-amd64-ci/76/console

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-clock-dev-ubuntu-clock-app-utopic-3.0-ci/76/rebuild

review: Needs Fixing (continuous-integration)

[Nekhelesh Ramananthan (nik90)]

hmm I will have to check with Charles about this. Will find out tomorrow.

[Charles Kerr (charlesk)]

Nik90, TBH I haven't dropped arguments out of a signal/slot function and am not sure what Qt does under the covers there. If Victor's patch works, I guess that's our answer.

Another way to fix the warning is to leave the function signature alone (i.e., keep the arguments the same to match the org.freedesktop.DBus.Properties.PropertiesChanged signature) but remove the names of the unused fields to let the compiler know they're unused. I usually do this by putting the variable name inside a comment so it's hidden from the compiler but that humans can still see what's going on. Specifically:

void onSettingsChanged(const QString &interface,
                       const QVariantMap &properties,
                       const QStringList& /*invalidated_properties*/);

[Nekhelesh Ramananthan (nik90)]

@Victor, what you proposed and what charles said both work. I tested it on the utopic phone emulator. I think however we should stick to charles's solution since this way we do not change the function signature. So can you please change that to,

=== modified file 'backend/modules/Alarm/Settings/alarmsettings.cpp'
--- backend/modules/Alarm/Settings/alarmsettings.cpp 2014-07-27 22:18:23 +0000
+++ backend/modules/Alarm/Settings/alarmsettings.cpp 2014-07-28 15:52:41 +0000
@@ -34,11 +34,12 @@
                        "org.freedesktop.DBus.Properties",
                        "PropertiesChanged",
                        this,
- SLOT(onSettingsChanged(QString, QVariantMap)));
+ SLOT(onSettingsChanged(QString, QVariantMap, QStringList)));
 }

 void AlarmSettings::onSettingsChanged(const QString &interface,
- const QVariantMap &properties)
+ const QVariantMap &properties,
+ const QStringList /*&valid*/)
 {
     if(interface != "com.canonical.indicator.datetime.AlarmProperties") {
         // Check if the properties changed are in the correct interface

=== modified file 'backend/modules/Alarm/Settings/alarmsettings.h'
--- backend/modules/Alarm/Settings/alarmsettings.h 2014-07-27 22:18:23 +0000
+++ backend/modules/Alarm/Settings/alarmsettings.h 2014-07-28 15:51:29 +0000
@@ -79,7 +79,8 @@
      settings values are changed on dBus side
     */
     void onSettingsChanged(const QString &interface,
- const QVariantMap &properties);
+ const QVariantMap &properties,
+ const QStringList /*invalidated_properties*/);
 };

[Victor Thompson (vthompson)]

I will do this the way Charles suggests, I agree it's a better option. However, I'd like to leave the variable name in the commentary as-is:

=== modified file 'backend/modules/Alarm/Settings/alarmsettings.h'
--- backend/modules/Alarm/Settings/alarmsettings.h 2014-07-27 22:18:23 +0000
+++ backend/modules/Alarm/Settings/alarmsettings.h 2014-07-28 15:51:29 +0000
@@ -79,7 +79,8 @@
      settings values are changed on dBus side
     */
     void onSettingsChanged(const QString &interface,
- const QVariantMap &properties);
+ const QVariantMap &properties,
+ const QStringList /*&valid*/);

review: Needs Fixing

[Victor Thompson (vthompson)]

It seems Qt has a defined macro for this purpose [1]. Maybe that's a better option? I assume that it's portable.

[1] http://qt-project.org/doc/qt-5/qtglobal.html#Q_UNUSED

[Nekhelesh Ramananthan (nik90)]

> It seems Qt has a defined macro for this purpose [1]. Maybe that's a better
> option? I assume that it's portable.
>
> [1] http://qt-project.org/doc/qt-5/qtglobal.html#Q_UNUSED

Reading through http://www.qtcentre.org/threads/39725-A-more-extended-explanation-for-Q_UNUSED-macro, either one is fine. So I leave the choice to you.

[lp:~vthompson/ubuntu-clock-app/resolve-build-warning]

33. By Victor Thompson on 2014-07-28

Update solution

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

FAILED: Continuous integration, rev:33
http://91.189.93.70:8080/job/ubuntu-clock-dev-ubuntu-clock-app-utopic-3.0-ci/83/
Executed test runs:
    FAILURE: http://91.189.93.70:8080/job/generic-mediumtests-utopic/1181/console
    FAILURE: http://91.189.93.70:8080/job/ubuntu-clock-dev-ubuntu-clock-app-utopic-3.0-utopic-amd64-ci/83/console

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-clock-dev-ubuntu-clock-app-utopic-3.0-ci/83/rebuild

review: Needs Fixing (continuous-integration)

[Nekhelesh Ramananthan (nik90)]

lgtm

review: Approve

=== modified file 'backend/modules/Alarm/Settings/alarmsettings.cpp'
--- backend/modules/Alarm/Settings/alarmsettings.cpp	2014-07-18 14:56:49 +0000
+++ backend/modules/Alarm/Settings/alarmsettings.cpp	2014-07-28 23:09:33 +0000
@@ -39,7 +39,7 @@
 
 void AlarmSettings::onSettingsChanged(const QString &interface,
                                       const QVariantMap &properties,
-                                      const QStringList &valid)
+                                      const QStringList & /*valid*/)
 {
     if(interface != "com.canonical.indicator.datetime.AlarmProperties") {
         // Check if the properties changed are in the correct interface

=== modified file 'backend/modules/Alarm/Settings/alarmsettings.h'
--- backend/modules/Alarm/Settings/alarmsettings.h	2014-07-18 14:56:49 +0000
+++ backend/modules/Alarm/Settings/alarmsettings.h	2014-07-28 23:09:33 +0000
@@ -80,7 +80,7 @@
     */
     void onSettingsChanged(const QString &interface,
                            const QVariantMap &properties,
-                           const QStringList &valid);
+                           const QStringList & /*valid*/);
 };
 
 #endif