Add QML Support using Qt6 #236

CMGeorge · 2023-05-23T11:06:26Z

All changes did not affects your iplementation.
Addes some Q_INVOKABLE and Q_PROPERTY + minor changes using #ifdef
Tested for the moment only o Mac but should work everywhere

Only thing you should take in consideration is the set of
SET(CMAKE_AUTOMOC ON)
and remove of
QT_WRAP_CPP(qtkeychain_MOC_OUTFILES keychain.h keychain_p.h gnomekeyring_p.h)

QT_WRAP_CPP iterfer with crating qmltypes
Full informations:

New:

Added QML support on Qt6 using cmake
To activate set the qt6 and qml flags

-DBUILD_WITH_QT6=ON -DQTKEYCHAIN_BUILD_WITH_QML=ON

import QtKeychain 1.0

WritePasswordJob{
        id: storeJobObject
        service: ""
        onFinished: {
            console.debug("Store password complete")
        }
    }
    ReadPasswordJob{
        id: readJobObject
        service: ""

    }

storeJobObject.key = "username";
storeJobObject.setTextData("password");
storeJobObject.start();

readJobObject.key = "username"
readJobObject.finished.connect(function (returnedPassword){
                console.debug("Password is: "+returnedPassword.textData())
})
readJobObject.start();

…qtkeychain into feature/Origin_version

frankosterfeld

Great initiative :)

keychain.h

frankosterfeld · 2023-05-23T12:28:09Z

keychain.h

     * @see finished()
     */
-    void start();
+    Q_INVOKABLE void start();


Make this a slot

frankosterfeld · 2023-05-23T12:28:27Z

keychain.h

     * @return An error message that might provide details if error() returns OtherError.
     */
-    QString errorString() const;
+    Q_INVOKABLE QString errorString() const;


I'd add a property for this

frankosterfeld · 2023-05-23T12:29:42Z

keychain.h

-    explicit WritePasswordJob( const QString& service, QObject* parent=nullptr );
+    #ifdef BUILD_WITH_QML
+        //make objecte creatabble from QML - Just to make sure original code will not broke
+        explicit WritePasswordJob(const QString& service="", QObject* parent=nullptr );


I think we don't need the ifdef here, I'd just make it const QString &service={} for both use cases

frankosterfeld · 2023-05-23T12:30:29Z

translations/qtkeychain_de.ts

Please revert the changes to the .ts files (I should really remove these updates from the build)

frankosterfeld · 2023-05-23T12:36:25Z

CMakeLists.txt

 else()
  find_package(Qt6 COMPONENTS Core REQUIRED)
+  if(QTKEYCHAIN_BUILD_WITH_QML)
+      find_package(Qt6 COMPONENTS QML REQUIRED)


I'm a bit unsure how this dependency fits in with distro packages and the like, one can either disable the dependency to not have the qtkeychain package depend on QML, or enable it, and add the dependency. I guess it's maximally annoying for application developers if then some distros enable QML support and some ignore the flag and thus don't.
I'm leaning to enable QML support by default, and let those who want a minimal build do custom builds with the flag set to OFF.

As a developer of an offscreen library depending on Qt Keychain I'd rather be happy not to drag the dependency on QML. As for the CMake part, I think there's a way to introduce this dependency down the line in *Config.cmake file instead and that can even be conditional, but I didn't try it.

Are you relying on distro packages, or are pulling in qtkeychain directly?

In CI, I pull Qt Keychain from a tag and build it directly because it's the only way to have one unified logic across 3 platforms; but distros packaging my library have all reasons to link it externally.

frankosterfeld · 2023-05-23T12:37:19Z

ReadMe.md

+```QML
+WritePasswordJob{
+        id: storeJobObject
+        service: ""


Maybe set something here for demonstration? Or remove the line

frankosterfeld · 2023-05-23T12:37:57Z

ReadMe.md

+```
+
+```QML
+import QtKeychain 1.0


Need to think about versioning here. Should it follow the qtkeychain versioning (then this would be 0.15), or not?

Gonna set it to
VERSION "${CMAKE_PROJECT_VERSION_MAJOR}.${CMAKE_PROJECT_VERSION_MINOR}
Becaue the entire QML implementation is in fact the LIbrary so can track the library version.

frankosterfeld · 2023-05-23T12:39:19Z

keychain.h

-    explicit ReadPasswordJob( const QString& service, QObject* parent=nullptr );
+    #ifdef BUILD_WITH_QML
+        //make objecte creatabble from QML - Just to make sure original code will not broke
+        explicit ReadPasswordJob( const QString& service="", QObject* parent=nullptr );


think we don't need the ifdef here, I'd just make it const QString &service={} for both use cases

frankosterfeld · 2023-05-23T12:39:55Z

keychain.h

     * @see Job::key()
     */
-    QByteArray binaryData() const;
+    Q_INVOKABLE QByteArray binaryData() const;


I'd make these properties

vkrause · 2023-05-26T14:37:54Z

I'm wondering if there's a way to make the QML API more declarative, rather than exposing jobs more or less directly? Something like:

KeychainStore {
  id: password
  key: "myAccount"
}

TextField {
  id: passwordEdit
  text: password.textData // async reading of key chain data via property binding
}

Button {
  text: "Save"
  onClicked: password.textData = passwordEdit.text // assigning a new value triggers a write job
  enabled: password.state == KeychainStore.Loaded // state properties for indicating errors/progress/completion
}

The QSettings QML API might also be something to look at for this.

This can happen if a dependency CMake config module is itself also looking for QtKeychain, which then results in a fatal CMake error. Observed between NeoChat and libQuotient.

Also, the definitions needs to be fully qualified, otherwise this is producing symbols in the top-level namespace instead.

…Origin_version

…qtkeychain into feature/Origin_version # Conflicts: # qtkeychain/keychain_android.cpp # translations/qtkeychain_ru.ts

Calugar George added 7 commits May 23, 2023 11:41

Test add of qml module suport

881e4c1

Add automoc and remove manual cpp moc directive from cmake

a840d64

Make readn and write password creatable from QML

ab55929

Improve qml support allowing adding service name

0429e70

Add also setTextData setBinaryData as invokable to allow qml access

5a89b4b

Update readme

8da8c3f

Merge branch 'feature/Origin_version' of https://github.com/CMGeorge/…

624de97

…qtkeychain into feature/Origin_version

frankosterfeld requested changes May 23, 2023

View reviewed changes

vkrause and others added 11 commits June 1, 2023 10:38

Protect against creating the QtKeychain::QtKeychain alias target twice

2095295

This can happen if a dependency CMake config module is itself also looking for QtKeychain, which then results in a fatal CMake error. Observed between NeoChat and libQuotient.

Export QKeychain::isAvailable() to make it usable in a shared build

269eacf

Also, the definitions needs to be fully qualified, otherwise this is producing symbols in the top-level namespace instead.

Prepare 0.14.1

69f993c

Mino t changes

bcefcf8

Merge commit '69f993c47efed7e557d79a30a367014d9a27d809' into feature/…

30e4920

…Origin_version

Minor build fix at import

8c094de

Merge with iniitial library implementation

a4cb371

cmake cleanup

599b3f1

Try restore the qml support

4e98b29

Small fix using the old code

0442e18

Merge branch 'feature/Origin_version' of https://github.com/CMGeorge/…

13c6a81

…qtkeychain into feature/Origin_version # Conflicts: # qtkeychain/keychain_android.cpp # translations/qtkeychain_ru.ts

Add QML Support using Qt6 #236

Are you sure you want to change the base?

Add QML Support using Qt6 #236

Uh oh!

Conversation

CMGeorge commented May 23, 2023

New:

Uh oh!

frankosterfeld left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vkrause commented May 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants