broulik added inline comments.
INLINE COMMENTS
> CMakeLists.txt:69
> +if(ANDROID)
> +install(FILES
> + "${CMAKE_CURRENT_BINARY_DIR}/KF5KirigamiAddonsConfig.cmake"
Why this only on Android? or is that used for that dummy library?
> FindGradle.cmake:2
> +#.rst:
> +# FindGradle
> +# ----------
Feels like this should go into ECM?
> DateDialog.qml:12
> +
> +import org.kde.kirigamiaddons.dateandtime 0.1 as KDT
> +
`org.kde.kirigamiaddons.private.dateandtime` to emphasis it's an impl detail
> DateDialog.qml:15
> +Item {
> + signal datePicked(date theDate)
> +
`theDate` is a bit awkward since that's what you'd have to use when you use the
signal
> KF5KirigamiDateAndTimeAndroid-android-dependencies.xml:5
> + <depends>
> + <jar bundling="1" file="jar/KF5DateAndTime.aar"/>
> + </depends>
`KF5KirigamiAddonsDateAndTime`?
> TimeDialog.qml:15
> +Item {
> + signal timePicked(date theTime)
> +
Generally conversions between C++ timedate and JavaScript `Date` is bad.
There's no way to represent just a time with no date and timezone associated
with it in JavaScript.
While it's ugly, I'd suggest we return a bunch of `int`.
Also, I think we should have the selected time as properties and an
`accepted`/rejected signal or similar.
(Same probably goes for the date picker)
> TimeDialog.qml:35
> + // Dummy for AndroidUtils object when not on Android
> + Item {
> + id: dummy
Use `QtObject` for this then
> androidutils.cpp:12
> +#include <QtAndroid>
> +#include <QThread>
> +
Unused
> androidutils.cpp:54
> +
> +static const JNINativeMethod methods[] = {{"dateSelected", "(III)V", (void
> *)dateSelected}, {"cancelled", "()V", (void *)dateCancelled}};
> +
`dateMethods`
> androidutils.cpp:61
> + static bool initialized = false;
> + if (initialized)
> + return JNI_VERSION_1_6;
Coding style
> androidutils.cpp:66
> + JNIEnv *env = nullptr;
> + auto foo = vm->GetEnv((void **)&env, JNI_VERSION_1_4);
> +
Better name please
> androidutils.cpp:89
> +{
> + QAndroidJniObject
> picker("org/kde/kirigamiaddons/dateandtime/DatePicker",
> "(Landroid/app/Activity;J)V", QtAndroid::androidActivity().object(),
> QDateTime::currentDateTime().toMSecsSinceEpoch());
> + picker.callMethod<void>("doShow");
There's `QDateTime::currentMSecsSinceEpoch()`
> androidutils.h:10
> +#include <QObject>
> +#include <QDate>
> +#include <QTime>
Just forward-declare?
> androidutils.h:26
> +
> + static AndroidUtils *instance();
> +
Generally we want singletons to return references these days to communicate
ownership properly
> androidutils.h:29
> +Q_SIGNALS:
> + void datePickerFinished(bool accepted, const QDate date);
> + void timePickerFinished(bool accepted, const QTime &time);
`const QDate &`
> plugin.cpp:58
> + qmlRegisterSingletonType<AndroidUtils>(uri, 0, 1, "AndroidUtils",
> [](QQmlEngine *, QJSEngine *) -> QObject * {
> + return AndroidUtils::instance();
> + });
You need to set
QQmlEngine::setObjectOwnership(..., QQmlEngine::CppOwnership);
> A QObject singleton type instance returned from a singleton type provider is
> owned by the QML engine unless the object has explicit
> QQmlEngine::CppOwnership flag set.
REPOSITORY
R1047 Kirigami Addons
REVISION DETAIL
https://phabricator.kde.org/D29268
To: nicolasfella, davidedmundson, vkrause, broulik
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus,
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, ahiemstra, mart
