Re: CMake config & target challenges on moving to KF5 namespace; dir structure & API dox (Re: Submitting Grantlee as a KF5 Framework)

[Friedrich W. H. Kossebau]

Hi Stephen,

Am Sonntag, 29. Dezember 2019, 00:10:50 CET schrieb Stephen Kelly:
> On 22/12/2019 16:08, Stephen Kelly wrote:
> > On 21/12/2019 23:55, Friedrich W. H. Kossebau wrote:
> >> Perhaps joining the "Release Service" (formerly known as "KDE
> >> Applications")
> >> is a better place then, it also contains a set of libraries already.
> >> That would serve the purpose of having releases happening regularly.
> > 
> > The goals of making Grantlee a Framework are:
> > 
> > * Make more frequent releases which don't depend on me
> > 
> > * Make it more easy for others to contribute to development
> > 
> > 
> > I think at the point that renaming happens, the name Grantlee will
> > disappear, and we'll have two libraries (KF5::TextDocument and
> > KF5::TextTemplates or so in CMake and probably removing the C++
> > namespace).
> > 
> > I think all of that should be done together and I don't think that
> > should be done until compatibility is broken to become Qt6-based (KF6).
> 
> My conclusion from reading this thread is that this is the way forward:
> 
> * Grantlee does not become a KF5 framework. I'll continue to make
> releases from github if needed based on Qt 5. We can consider
> re-evaluating that in the future.

Not becoming a KF5 module should not stop you/us making Grantlee a project now 
developed in the KDE community. Given your two goals cited above, making 
Grantlee hosted on KDE resources and releasing it as part of the "release 
service" should satisfy your goals already now.

And it would allow to make Grantlee already now move as close enough as 
possible to KF standards, besides the naming ones. So that at KF6 time only 
those things are left to do both on producer & consumer side which are about 
ABI breakage.

Why are you proposing to do a step back instead to the old state, which 
everyone including you considered not that satisfying?

I hope my personal objections raised about it becoming an official KF module 
already now have not arrived with you as objection in general. On the 
opposite, I agree with the ideas behind this move. I have just strict feeling 
about KF as a product itself when it comes to consumer as well as cross-module 
ontributor experience.
So please be aware that I would be sad if you decided to have Grantlee go back 
to lonely cowboy mode :)

Cheers
Friedrich

Re: CMake config & target challenges on moving to KF5 namespace; dir structure & API dox (Re: Submitting Grantlee as a KF5 Framework)

[Stephen Kelly]

On 22/12/2019 16:08, Stephen Kelly wrote:


On 21/12/2019 23:55, Friedrich W. H. Kossebau wrote:
Perhaps joining the "Release Service" (formerly known as "KDE 
Applications")

is a better place then, it also contains a set of libraries already.
That would serve the purpose of having releases happening regularly.



The goals of making Grantlee a Framework are:

* Make more frequent releases which don't depend on me

* Make it more easy for others to contribute to development


I think at the point that renaming happens, the name Grantlee will 
disappear, and we'll have two libraries (KF5::TextDocument and 
KF5::TextTemplates or so in CMake and probably removing the C++ 
namespace).


I think all of that should be done together and I don't think that 
should be done until compatibility is broken to become Qt6-based (KF6).



My conclusion from reading this thread is that this is the way forward:

* Grantlee does not become a KF5 framework. I'll continue to make 
releases from github if needed based on Qt 5. We can consider 
re-evaluating that in the future.


* For KF6, I will submit two separate Tier 1 frameworks, in two 
different repos, what I here called ktextdocument/KF6::TextDocument and 
ktexttemplate/KF6::TextTemplate


The two libraries are independent. Having them in the same KF* repo 
doesn't make sense.


Thanks,

Stephen.

Re: Keysmith in kdereview

[Christoph Feck]

On 12/28/19 19:17, Jesus Varela wrote:

I am new and have tons of questions about all these emails I see.

From what I understand there is software being added to the core KDE
framework. Will this make the framework less desired due to it being
bloated with tons of features? Or is it desired to have more features even
if it makes the framework heavier?

I hear about a KDE/QT powered phone in the works but then I wonder how a
huge framework like KDE will work on a mobile device. Is there a way to
break the framework into parts that only deploy the necessary pieces?

Sorry for the noob questions. Please refer me to any docs I should read.


Our framework is already splitted into dozens of individual libraries, 
see https://techbase.kde.org/KDE_Frameworks


--
Christoph Feck

Re: Keysmith in kdereview

[Jesus Varela]

Hi everyone,

I am new and have tons of questions about all these emails I see.

>From what I understand there is software being added to the core KDE
framework. Will this make the framework less desired due to it being
bloated with tons of features? Or is it desired to have more features even
if it makes the framework heavier?

I hear about a KDE/QT powered phone in the works but then I wonder how a
huge framework like KDE will work on a mobile device. Is there a way to
break the framework into parts that only deploy the necessary pieces?

Sorry for the noob questions. Please refer me to any docs I should read.

Thanks,

JV

On Sat, Dec 28, 2019, 11:39 Friedrich W. H. Kossebau 
wrote:

> Sending my reply on-list while Johan's resent reply to list seems stuck in
> moderation:
>
> Am Samstag, 28. Dezember 2019, 15:03:33 CET schrieb Johan Ouwerkerk:
> > On Wed, Dec 18, 2019 at 5:43 PM Friedrich W. H. Kossebau
> >
> >  wrote:
> > > Other things noticed on superficial look:
> > > * UI not translated, i18n support setup missing completely
> >
> > Yes, there is an issue for that on invent:
> > https://invent.kde.org/kde/keysmith/issues/5
>
> That one is a blocker though to pass kdereview, for what I understand from
> https://community.kde.org/ReleasingSoftware#Sanity_Checklist as linked
> from
> https://community.kde.org/Policies/Application_Lifecycle#kdereview
>
> At least personally I would expect this to be a minimum requirement for
> software created officially in the KDE community. Perhaps new generation
> of
> KDE develpoers thinks differently, but in that case please reconsider
> whether
> i18n is not a fundamental need :)
>
> > > * uses own "ENABLE_TESTING", not "BUILD_TESTING" flag from
> > > KDECompilerSettings>
> > >   proposed:
> > >   + switch flag use to BUILD_TESTING
> > >   - remove option(ENABLE_TESTING "Enable tests" ON)
> > >   - remove enable_testing() (done by KDECompilerSettings)
>
> My bad, s/KDECompilerSettings/KDECMakeSettings/g here.
>
> > I'm not entirely sure what the origins of this are but see also the CI
> > template for building flatpaks:
> >
> https://invent.kde.org/sysadmin/ci-tooling/raw/master/invent/binary-flatpak
> .
> > yml As you can see from the example usage the `-DENABLE_TESTING` flag is
> > suggested there.
> >
> > Speaking as a developer I don't really care what it is called, but I
> > would like it to be on by default. Is that the case for
> > `BUILD_TESTING`?
>
> Main motivation here is consistency in the code created in the KDE
> community,
> so people working across KDE projects do not have to switch mind all the
> time.
>
> Yes, BUILD_TESTING is ON by default, either indirectly via CTest or
> explicit,
> see
> https://phabricator.kde.org/source/extra-cmake-modules/browse/master/kde-modules/KDECMakeSettings.cmake$190
> and https://cmake.org/cmake/help/latest/
> module/CTest.html 
>
> The people doing flatpaks want to fix the template, please poke them to do
> so
> (did already a comment on the commit myself, but someone needs to run
> after
> this to also happen :) ).
>
> Cheers
> Friedrich
>
>
>

Re: Keysmith in kdereview

[Johan Ouwerkerk]

First of all sorry for the duplicate reply Friedrich. I messed up with
the send button earlier.

Here goes the second try:

>
> Did some usual-nitpick-CMake-code cleanup commit already (things which also
> apply to other new Plasma repos, someone might want to brush over their
> CMakeLists.txt as well, using that commit as reference).
>

Thanks for that one!

>
> Other things noticed on superficial look:
> * UI not translated, i18n support setup missing completely

Yes, there is an issue for that on invent:
https://invent.kde.org/kde/keysmith/issues/5

> * uses own "ENABLE_TESTING", not "BUILD_TESTING" flag from KDECompilerSettings
>   proposed:
>   + switch flag use to BUILD_TESTING
>   - remove option(ENABLE_TESTING "Enable tests" ON)
>   - remove enable_testing() (done by KDECompilerSettings)

I'm not entirely sure what the origins of this are but see also the CI
template for building flatpaks:
https://invent.kde.org/sysadmin/ci-tooling/raw/master/invent/binary-flatpak.yml
As you can see from the example usage the `-DENABLE_TESTING` flag is
suggested there.

Speaking as a developer I don't really care what it is called, but I
would like it to be on by default. Is that the case for
`BUILD_TESTING`?

> * you might want to check if KF 5.37 has minimum version of Kirigami features
> used, otherwise needs bumping
>

Good point: I've created a MR for this:
https://invent.kde.org/kde/keysmith/merge_requests/26

Regards,

 - Johan

Re: Keysmith in kdereview

[Johan Ouwerkerk]

First of all: sorry for the duplicate reply Albert. There was a mixup
with the reply button on my end, unfortunately.

Here goes attempt #2:

> I think it'd be good if you used a QVarLengthArray instead of "char 
> code[m_pinLength];"
>

Yes that is a known wart. There are a few issues/MRs that touch on
this particular point:

 - https://invent.kde.org/kde/keysmith/merge_requests/24 : as a side
effect of refactoring it is taken care of in a proposed MR to fix
issue 2: https://invent.kde.org/kde/keysmith/issues/2
 - https://invent.kde.org/kde/keysmith/issues/6 : we still have to
implement encryption/decryption of secrets and that may also impact
the code that generates tokens
 - https://invent.kde.org/kde/keysmith/issues/9 : we may want to drop
liboath (oath-toolkit) due to its limitations and quirks and
difficulty getting it to build on platforms where it is not packaged
(i.e. Android)

We don't need to use QVarLengthArray or any VLA, really: this is not
performance critical and pre-allocating a suitably sized QByteArray
does the job just as well but in a format that is a bit more natural
for converting into QString later (QString::fromUtf8). But the general
criticism of code like `char code[m_pinLength];` is definitely valid
:)

Regards,

 - Johan

Re: Keysmith in kdereview

[Friedrich W. H. Kossebau]

Sending my reply on-list while Johan's resent reply to list seems stuck in 
moderation:

Am Samstag, 28. Dezember 2019, 15:03:33 CET schrieb Johan Ouwerkerk:
> On Wed, Dec 18, 2019 at 5:43 PM Friedrich W. H. Kossebau
> 
>  wrote:
> > Other things noticed on superficial look:
> > * UI not translated, i18n support setup missing completely
> 
> Yes, there is an issue for that on invent:
> https://invent.kde.org/kde/keysmith/issues/5

That one is a blocker though to pass kdereview, for what I understand from 
https://community.kde.org/ReleasingSoftware#Sanity_Checklist as linked from 
https://community.kde.org/Policies/Application_Lifecycle#kdereview

At least personally I would expect this to be a minimum requirement for 
software created officially in the KDE community. Perhaps new generation of 
KDE develpoers thinks differently, but in that case please reconsider whether 
i18n is not a fundamental need :)

> > * uses own "ENABLE_TESTING", not "BUILD_TESTING" flag from
> > KDECompilerSettings> 
> >   proposed:
> >   + switch flag use to BUILD_TESTING
> >   - remove option(ENABLE_TESTING "Enable tests" ON)
> >   - remove enable_testing() (done by KDECompilerSettings)

My bad, s/KDECompilerSettings/KDECMakeSettings/g here.

> I'm not entirely sure what the origins of this are but see also the CI
> template for building flatpaks:
> https://invent.kde.org/sysadmin/ci-tooling/raw/master/invent/binary-flatpak.
> yml As you can see from the example usage the `-DENABLE_TESTING` flag is
> suggested there.
> 
> Speaking as a developer I don't really care what it is called, but I
> would like it to be on by default. Is that the case for
> `BUILD_TESTING`?

Main motivation here is consistency in the code created in the KDE community, 
so people working across KDE projects do not have to switch mind all the time.

Yes, BUILD_TESTING is ON by default, either indirectly via CTest or explicit, 
see 
https://phabricator.kde.org/source/extra-cmake-modules/browse/master/kde-modules/KDECMakeSettings.cmake$190
 and https://cmake.org/cmake/help/latest/
module/CTest.html

The people doing flatpaks want to fix the template, please poke them to do so 
(did already a comment on the commit myself, but someone needs to run after 
this to also happen :) ).

Cheers
Friedrich