D29268: [WIP] Add Date/Time dialog

[Nicolas Fella]

nicolasfella added inline comments.

INLINE COMMENTS

> broulik wrote in androidutils.h:29
> `const QDate &`

Actually clazy disagrees, QDate and QTime should be passed by value

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

D29268: [WIP] Add Date/Time dialog

[Nicolas Fella]

nicolasfella marked an inline comment as done.
nicolasfella added inline comments.

INLINE COMMENTS

> broulik wrote in TimeDialog.qml:15
> 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)

If we do this should the date thing also use ints for symmentry?

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

D29268: [WIP] Add Date/Time dialog

[Nicolas Fella]

nicolasfella updated this revision to Diff 83053.
nicolasfella marked 10 inline comments as done.
nicolasfella added a comment.


  - fix
  - Address some comments

REPOSITORY
  R1047 Kirigami Addons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29268?vs=81471&id=83053

BRANCH
  work

REVISION DETAIL
  https://phabricator.kde.org/D29268

AFFECTED FILES
  CMakeLists.txt
  KF5KirigamiAddonsConfig.cmake.in
  cmake/modules/FindGradle.cmake
  cmake/modules/local.properties.in
  cmake/modules/settings.gradle.in
  src/dateandtime/CMakeLists.txt
  src/dateandtime/DateDialog.qml
  src/dateandtime/KF5KirigamiDateAndTimeAndroid-android-dependencies.xml
  src/dateandtime/TimeDialog.qml
  src/dateandtime/android/AndroidManifest.xml
  src/dateandtime/android/CMakeLists.txt
  src/dateandtime/android/build.gradle
  src/dateandtime/android/org/kde/kirigamiaddons/dateandtime/DatePicker.java
  src/dateandtime/android/org/kde/kirigamiaddons/dateandtime/TimePicker.java
  src/dateandtime/lib/androidutils.cpp
  src/dateandtime/lib/androidutils.h
  src/dateandtime/lib/plugin.cpp
  src/dateandtime/qmldir

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

D29268: [WIP] Add Date/Time dialog

[Nicolas Fella]

nicolasfella added inline comments.

INLINE COMMENTS

> broulik wrote in CMakeLists.txt:69
> Why this only on Android? or is that used for that dummy library?

It's for the dummy library only

> broulik wrote in FindGradle.cmake:2
> Feels like this should go into ECM?

Yep

> broulik wrote in DateDialog.qml:12
> `org.kde.kirigamiaddons.private.dateandtime` to emphasis it's an impl detail

I'd rather do that in a separate patch since it should also be done for other 
classes

> broulik wrote in TimeDialog.qml:15
> 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)

In this particular case the timezone is not relevant though

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

D29268: [WIP] Add Date/Time dialog

[Nicolas Fella]

nicolasfella added a comment.


  In D29268#659586 , @vkrause wrote:
  
  > There might be ways around the native function registration issue from the 
QML thread, e.g. by using the alternative approach of exported (mangled) 
symbols instead: 
https://docs.oracle.com/javase/1.5.0/docs/guide/jni/spec/design.html -> 
"Loading and Linking Native Methods".
  
  
  I tried that, but it didn't seem to work.

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

D29268: [WIP] Add Date/Time dialog

[Volker Krause]

vkrause added a comment.


  There might be ways around the native function registration issue from the 
QML thread, e.g. by using the alternative approach of exported (mangled) 
symbols instead: 
https://docs.oracle.com/javase/1.5.0/docs/guide/jni/spec/design.html -> 
"Loading and Linking Native Methods".

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

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
> +
> +
> +

`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 
> +#include 
> +

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("doShow");

There's `QDateTime::currentMSecsSinceEpoch()`

> androidutils.h:10
> +#include 
> +#include 
> +#include 

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(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

D29268: [WIP] Add Date/Time dialog

[Nicolas Fella]

nicolasfella updated this revision to Diff 81471.
nicolasfella added a comment.


  - fix

REPOSITORY
  R1047 Kirigami Addons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29268?vs=81470&id=81471

BRANCH
  work

REVISION DETAIL
  https://phabricator.kde.org/D29268

AFFECTED FILES
  CMakeLists.txt
  KF5KirigamiAddonsConfig.cmake.in
  cmake/modules/FindGradle.cmake
  cmake/modules/local.properties.in
  cmake/modules/settings.gradle.in
  src/dateandtime/CMakeLists.txt
  src/dateandtime/DateDialog.qml
  src/dateandtime/KF5KirigamiDateAndTimeAndroid-android-dependencies.xml
  src/dateandtime/TimeDialog.qml
  src/dateandtime/android/AndroidManifest.xml
  src/dateandtime/android/CMakeLists.txt
  src/dateandtime/android/build.gradle
  src/dateandtime/android/org/kde/kirigamiaddons/dateandtime/DatePicker.java
  src/dateandtime/android/org/kde/kirigamiaddons/dateandtime/TimePicker.java
  src/dateandtime/lib/androidutils.cpp
  src/dateandtime/lib/androidutils.h
  src/dateandtime/lib/plugin.cpp
  src/dateandtime/qmldir

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

D29268: [WIP] Add Date/Time dialog

[Nicolas Fella]

nicolasfella created this revision.
nicolasfella added reviewers: davidedmundson, vkrause, broulik.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
nicolasfella requested review of this revision.

REVISION SUMMARY
  Import the date/time picker used in KTrip. It wraps the existing date/time 
picker components into a dialog. On Android it calls the native date/time 
picker instead.
  
  The Android implementation is slightly messy since we apparently can't do JNI 
things directly from a QML plugin because it gets loaded in the wrong thread. 
As a workaround we have a linkable library on Android that forces JNI 
initialization in the right thread.
  
  Finish

TEST PLAN
  Ported KTrip to it

REPOSITORY
  R1047 Kirigami Addons

BRANCH
  work

REVISION DETAIL
  https://phabricator.kde.org/D29268

AFFECTED FILES
  CMakeLists.txt
  KF5KirigamiAddonsConfig.cmake.in
  cmake/modules/FindGradle.cmake
  cmake/modules/local.properties.in
  cmake/modules/settings.gradle.in
  src/dateandtime/CMakeLists.txt
  src/dateandtime/DateDialog.qml
  src/dateandtime/KF5KirigamiDateAndTimeAndroid-android-dependencies.xml
  src/dateandtime/TimeDialog.qml
  src/dateandtime/android/AndroidManifest.xml
  src/dateandtime/android/CMakeLists.txt
  src/dateandtime/android/build.gradle
  src/dateandtime/android/org/kde/kirigamiaddons/dateandtime/DatePicker.java
  src/dateandtime/android/org/kde/kirigamiaddons/dateandtime/TimePicker.java
  src/dateandtime/lib/androidutils.cpp
  src/dateandtime/lib/androidutils.h
  src/dateandtime/lib/plugin.cpp
  src/dateandtime/qmldir

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