D29268: [WIP] Add Date/Time dialog

[Kai Uwe Broulik]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200429/c859e804/attachment-0001.html>