Re: Adding experimental parts to a KF5 library

[Kevin Kofler]

Ivan Čukić wrote:
> I do agree that is would be a proper way to handle it. The only
> problem I see with it is that the point is actually not to provide
> binary compatibility, nor proper handling of BIC.
> 
> At least in the case I have. Namely, the point is for the library to
> be used *only* for things that are in development - because projects
> that wish to use it have a longer release cycles than the frameworks.
> But, on the other hand, if one of those projects were to release a
> stable version against a 0.x version of the library, it would need BIC
> handling.

What does giving the library a fixed (i.e. unchanging) soname improve there? 
We would still need to rebuild the programs for the new version of the 
library, but our packaging tools would NOT tell us that we need to do that. 
The result is packages that install without errors and then fail to run, 
which is very nasty. If you give it a 0.x soversion and remember to 
increment x on each BIC change, we will know to rebuild affected packages.

The only thing worse than ABI changes is SILENT ABI changes.

Kevin Kofler

Re: Review Request 121831: libksysguard: process.h: encapsulate private fields

[Gregor Mi]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121831/#review74100
---


see also last comments in https://git.reviewboard.kde.org/r/122072/ about 
redesigning it with d_ptr to make process.h future-proof

- Gregor Mi


On Jan. 12, 2015, 2:30 p.m., Gregor Mi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121831/
> ---
> 
> (Updated Jan. 12, 2015, 2:30 p.m.)
> 
> 
> Review request for KDE Base Apps and John Tapsell.
> 
> 
> Repository: libksysguard
> 
> 
> Description
> ---
> 
> In process.h there are several public fields. I propose to encapsulate the 
> private fields with getter methods. I implemented it exemplary for the fields 
> 'login', 'uid' and 'euid'.
> 
> 
> (In a separate commit I would add the .reviewboardrc file)
> 
> What is the current policy on using small C++ macros as done in this RR? Use 
> it (code is more compact and readable) or don't use it (better for debugging)?
> 
> 
> Diffs
> -
> 
>   processcore/process.h 85a3a13388c44f768040dbc6602ab3211edd5b21 
>   .reviewboardrc PRE-CREATION 
>   processcore/processes_linux_p.cpp 0cff0e8b407a087dc29f755b12ea3d784ba34e6a 
>   processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 
>   processui/ProcessModel.cpp 3acf52b92f4a8ca054d88aad1ec6b31f4a31f297 
>   processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 
>   processcore/process.cpp 190f4902fa6f3bae2d8b60dbf1a43be71beb1820 
> 
> Diff: https://git.reviewboard.kde.org/r/121831/diff/
> 
> 
> Testing
> ---
> 
> Compiles and runs. Data is still shown. No error.
> 
> 
> Thanks,
> 
> Gregor Mi
> 
>

Re: Review Request 121831: libksysguard: process.h: encapsulate private fields

[Gregor Mi]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121831/#review74096
---



processcore/process.h


Requires SOVERSION bump.

And: wouldn't it be better to move the now private variables behind a d_ptr?


- Gregor Mi


On Jan. 12, 2015, 2:30 p.m., Gregor Mi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121831/
> ---
> 
> (Updated Jan. 12, 2015, 2:30 p.m.)
> 
> 
> Review request for KDE Base Apps and John Tapsell.
> 
> 
> Repository: libksysguard
> 
> 
> Description
> ---
> 
> In process.h there are several public fields. I propose to encapsulate the 
> private fields with getter methods. I implemented it exemplary for the fields 
> 'login', 'uid' and 'euid'.
> 
> 
> (In a separate commit I would add the .reviewboardrc file)
> 
> What is the current policy on using small C++ macros as done in this RR? Use 
> it (code is more compact and readable) or don't use it (better for debugging)?
> 
> 
> Diffs
> -
> 
>   processcore/process.h 85a3a13388c44f768040dbc6602ab3211edd5b21 
>   .reviewboardrc PRE-CREATION 
>   processcore/processes_linux_p.cpp 0cff0e8b407a087dc29f755b12ea3d784ba34e6a 
>   processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 
>   processui/ProcessModel.cpp 3acf52b92f4a8ca054d88aad1ec6b31f4a31f297 
>   processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 
>   processcore/process.cpp 190f4902fa6f3bae2d8b60dbf1a43be71beb1820 
> 
> Diff: https://git.reviewboard.kde.org/r/121831/diff/
> 
> 
> Testing
> ---
> 
> Compiles and runs. Data is still shown. No error.
> 
> 
> Thanks,
> 
> Gregor Mi
> 
>

Re: SFLPhone-KDE is now Ring-KDE and is moving to kdereview

[Elv1313 .]

Hello Albert,

Thanks for your comments.

> * src/abstractitembackend.h is twice in libringclient_LIB_HDRS

I will fix that, thanks, maybe we should add a Krazy2 check for that.
I know Laurent Model has posted one on planetkde.org for the code
a while back, I haven’t used it yet.

> * You have some d-pointers in some of your classes but not on others, why is
that?

Umm, I was under the impression I had fixed that in the commit before posting
this email. Krazy2 doesn't seem to analyze LibRingClient yet, even if I
checked it on projects.kde.org. Maybe it does really take more than 5 days?
Anyhow, I will fix more of them. Some classes are really just templates aliases
+ QObject inheritance, so those obviously don't need d-pointers. Other simply
have no private members, I am not sure if I should add d-pointer to those just
in case, maybe I should.

> * There's a catch regarding translations. You are using
> $XGETTEXT_QT in your Messages.sh. That creates a .po file that only works for
> clients that access those translations via kdelibs4 i18n() (or maybe other
> gettext wrappers) but not (i think) via kf5 ki18n() or plain Qt tr(). For Qt5
> we have $EXTRACT_TR_STRINGS that creates a proper .tr file to use with Qt5 (no
> idea if it works with Qt4, maybe it does). This is a bit weird too because
> your branch has both Qt4 and Qt5 support in the same branch but our
> translation system is build around the expectation that there will be separate
> branches and thus they will have different Messages.sh catering for each qt4
> and qt5. At this stage I'm not sure what's the best thing for you guys to do
> :/ Are you mainly targetting qt4 or qt5 for clients to use?

The problem is that we are doing a very major release right now and the kf5 port
is not ready yet, so it wont make the cut for our deadlines. There is 2 or 3
other projects trying to release a fully decentralized and secure communication
playform right now. We think we still are the closest to that, so we hurry up.
I prefer to spend my time on security details than KF5 port for the next month,
so we will miss the spring distribution cycle for KF5. After the 2.0 release,
getting rid of the Qt4 support will be something I will do. I don't care about
Qt4 at this point, but the KDE ui is still using KDE4 and I also wait for KDEPim
kf5 support to mature before doing the switch. The others clients, such as GTK
and OS X, use Qt5 and some patches are starting to be Qt5 only, but I don't
want to drop Qt4 until about mid-March. I guess translation support that works
on KDE4 is better than nothing for now. The new OSX and Gnome client don't have
any translations yet anyway, so the fact that the few tr() in libringclient are
not translated don't have an impact on them yet.

As you saw, it also cause the CMakeLists.txt to be ugly and bloated and I also
have to keep the deprecated/qt4support flag on. I also can't use the abstract
proxy model and other classes that made their way into QtCore, nor switch to
the much easier to lint connect() syntax. There is quite a few //TODO qt5 in
the code. This is a (temporary) very bad situation...

Regards,
Emmanuel