Re: KHealthCertificate in kdereview
On Montag, 12. Juli 2021 13:36:11 CEST Harald Sitter wrote:
> My only gripe, besides what Albert already pointed out, is that all
> the properties are WRITEable but have no NOTIFY signal nor are they
> CONSTANT. One of those things ought to change. Considering only the
> certificate objects have properties and they are always constructed by
> the parsers I guess you could just drop the WRITE and make them
> CONSTANT?
CONSTANT vs NOTIFY is a bit pointless on Q_GADGETs as NOTIFY isn't possible
there at all, but WRITE is indeed neither needed nor desired here. Fixed.
> There is a somewhat different approach, that I only bring up because I
> do enjoy meta programming way too much: Currently the parsers have
> exhaustive if-else constructs to map incoming fields to setters. What
> you could do is make static maps from incoming keys to property key
> and then write the property filing in an abstract fashion e.g. to
>
> illustrate:
> > static QMap propertyMap{{"tg", "disease"}, {"fr",
> > "dateOfPositiveTest"}, ...}; while (reader.hasNext()) {
> > cert.setProperty(propertyMap.value(key), CborUtils::readString(reader));
> > // needs some error handling when key mapping fails and possibly type
> > conversion helpers }
>
> Then the WRITE would make sense and I'd use a single "changed()"
> signal to NOTIFY on all the properties. drkonqi's bugzilla library
> does something like that to model API objects for example.
Good idea in general, however tricky to apply here I think as the amount of
different types or ways of parsing the raw data is not significantly less than
the amount of properties, so I don't expect much to be gained by this.
Thanks!
Volker
signature.asc
Description: This is a digitally signed message part.
Re: KHealthCertificate in kdereview
On Montag, 12. Juli 2021 00:36:05 CEST Albert Astals Cid wrote: > El diumenge, 11 de juliol de 2021, a les 15:32:27 (CEST), Volker Krause va escriure: > > Hi, > > > > KHealthCertificate is a library for decoding digital vaccination, test and > > recovery certificates. Supported formats/features are: > > * EU DGC: almost all data found in vaccination, test and recovery > > certificates, verification of ECDSA and RSA/PSS signatures. > > * India: basic support for vaccination certificates, no signature > > verification yet. > > > > https://invent.kde.org/pim/khealthcertificate > > > > If you have more samples/information about the Indian system, or systems > > deployed elsewhere in the world, I'd be very much interested :) > > > > The first user of this is the basic vaccination certificate manager in > > Itinerary (currently optional), other potential users who have expressed > > interest are Qrca and MyGnuHealth, as well as a possible stand-alone > > vaccination certificate wallet app for Plasma Mobile. > > > > Beyond kdereview the goal would be to join an automated release process, > > Plasmo Gear is probably the best option of those given the expected users. > > > > A note on translations: The only user-visible strings right now are those > > in the EU DGC JSON files mapping various codes to human readable texts. > > Those datasets are supposed to be translated eventually, by upstream. > > This however isn't implemented yet apparently, the official apps are > > waiting for this as well (e.g. > > https://github.com/Digitaler-Impfnachweis/covpass-android/blob/ > > main/covpass-sdk/src/main/java/de/rki/covpass/sdk/storage/ > > EUValueSetRepository.kt#L11). So I'd rather wait for that getting fixed > > rather than bothering our translators with various medical product names > > :) > > > > For testing this you need a fairly recent KF5, KCodecs 5.84 for Base45 > > support and Prison 5.85 if your phone screen is too small for the usually > > very large barcodes. > > Rename some non installed headers in src/lib to *_p.h ? > > I tried reading the headers to see how i would use this and i was left with > some questionmarks. > > Am I supposed to call KHealthCertificateParser::parse? With what? Because it > says "data received from a barcode scanner" but i guess that's not the > QImage data? > > Also it returns a QVariant ? What's in there? Documentation has been expanded and private headers are renamed. Thanks! Volker signature.asc Description: This is a digitally signed message part.
Re: KHealthCertificate in kdereview
My only gripe, besides what Albert already pointed out, is that all
the properties are WRITEable but have no NOTIFY signal nor are they
CONSTANT. One of those things ought to change. Considering only the
certificate objects have properties and they are always constructed by
the parsers I guess you could just drop the WRITE and make them
CONSTANT?
There is a somewhat different approach, that I only bring up because I
do enjoy meta programming way too much: Currently the parsers have
exhaustive if-else constructs to map incoming fields to setters. What
you could do is make static maps from incoming keys to property key
and then write the property filing in an abstract fashion e.g. to
illustrate:
> static QMap propertyMap{{"tg", "disease"}, {"fr",
> "dateOfPositiveTest"}, ...};
> while (reader.hasNext()) {
> cert.setProperty(propertyMap.value(key), CborUtils::readString(reader));
> // needs some error handling when key mapping fails and possibly type
> conversion helpers
> }
Then the WRITE would make sense and I'd use a single "changed()"
signal to NOTIFY on all the properties. drkonqi's bugzilla library
does something like that to model API objects for example.
HS
Re: KHealthCertificate in kdereview
El diumenge, 11 de juliol de 2021, a les 15:32:27 (CEST), Volker Krause va escriure: > Hi, > > KHealthCertificate is a library for decoding digital vaccination, test and > recovery certificates. Supported formats/features are: > * EU DGC: almost all data found in vaccination, test and recovery > certificates, verification of ECDSA and RSA/PSS signatures. > * India: basic support for vaccination certificates, no signature > verification > yet. > > https://invent.kde.org/pim/khealthcertificate > > If you have more samples/information about the Indian system, or systems > deployed elsewhere in the world, I'd be very much interested :) > > The first user of this is the basic vaccination certificate manager in > Itinerary (currently optional), other potential users who have expressed > interest are Qrca and MyGnuHealth, as well as a possible stand-alone > vaccination certificate wallet app for Plasma Mobile. > > Beyond kdereview the goal would be to join an automated release process, > Plasmo Gear is probably the best option of those given the expected users. > > A note on translations: The only user-visible strings right now are those in > the EU DGC JSON files mapping various codes to human readable texts. Those > datasets are supposed to be translated eventually, by upstream. This however > isn't implemented yet apparently, the official apps are waiting for this as > well (e.g. https://github.com/Digitaler-Impfnachweis/covpass-android/blob/ > main/covpass-sdk/src/main/java/de/rki/covpass/sdk/storage/ > EUValueSetRepository.kt#L11). So I'd rather wait for that getting fixed > rather > than bothering our translators with various medical product names :) > > For testing this you need a fairly recent KF5, KCodecs 5.84 for Base45 > support > and Prison 5.85 if your phone screen is too small for the usually very large > barcodes. Rename some non installed headers in src/lib to *_p.h ? I tried reading the headers to see how i would use this and i was left with some questionmarks. Am I supposed to call KHealthCertificateParser::parse? With what? Because it says "data received from a barcode scanner" but i guess that's not the QImage data? Also it returns a QVariant ? What's in there? Cheers, Albert > > Thanks, > Volker
Re: KHealthCertificate in kdereview
Am Sonntag, 11. Juli 2021, 17:53:31 CEST schrieb Volker Krause: > done, with the exception of switching away from > KDEFrameworksCompilerSettings, that can only be done for 5.85, for earlier > versions that would require copying a bunch of settings it seems. Well, you still have some settings duplicated as of now, so does that really make a difference? ;) It is given a bad example, and cmake code tends to be copied around as magic lines, so please consider giving a good example when you can. Cheers Friedrich
Re: KHealthCertificate in kdereview
On Sonntag, 11. Juli 2021 16:07:37 CEST Friedrich W. H. Kossebau wrote:
> Quick feedback after looking at the cmake code:
>
> * do not explicitly list ECM_KDE_MODULE_DIR, it is part of ECM_MODULE_PATH
> * do not use KDEFrameworkCompilerSettings for non-KF projects, only
> KDECompilerSettings
> * no need to set CMAKE_AUTOMOC & CMAKE_AUTORCC, done by KDECompilerSettings
> for required ECM
> * set(CMAKE_CXX_STANDARD 17) before including KDECompilerSettings
> * call feature_summary as last thing, for consistency and some logic in
> execution
> * avoid REQUIRED in find_package() calls, only use in
> set_package_properties, so cmake will not cancel in middle of run, but get
> to listing found/notfound summary (Qt, ECM, KF are harder to do here due to
> macros expected later, and usually present, so fine there)
> * include the 3 KDE cmake setting modules as first and in this order:
> include(KDEInstallDirs)
> include(KDECMakeSettings)
> include(KDECompilerSettings NO_POLICY_SCOPE)
> * I recommend using set_target_properties() right next to add_${target},
> easier to find on need and product definition in one place makes more
> sense
done, with the exception of switching away from KDEFrameworksCompilerSettings,
that can only be done for 5.85, for earlier versions that would require
copying a bunch of settings it seems.
Thanks,
Volker
signature.asc
Description: This is a digitally signed message part.
Re: KHealthCertificate in kdereview
Am Sonntag, 11. Juli 2021, 16:07:37 CEST schrieb Friedrich W. H. Kossebau: > * no need to set CMAKE_AUTOMOC & CMAKE_AUTORCC, done by KDECompilerSettings > for required ECM KDECMakeSettings rather :) Cheers Friedrich
Re: KHealthCertificate in kdereview
Quick feedback after looking at the cmake code:
* do not explicitly list ECM_KDE_MODULE_DIR, it is part of ECM_MODULE_PATH
* do not use KDEFrameworkCompilerSettings for non-KF projects, only
KDECompilerSettings
* no need to set CMAKE_AUTOMOC & CMAKE_AUTORCC, done by KDECompilerSettings
for required ECM
* set(CMAKE_CXX_STANDARD 17) before including KDECompilerSettings
* call feature_summary as last thing, for consistency and some logic in
execution
* avoid REQUIRED in find_package() calls, only use in set_package_properties,
so cmake will not cancel in middle of run, but get to listing found/notfound
summary (Qt, ECM, KF are harder to do here due to macros expected later, and
usually present, so fine there)
* include the 3 KDE cmake setting modules as first and in this order:
include(KDEInstallDirs)
include(KDECMakeSettings)
include(KDECompilerSettings NO_POLICY_SCOPE)
* I recommend using set_target_properties() right next to add_${target},
easier to find on need and product definition in one place makes more sense
Otherwise looks clean and modern to me :)
Cheers
Friedrich
