Re: libs/kworkspace/kdisplaymanager.cpp mess
On Thu, Apr 25, 2013 at 03:11:25PM +0200, Martin Bříza wrote: after fixing a bit in the $subj file I've realized it (in my opinion) should be split into one abstract class with a factory handling the back-ends provided by the current session managers such as ConsoleKit and systemd-logind while providing one module for the legacy DMs with their socket communication protocols. To provide reasons why to do it: - The current state is nearly unmaintainable that tiny file? you have a low threshold. ;) and having all three (for now) ways of session handling in one file doesn't feel very clean. it was the best at the time of creation due to the amount of shared code (the old gdm and kdm socket code was identical except for the string literals and a few conditionals). the ck code was tacked on later. at this point the old gdm code can be probably purged, making the kdm code independent. you should do that in a separate first commit. the ck and systemd paths may share some code, though. maybe add a DBusUtils class. I would leave creating the CK module to somebody who is experienced with how exactly the whole system works and knows if it is safe. good luck with that. i'll do reviews, not a bit more. note that partial works (be it regressions or just a highly asymmetrical code structure) will not be accepted. if you don't find somebody to do the refactoring for you, you need to take the route of minimal modification.
Re: Review Request 110083: Make kdepim libs optional dependency for libkfbapi
On April 26, 2013, 8:09 a.m., Kevin Krammer wrote: I am wondering if it wouldnt't make more sense to have the PIM integration as a separate lib, so that libkfbapi has a stable API and ABI at all times. I like that idea. I'll update the review with it then :) - Martin --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110083/#review31608 --- On April 26, 2013, 7:37 a.m., Martin Klapetek wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110083/ --- (Updated April 26, 2013, 7:37 a.m.) Review request for kdelibs and KDEPIM-Libraries. Description --- kdepim-libs are needed in the facebook library only to return user info as KABC::Addressee and note as KMime::Message, plus events use KCalCore classes. This patch makes dependency on kdepim-libs optional and disables building the event classes completely in case kdepim-libs was not found. Diffs - CMakeLists.txt 5aefdcf libkfbapi/CMakeLists.txt dac62bc libkfbapi/commentinfo.h e5578c7 libkfbapi/config.h.cmake PRE-CREATION libkfbapi/likeinfo.h e06052e libkfbapi/noteinfo.h 2492db1 libkfbapi/noteinfo.cpp 154593d libkfbapi/notificationinfo.h a882694 libkfbapi/userinfo.h ac22a7e libkfbapi/userinfo.cpp 26c64da libkfbapi/userinfoparser_p.h 0189a17 Diff: http://git.reviewboard.kde.org/r/110083/diff/ Testing --- Builds correctly here in both cases Thanks, Martin Klapetek
Re: Review Request 110083: Make kdepim libs optional dependency for libkfbapi
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110083/ --- (Updated April 29, 2013, 11:17 a.m.) Review request for kdelibs and KDEPIM-Libraries. Changes --- Update libkfbapi to have the kdepim interface as optional additional library Description --- kdepim-libs are needed in the facebook library only to return user info as KABC::Addressee and note as KMime::Message, plus events use KCalCore classes. This patch makes dependency on kdepim-libs optional and disables building the event classes completely in case kdepim-libs was not found. Diffs (updated) - CMakeLists.txt 5aefdcf libkfbapi/CMakeLists.txt dac62bc libkfbapi/commentinfo.h e5578c7 libkfbapi/kdepim-utils.h PRE-CREATION libkfbapi/kdepim-utils.cpp PRE-CREATION libkfbapi/likeinfo.h e06052e libkfbapi/noteinfo.h 2492db1 libkfbapi/noteinfo.cpp 154593d libkfbapi/notificationinfo.h a882694 libkfbapi/userinfo.h ac22a7e libkfbapi/userinfo.cpp 26c64da libkfbapi/userinfoparser_p.h 0189a17 Diff: http://git.reviewboard.kde.org/r/110083/diff/ Testing --- Builds correctly here in both cases Thanks, Martin Klapetek
Re: Review Request 110083: Make kdepim libs optional dependency for libkfbapi
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110083/ --- (Updated April 29, 2013, 11:18 a.m.) Review request for kdelibs and KDEPIM-Libraries. Changes --- Upload correct patch :) Description --- kdepim-libs are needed in the facebook library only to return user info as KABC::Addressee and note as KMime::Message, plus events use KCalCore classes. This patch makes dependency on kdepim-libs optional and disables building the event classes completely in case kdepim-libs was not found. Diffs (updated) - CMakeLists.txt 5aefdcf LibKFbAPI-KDEPIM.pc.in PRE-CREATION LibKFbAPI-KDEPIMConfig.cmake.in PRE-CREATION LibKFbAPI.pc.in af537d1 libkfbapi/CMakeLists.txt dac62bc libkfbapi/commentinfo.h e5578c7 libkfbapi/kdepim-utils.h PRE-CREATION libkfbapi/kdepim-utils.cpp PRE-CREATION libkfbapi/likeinfo.h e06052e libkfbapi/noteinfo.h 2492db1 libkfbapi/noteinfo.cpp 154593d libkfbapi/notificationinfo.h a882694 libkfbapi/userinfo.h ac22a7e libkfbapi/userinfo.cpp 26c64da libkfbapi/userinfoparser_p.h 0189a17 Diff: http://git.reviewboard.kde.org/r/110083/diff/ Testing --- Builds correctly here in both cases Thanks, Martin Klapetek
Re: libs/kworkspace/kdisplaymanager.cpp mess
Hi, On Fri, 26 Apr 2013 23:30:43 +0200, Albert Astals Cid aa...@kde.org wrote: El Dijous, 25 d'abril de 2013, a les 15:11:25, Martin Briza va escriure: after fixing a bit in the $subj Which repo is that? Are we installing that file header? The systemd support fix is now in master branch, if you meant that. Header of the library is public, yes. Kind Regards, Martin
Re: libs/kworkspace/kdisplaymanager.cpp mess
On Mon, Apr 29, 2013 at 03:48:35PM +0200, Martin Briza wrote: On Mon, 29 Apr 2013 09:17:15 +0200, Oswald Buddenhagen o...@kde.org wrote: note that partial works (be it regressions or just a highly asymmetrical code structure) will not be accepted. if you don't find somebody to do the refactoring for you, you need to take the route of minimal modification. It will work the same as it does now but I'll split the systemd code away from the main (let's say legacy) part of the code. this is what i mean with highly asymmetrical. no-go.
Re: libs/kworkspace/kdisplaymanager.cpp mess
Le Monday 29 April 2013 15:47:40 Martin Briza a écrit : Hi, On Fri, 26 Apr 2013 23:30:43 +0200, Albert Astals Cid aa...@kde.org wrote: El Dijous, 25 d'abril de 2013, a les 15:11:25, Martin Briza va escriure: after fixing a bit in the $subj Which repo is that? Are we installing that file header? The systemd support fix is now in master branch, if you meant that. Header of the library is public, yes. I guess Albert was wondering about the git repository. This file is in the kde-workspace git repo. Aurélien
Initialization of QSharedDataPointer's for empty objects (i.e. Foo::Foo(void))
Hey guys!
On the PIM mailing list was recently a post about excessive memory consumption
in a data structure which stores multiple shared data objects
(KABC::Addressee).
Looking at it, I noticed that all the shared data objects always allocate
their private data, even if it is potentially empty.
This is apparently also the way its being done in the documentation, see [1].
I guess that is a oversimplification and wonder what you all think. Imo,
classes that use QSharedData should also use the shared_empty/null pattern
that QString e.g. uses. Otherwise it can be quite costly to create many such
items even though they are empty.
I.e.: In the ctors that construct an empty object do not call new Private
but instead (re-)use a static shared empty object created on the stack - see
shared_empty and shared_null in qstring.{cpp,h}.
I do wonder though why QString has both, a shared_empty and a shared_null
object. A single one should suffice for most objects, no?
Does anyone have any experience with this? Are there any pitfalls or downsides
to the approach taken by QString? I also thought about not initializing the
QSaredDataPointer at all but that would require a lot of additional code when
its being used to ensure its only accessed when it is initialized. I think the
pattern applied in QString is much nicer in that regard.
[1]: http://qt-project.org/doc/qt-4.8/qshareddatapointer.html
--
Milian Wolff
m...@milianw.de
http://milianw.de
signature.asc
Description: This is a digitally signed message part.
Re: Initialization of QSharedDataPointer's for empty objects (i.e. Foo::Foo(void))
On 2013-04-29, Milian Wolff m...@milianw.de wrote: I do wonder though why QString has both, a shared_empty and a shared_null object. A single one should suffice for most objects, no? I'm pretty sure this is because QString can be empty and not null. e.g. the difference between QString() the empty string. /Sune
Re: Initialization of QSharedDataPointer's for empty objects (i.e. Foo::Foo(void))
On segunda-feira, 29 de abril de 2013 21.52.37, Milian Wolff wrote:
I.e.: In the ctors that construct an empty object do not call new
Private but instead (re-)use a static shared empty object created on the
stack - see shared_empty and shared_null in qstring.{cpp,h}.
You probably did not mean what you said: you do not want something allocated
on the stack. You want a global. Like QString :-)
To be *really* like QString, you want your private to be POD. That means you
cannot derive from QSharedData, you cannot use QAtomicInt, QString or
QByteArray. That may be taking it a bit too far.
If your private class is not POD, you should use a K/Q_GLOBAL_STATIC instead
to hold your private. Examples:
corelib/mimetypes/qmimedatabase.cpp:Q_GLOBAL_STATIC(QMimeDatabasePrivate,
staticQMimeDatabase)
corelib/tools/qlocale_win.cpp:Q_GLOBAL_STATIC(QSystemLocalePrivate,
systemLocalePrivate)
gui/text/qfontdatabase.cpp:Q_GLOBAL_STATIC(QFontDatabasePrivate, privateDb)
network/access/qabstractnetworkcache.cpp:Q_GLOBAL_STATIC(QNetworkCacheMetaDataPrivate,
metadata_shared_invalid)
plugins/bearer/qnetworksession_impl.cpp:Q_GLOBAL_STATIC(QNetworkSessionManagerPrivate,
sessionManager);
sql/kernel/qsqlquery.cpp:Q_GLOBAL_STATIC_WITH_ARGS(QSqlQueryPrivate,
nullQueryPrivate, (0))
I do wonder though why QString has both, a shared_empty and a shared_null
object. A single one should suffice for most objects, no?
QString().isNull() == true
QString().isNull() == false
Does anyone have any experience with this? Are there any pitfalls or
downsides to the approach taken by QString? I also thought about not
initializing the QSaredDataPointer at all but that would require a lot of
additional code when its being used to ensure its only accessed when it is
initialized. I think the pattern applied in QString is much nicer in that
regard.
It is, but the pattern of having a null d pointer also exists elsewhere in Qt.
For example, in QUrl, it's implemented manually. But there are also a few
examples of doing that with QSharedDataPointer.
Take a look at QNetworkProxy or QUrlQuery:
template void QSharedDataPointerQNetworkProxyPrivate::detach()
{
if (d d-ref.load() == 1)
return;
QNetworkProxyPrivate *x = (d ? new QNetworkProxyPrivate(*d)
: new QNetworkProxyPrivate);
x-ref.ref();
if (d !d-ref.deref())
delete d;
d = x;
}
And then:
QNetworkProxy::ProxyType QNetworkProxy::type() const
{
return d ? d-type : DefaultProxy;
}
--
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
Software Architect - Intel Open Source Technology Center
PGP/GPG: 0x6EF45358; fingerprint:
E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358
signature.asc
Description: This is a digitally signed message part.
Re: Initialization of QSharedDataPointer's for empty objects (i.e. Foo::Foo(void))
On Monday 29 April 2013 15:16:29 Thiago Macieira wrote:
On segunda-feira, 29 de abril de 2013 21.52.37, Milian Wolff wrote:
I.e.: In the ctors that construct an empty object do not call new
Private but instead (re-)use a static shared empty object created on the
stack - see shared_empty and shared_null in qstring.{cpp,h}.
You probably did not mean what you said: you do not want something allocated
on the stack. You want a global. Like QString :-)
Yes of course - thanks for the correction.
To be *really* like QString, you want your private to be POD. That means you
cannot derive from QSharedData, you cannot use QAtomicInt, QString or
QByteArray. That may be taking it a bit too far.
If your private class is not POD, you should use a K/Q_GLOBAL_STATIC instead
to hold your private.
No, I don't think making it a POD is required. {K,Q}_GLOBAL_STATIC with
Q_MOVABLE_TYPE should be sufficient in most cases I think.
Examples:
snip
Interestingly I can't find a case cases don't seem to be using
QSharedDataPointer, or am I missing something?
And is the following not OK due to random static initialization order in
C++03? http://paste.kde.org/734738/
But in C++11 it should be fine, no? The alternative using {K,Q}_GLOBAL_STATIC
is imo much uglier, see: http://paste.kde.org/734750/
I could esp. not make it work with a private Private class as used in the
previous code snippet. I.e. this: http://paste.kde.org/734756/ triggers the
following compile error:
/home/milian/projects/foo/src/main.cpp: In function
‘QSharedDataPointerFoo::Private* s_sharedEmpty()’:
/home/milian/projects/foo/src/main.cpp:21:56: error:
‘QSharedDataPointerFoo::Private* s_sharedEmpty()’ was declared ‘extern’ and
later ‘static’ [-fpermissive]
Q_GLOBAL_STATIC(QSharedDataPointerFoo::Private, s_sharedEmpty)
^
/home/milian/projects/foo/src/main.cpp:12:46: error: previous declaration of
‘QSharedDataPointerFoo::Private* s_sharedEmpty()’ [-fpermissive]
friend QSharedDataPointerFoo::Private* ::s_sharedEmpty();
Thanks for the useful input Thiago, much appreciated!
--
Milian Wolff
m...@milianw.de
http://milianw.de
signature.asc
Description: This is a digitally signed message part.
Re: Initialization of QSharedDataPointer's for empty objects (i.e. Foo::Foo(void))
On terça-feira, 30 de abril de 2013 01.01.36, Milian Wolff wrote:
Interestingly I can't find a case cases don't seem to be using
QSharedDataPointer, or am I missing something?
Right. None of them use QSharedDataPointer.
You can find uses of that in these changes I uploaded during the weekend:
https://codereview.qt-project.org/54942
https://codereview.qt-project.org/54943
In particular, pay attention to patch 1 in those and in 54941, before
QLocalePrivate became POD. That's probably what you're looking for.
And is the following not OK due to random static initialization order in
C++03? http://paste.kde.org/734738/
It's *not* ok for Qt. I don't remember what KDE rules are now on dynamic
initialisation. Qt requires all static variables to be POD, or else you need
to use Q_GLOBAL_STATIC. So the Foo::Private::s_sharedEmpty type is not
permitted in Qt.
Otherwise, your code is fine.
But in C++11 it should be fine, no?
No, it doesn't make any difference.
The alternative using
{K,Q}_GLOBAL_STATIC is imo much uglier, see: http://paste.kde.org/734750/
Beauty is in the eye of the beholder. I find that prettier.
I could esp. not make it work with a private Private class as used in the
previous code snippet. I.e. this: http://paste.kde.org/734756/ triggers the
following compile error:
/home/milian/projects/foo/src/main.cpp: In function
‘QSharedDataPointerFoo::Private* s_sharedEmpty()’:
/home/milian/projects/foo/src/main.cpp:21:56: error:
‘QSharedDataPointerFoo::Private* s_sharedEmpty()’ was declared ‘extern’
and later ‘static’ [-fpermissive]
Q_GLOBAL_STATIC(QSharedDataPointerFoo::Private, s_sharedEmpty)
^
/home/milian/projects/foo/src/main.cpp:12:46: error: previous declaration of
‘QSharedDataPointerFoo::Private* s_sharedEmpty()’ [-fpermissive] friend
QSharedDataPointerFoo::Private* ::s_sharedEmpty();
Remove the friendship, move the Private class out of the private: section.
To befriend a static function, the function needs to be forward-declared first.
But you can't forward-declare the function if it returns a nested structure.
--
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
Software Architect - Intel Open Source Technology Center
PGP/GPG: 0x6EF45358; fingerprint:
E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358
signature.asc
Description: This is a digitally signed message part.
