Re: Review Request 115016: Make KJob usable from QML
On Wed, Jan 15, 2014, at 4:43, Kevin Krammer wrote: On Tuesday, 2014-01-14, 23:12:56, Aurélien Gâteau wrote: On Jan. 14, 2014, 9:20 p.m., Alex Merry wrote: src/lib/jobs/kjob.h, line 92 https://git.reviewboard.kde.org/r/115016/diff/1/?file=233991#file233991 line92 I don't think this will work; I'm fairly sure that notify signals must have zero or one arguments, and the one argument must be the same type as the property. The notify signal in this class has a preceding KJob* argument. Aleix Pol Gonzalez wrote: Quoting the documentation: A NOTIFY signal is optional. If defined, it should specify one existing signal in that class that is emitted whenever the value of the property changes. Also I've been using this in a plasmoid I'm porting and it doesn't seem to cause problems. Quoting the *Qt 5* documentation: (http://doc-snapshot.qt-project.org/qt5-stable/properties.html) A NOTIFY signal is optional. If defined, it should specify one existing signal in that class that is emitted whenever the value of the property changes. NOTIFY signals for MEMBER variables must take zero or one parameter, which must be of the same type as the property. The parameter will take the new value of the property. The fix is probably just a matter of introducing a void percentChanged(int) signal, and emitting it wherever percent() is emitted. No need, the percent property is not using the MEMBER option of the Q_PROPERY macro, it is using the classic READ followed by getter function approach. Oh, good point. I missed the MEMBER part (actually I didn't even know about MEMBER properties, one always learns!) Aurélien ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115016: Make KJob usable from QML
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115016/#review47489 --- This review has been submitted with commit 08f24ddc85bd6c64097e4700a2db7e6f44090b5d by Aleix Pol to branch master. - Commit Hook On Jan. 14, 2014, 8:16 p.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115016/ --- (Updated Jan. 14, 2014, 8:16 p.m.) Review request for KDE Frameworks. Repository: kcoreaddons Description --- Add a couple of Q_PROPERTY and Q_SCRIPTABLE to KJob, so that it can be consumed elsewhere. The idea isn't to be able to implement KJobs, but to be able to keep track of them from QML and check if it succeeded. Diffs - src/lib/jobs/kjob.h 24dbaec Diff: https://git.reviewboard.kde.org/r/115016/diff/ Testing --- Everything still builds, shouldn't be a big deal. Thanks, Aleix Pol Gonzalez ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115016: Make KJob usable from QML
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115016/ --- (Updated Jan. 16, 2014, 11:06 a.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Repository: kcoreaddons Description --- Add a couple of Q_PROPERTY and Q_SCRIPTABLE to KJob, so that it can be consumed elsewhere. The idea isn't to be able to implement KJobs, but to be able to keep track of them from QML and check if it succeeded. Diffs - src/lib/jobs/kjob.h 24dbaec Diff: https://git.reviewboard.kde.org/r/115016/diff/ Testing --- Everything still builds, shouldn't be a big deal. Thanks, Aleix Pol Gonzalez ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115016: Make KJob usable from QML
On 15/01/14 12:43, Kevin Krammer wrote: On Tuesday, 2014-01-14, 23:12:56, Aurélien Gâteau wrote: A NOTIFY signal is optional. If defined, it should specify one existing signal in that class that is emitted whenever the value of the property changes. NOTIFY signals for MEMBER variables must take zero or one parameter, which must be of the same type as the property. The parameter will take the new value of the property. The fix is probably just a matter of introducing a void percentChanged(int) signal, and emitting it wherever percent() is emitted. No need, the percent property is not using the MEMBER option of the Q_PROPERY macro, it is using the classic READ followed by getter function approach. Ah, good catch; I'd previously misread that... Alex ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115016: Make KJob usable from QML
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115016/#review47440 --- Ship it! After Kevin Krammer's comment on the list, I realised I'd been misreading the Q_PROPERTY docs. So this is fine, and can go in. - Alex Merry On Jan. 14, 2014, 8:16 p.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115016/ --- (Updated Jan. 14, 2014, 8:16 p.m.) Review request for KDE Frameworks. Repository: kcoreaddons Description --- Add a couple of Q_PROPERTY and Q_SCRIPTABLE to KJob, so that it can be consumed elsewhere. The idea isn't to be able to implement KJobs, but to be able to keep track of them from QML and check if it succeeded. Diffs - src/lib/jobs/kjob.h 24dbaec Diff: https://git.reviewboard.kde.org/r/115016/diff/ Testing --- Everything still builds, shouldn't be a big deal. Thanks, Aleix Pol Gonzalez ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 115016: Make KJob usable from QML
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115016/ --- Review request for KDE Frameworks. Repository: kcoreaddons Description --- Add a couple of Q_PROPERTY and Q_SCRIPTABLE to KJob, so that it can be consumed elsewhere. The idea isn't to be able to implement KJobs, but to be able to keep track of them from QML and check if it succeeded. Diffs - src/lib/jobs/kjob.h 24dbaec Diff: https://git.reviewboard.kde.org/r/115016/diff/ Testing --- Everything still builds, shouldn't be a big deal. Thanks, Aleix Pol Gonzalez ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115016: Make KJob usable from QML
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115016/#review47395 --- src/lib/jobs/kjob.h https://git.reviewboard.kde.org/r/115016/#comment33726 I don't think this will work; I'm fairly sure that notify signals must have zero or one arguments, and the one argument must be the same type as the property. The notify signal in this class has a preceding KJob* argument. - Alex Merry On Jan. 14, 2014, 8:16 p.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115016/ --- (Updated Jan. 14, 2014, 8:16 p.m.) Review request for KDE Frameworks. Repository: kcoreaddons Description --- Add a couple of Q_PROPERTY and Q_SCRIPTABLE to KJob, so that it can be consumed elsewhere. The idea isn't to be able to implement KJobs, but to be able to keep track of them from QML and check if it succeeded. Diffs - src/lib/jobs/kjob.h 24dbaec Diff: https://git.reviewboard.kde.org/r/115016/diff/ Testing --- Everything still builds, shouldn't be a big deal. Thanks, Aleix Pol Gonzalez ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115016: Make KJob usable from QML
On Jan. 14, 2014, 8:20 p.m., Alex Merry wrote: src/lib/jobs/kjob.h, line 92 https://git.reviewboard.kde.org/r/115016/diff/1/?file=233991#file233991line92 I don't think this will work; I'm fairly sure that notify signals must have zero or one arguments, and the one argument must be the same type as the property. The notify signal in this class has a preceding KJob* argument. Quoting the documentation: A NOTIFY signal is optional. If defined, it should specify one existing signal in that class that is emitted whenever the value of the property changes. Also I've been using this in a plasmoid I'm porting and it doesn't seem to cause problems. - Aleix --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115016/#review47395 --- On Jan. 14, 2014, 8:16 p.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115016/ --- (Updated Jan. 14, 2014, 8:16 p.m.) Review request for KDE Frameworks. Repository: kcoreaddons Description --- Add a couple of Q_PROPERTY and Q_SCRIPTABLE to KJob, so that it can be consumed elsewhere. The idea isn't to be able to implement KJobs, but to be able to keep track of them from QML and check if it succeeded. Diffs - src/lib/jobs/kjob.h 24dbaec Diff: https://git.reviewboard.kde.org/r/115016/diff/ Testing --- Everything still builds, shouldn't be a big deal. Thanks, Aleix Pol Gonzalez ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115016: Make KJob usable from QML
On Jan. 14, 2014, 9:20 p.m., Alex Merry wrote: src/lib/jobs/kjob.h, line 92 https://git.reviewboard.kde.org/r/115016/diff/1/?file=233991#file233991line92 I don't think this will work; I'm fairly sure that notify signals must have zero or one arguments, and the one argument must be the same type as the property. The notify signal in this class has a preceding KJob* argument. Aleix Pol Gonzalez wrote: Quoting the documentation: A NOTIFY signal is optional. If defined, it should specify one existing signal in that class that is emitted whenever the value of the property changes. Also I've been using this in a plasmoid I'm porting and it doesn't seem to cause problems. Quoting the *Qt 5* documentation: (http://doc-snapshot.qt-project.org/qt5-stable/properties.html) A NOTIFY signal is optional. If defined, it should specify one existing signal in that class that is emitted whenever the value of the property changes. NOTIFY signals for MEMBER variables must take zero or one parameter, which must be of the same type as the property. The parameter will take the new value of the property. The fix is probably just a matter of introducing a void percentChanged(int) signal, and emitting it wherever percent() is emitted. - Aurélien --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115016/#review47395 --- On Jan. 14, 2014, 9:16 p.m., Aleix Pol Gonzalez wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115016/ --- (Updated Jan. 14, 2014, 9:16 p.m.) Review request for KDE Frameworks. Repository: kcoreaddons Description --- Add a couple of Q_PROPERTY and Q_SCRIPTABLE to KJob, so that it can be consumed elsewhere. The idea isn't to be able to implement KJobs, but to be able to keep track of them from QML and check if it succeeded. Diffs - src/lib/jobs/kjob.h 24dbaec Diff: https://git.reviewboard.kde.org/r/115016/diff/ Testing --- Everything still builds, shouldn't be a big deal. Thanks, Aleix Pol Gonzalez ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
