Re: Re: Re: playground-libs/libkvkontakte has moved to kdereview
A Dissabte, 3 de setembre de 2011, Alexander Potashev vàreu escriure: 2011/9/2 Albert Astals Cid aa...@kde.org: Personally i prefer things not to be released from kdereview. Gilles Caulier (digiKam and KIPI-Plugins maintainer) told me that the tarball for KIPI-Plugins 2.1.0 will probably be created tomorrow. Therefore I hope to release libkvkontakte also tomorrow. Should now this library be moved to extragear/libs or playground/libs? Seems everyone was happy with review so move it to extragear? Anyway i prefer things not to be released from playground either (yes i know lots of people do it, that's why it is my personal opinion) so if you can not get it to be moved or prefer to stay in kdereview a bit more please do not let my personal opinion block you from doing a release. Albert -- Alexander Potashev
Re: Re: Re: playground-libs/libkvkontakte has moved to kdereview
2011/9/4 Albert Astals Cid aa...@kde.org: Seems everyone was happy with review so move it to extragear? Anyway i prefer things not to be released from playground either (yes i know lots of people do it, that's why it is my personal opinion) so if you can not get it to be moved or prefer to stay in kdereview a bit more please do not let my personal opinion block you from doing a release. I've already decided that libkvkontakte will be released (i.e. tarball-ed) today as part of digiKam SC, because otherwise we either need to remove the VKontakte export plugin from kipi-plugins or release libkvkontakte later in a separate tarball making life harder for packagers. -- Alexander Potashev
Re: Re: playground-libs/libkvkontakte has moved to kdereview
2011/9/2 Albert Astals Cid aa...@kde.org: Personally i prefer things not to be released from kdereview. Gilles Caulier (digiKam and KIPI-Plugins maintainer) told me that the tarball for KIPI-Plugins 2.1.0 will probably be created tomorrow. Therefore I hope to release libkvkontakte also tomorrow. Should now this library be moved to extragear/libs or playground/libs? -- Alexander Potashev
Re: Re: playground-libs/libkvkontakte has moved to kdereview
A Divendres, 2 de setembre de 2011, Alexander Potashev vàreu escriure: 2011/8/29 Alexander Potashev aspotas...@gmail.com: 2011/8/26 Alexander Potashev aspotas...@gmail.com: This argument wins. Just give me a couple of days to get to changing all classes to data only in private class strategy. Done. Is everything OK now to move into extragear/libs? Or I can release libkvkontakte from kdereview? Personally i prefer things not to be released from kdereview. Albert -- Alexander Potashev
Re: Re: playground-libs/libkvkontakte has moved to kdereview
A Divendres, 26 d'agost de 2011, Alexander Potashev vàreu escriure: 2011/8/26 David Faure fa...@kde.org: A last argument, code is easier to maintain if it follows the same rules everywhere. So in all public library code (Qt, kdelibs, and all other public libs) we should do things the same way, to ease maintainance. This argument wins. Just give me a couple of days to get to changing all classes to data only in private class strategy. Don't know what to do with classes like UserInfo, NoteInfo, MessageInfo, etc. What's special in those classes? Albert -- Alexander Potashev
Re: playground-libs/libkvkontakte has moved to kdereview
On Thursday 25 August 2011 19:23:10 Alexander Potashev wrote: 2011/8/25 Albert Astals Cid aa...@kde.org: The point is that usually you do not know what the library will end up doing and by using d-pointers everywhere you make it easier for yourself to maintain binary compatibility in the future. But in the case that most classes won't grow in the future by using my strategy (keeping d-ptr NULL when possible) we cut down the number of memory allocations, and also simplify the existing code that doesn't work with the private class' data. But you can't remove member variables, or change their types (if the size differs), and you have to be extra careful to not write inline methods that use the member variables because then you're screwed if you want to change anything. It also means you need to #include a lot more stuff into your header file. Cutting down on the number of memory allocations is important in very low- level often-allocated classes e.g. (QChar), but for anything else I doubt it makes much difference. A last argument, code is easier to maintain if it follows the same rules everywhere. So in all public library code (Qt, kdelibs, and all other public libs) we should do things the same way, to ease maintainance. -- David Faure, fa...@kde.org, http://www.davidfaure.fr Sponsored by Nokia to work on KDE, incl. Konqueror (http://www.konqueror.org).
Re: playground-libs/libkvkontakte has moved to kdereview
On Thursday 25 August 2011 19.23.10 Alexander Potashev wrote: 2011/8/25 Albert Astals Cid aa...@kde.org: The point is that usually you do not know what the library will end up doing and by using d-pointers everywhere you make it easier for yourself to maintain binary compatibility in the future. But in the case that most classes won't grow in the future by using mystrategy (keeping d-ptr NULL when possible) we cut down the number ofmemory allocations, and also simplify the existing code that doesn'twork with the private class' data. So, I'm going to keep following thecurrent strategy. There are several cornercases known where the d-pointer and a general policy of keeping the installed header file clean is a solution. One example is when you can get problems when dynamic_cast fails to work accross libraries (i.e. in code that uses your library) if there is not enough code in the cpp file. There are several other problems, like David also wrote, where you will end up accidentially changing ABI. So if you choose to not use this approach to solving ABI consistency, may I suggest you write some tests that export the ABI at release and compare it for every subsequent release so you can avoid accidental breakages. I've been doing C++ for years, and I still accidentally make breakages when I change something I didn't expect to cause issues, its much easier to break than you might expect ;) Cheers! -- Thomas Zander
Re: playground-libs/libkvkontakte has moved to kdereview
2011/8/9 Alexander Potashev aspotas...@gmail.com: playground-libs/libkvkontakte moved to kdereview today. The next target for this project is extragear/libs. libkvkontakte has been in kdereview for more than two weeks already. Is it OK to move it into extragear-libs now? -- Alexander Potashev
Re: Re: playground-libs/libkvkontakte has moved to kdereview
A Dijous, 25 d'agost de 2011, Alexander Potashev vàreu escriure: 2011/8/9 Alexander Potashev aspotas...@gmail.com: playground-libs/libkvkontakte moved to kdereview today. The next target for this project is extragear/libs. libkvkontakte has been in kdereview for more than two weeks already. Is it OK to move it into extragear-libs now? I thought you were going to get rid of the private members and use a d-pointer instead? Albert -- Alexander Potashev
Re: Re: playground-libs/libkvkontakte has moved to kdereview
2011/8/25 Albert Astals Cid aa...@kde.org: I thought you were going to get rid of the private members and use a d-pointer instead? What is the point of this? I think it will be OK to keep all class members in the main (public) classes and use d-ptr only in case of real necessity. -- Alexander Potashev
Re: Re: Re: playground-libs/libkvkontakte has moved to kdereview
A Dijous, 25 d'agost de 2011, Alexander Potashev vàreu escriure: 2011/8/25 Albert Astals Cid aa...@kde.org: I thought you were going to get rid of the private members and use a d-pointer instead? What is the point of this? I think it will be OK to keep all class members in the main (public) classes and use d-ptr only in case of real necessity. The point is that usually you do not know what the library will end up doing and by using d-pointers everywhere you make it easier for yourself to maintain binary compatibility in the future. But this is of course something you as maintainer of the library have to decide if it is worth or not. Albert -- Alexander Potashev
Re: Re: Re: playground-libs/libkvkontakte has moved to kdereview
2011/8/25 Albert Astals Cid aa...@kde.org: The point is that usually you do not know what the library will end up doing and by using d-pointers everywhere you make it easier for yourself to maintain binary compatibility in the future. But in the case that most classes won't grow in the future by using my strategy (keeping d-ptr NULL when possible) we cut down the number of memory allocations, and also simplify the existing code that doesn't work with the private class' data. So, I'm going to keep following the current strategy. -- Alexander Potashev
Re: playground-libs/libkvkontakte has moved to kdereview
2011/8/17 Thomas Zander zan...@kde.org: Most C++ libraries use this, but I suggest to take a look at kdelibs for inspiration. Implementation of p-pointers not always the same in the whole kdelibs. I preferred not to use neither Q_DECLARE_PRIVATE nor inheritance of *Private classes (as described at http://techbase.kde.org/Policies/Library_Code_Policy/Shared_D-Pointer_Example). You can check what I've done: http://commits.kde.org/libkvkontakte/094b12d2e2f2b6c73e3721607a909079722f93a2 I still need to polish the code before freezing the ABI, for example I'm planning to remove a couple of classes. -- Alexander Potashev
Re: playground-libs/libkvkontakte has moved to kdereview
On Wednesday 17 August 2011 09.24.31 Alexander Potashev wrote: If I'll add just a forward declaration like class NoteInfoPrivate; and a NoteInfoPrivate *p; into the NoteInfo class, will it be OK? I guess you mean using a d-pointer, yes, that's the suggested way of dealing with this kind of issue. So, the NoteInfoPrivate class may not have any declaration (except for the forward declaration) until it will be necessary, right? The concept of a d-pointer is a bit weird at first, for sure ;) You may want to take a peek at some examples to see how its done and steal some patterns of usage. Most C++ libraries use this, but I suggest to take a look at kdelibs for inspiration. And to answer your question; yes, its Ok to just have a forward declaration in the header file.
Re: Re: playground-libs/libkvkontakte has moved to kdereview
2011/8/15 Albert Astals Cid aa...@kde.org: A Dilluns, 15 d'agost de 2011, Alexander Potashev vàreu escriure: How about adding a QMapQString, QVariant m_ext; to *Info classes, so that I can store additional variables there? Most (but not all) *Job classes are unlikely to be expanded later, because they perform very simple operations. Why not simply use a d-pointer like it is explained in techbase? I thought about QMapQString, QVariant as of an easier solution since you won't need to declare the NoteInfoPrivate class. If I'll add just a forward declaration like class NoteInfoPrivate; and a NoteInfoPrivate *p; into the NoteInfo class, will it be OK? I guess you mean using a d-pointer, yes, that's the suggested way of dealing with this kind of issue. So, the NoteInfoPrivate class may not have any declaration (except for the forward declaration) until it will be necessary, right? -- Alexander Potashev
Re: Re: playground-libs/libkvkontakte has moved to kdereview
2011/8/17 Alexander Potashev aspotas...@gmail.com: So, the NoteInfoPrivate class may not have any declaration (except for the forward declaration) until it will be necessary, right? There is Q_DECLARE_PRIVATE macro, interesting... Should I use it instead? -- Alexander Potashev
Re: playground-libs/libkvkontakte has moved to kdereview
2011/8/15 Albert Astals Cid aa...@kde.org: You do not use d pointers in any of your classes thus maintaining Binary Compatibility is going to be almost impossible if you need to expand them. How about adding a QMapQString, QVariant m_ext; to *Info classes, so that I can store additional variables there? Most (but not all) *Job classes are unlikely to be expanded later, because they perform very simple operations. If I'll add just a forward declaration like class NoteInfoPrivate; and a NoteInfoPrivate *p; into the NoteInfo class, will it be OK? I'd also like if you used KCatalogLoader to load your translation catalog. Done. -- Alexander Potashev
Re: Re: playground-libs/libkvkontakte has moved to kdereview
A Dilluns, 15 d'agost de 2011, Alexander Potashev vàreu escriure: 2011/8/15 Albert Astals Cid aa...@kde.org: You do not use d pointers in any of your classes thus maintaining Binary Compatibility is going to be almost impossible if you need to expand them. How about adding a QMapQString, QVariant m_ext; to *Info classes, so that I can store additional variables there? Most (but not all) *Job classes are unlikely to be expanded later, because they perform very simple operations. Why not simply use a d-pointer like it is explained in techbase? If I'll add just a forward declaration like class NoteInfoPrivate; and a NoteInfoPrivate *p; into the NoteInfo class, will it be OK? I guess you mean using a d-pointer, yes, that's the suggested way of dealing with this kind of issue. I'd also like if you used KCatalogLoader to load your translation catalog. Done. Great. Albert -- Alexander Potashev
Re: playground-libs/libkvkontakte has moved to kdereview
On Tuesday 09 August 2011 12:07:49 Alexander Potashev wrote:
Hi,
playground-libs/libkvkontakte moved to kdereview today. The next
target for this project is extragear/libs. This project is a
library for interaction with the most popular social network in
Russia VKontakte.ru (also available at vk.com) using its public
API documented at
http://vkontakte.ru/pages.php?o=-1p=Документация (only in
Russian). libkvkontakte is already being used by
playground-pim/akonadi-vkontakte and the KIPI export plugin for
VKontakte which will be hopefully released with digiKam SC 2.1.0 in
September.
Some parts of code in libkvkontakte are based on
playground-pim/akonadi-facebook written by Thomas McGuire and
others.
Please, review.
Thanks.
It doesn't compile because asserts reference undeclared variables.
Attached patch shows where the errors are.
Christoph Feck (kdepepo)
KDE Quality Team
diff --git a/libkvkontakte/allnoteslistjob.cpp b/libkvkontakte/allnoteslistjob.cpp
index e095e94..59b604a 100644
--- a/libkvkontakte/allnoteslistjob.cpp
+++ b/libkvkontakte/allnoteslistjob.cpp
@@ -32,7 +32,7 @@ AllNotesListJob::AllNotesListJob(const QString accessToken, int uid)
void AllNotesListJob::startNewJob(int offset, int count)
{
-Q_ASSERT(out == 0 || out == 1);
+//Q_ASSERT(out == 0 || out == 1);
NotesListJob *job = new NotesListJob(m_accessToken, m_uid, offset, count);
connect(job, SIGNAL(result(KJob*)), this, SLOT(jobFinished(KJob*)));
@@ -67,7 +67,7 @@ void AllNotesListJob::jobFinished(KJob *kjob)
}
else {
// TODO: some new notes might have been added, what should we do then?
-Q_ASSERT(m_totalCount == listJob-totalCount());
+//Q_ASSERT(m_totalCount == listJob-totalCount());
}
// All jobs have finished
diff --git a/libkvkontakte/authenticationdialog.cpp b/libkvkontakte/authenticationdialog.cpp
index d7da524..9c95ef1 100644
--- a/libkvkontakte/authenticationdialog.cpp
+++ b/libkvkontakte/authenticationdialog.cpp
@@ -75,7 +75,7 @@ void AuthenticationDialog::setPermissions(const QStringList permissions)
void AuthenticationDialog::start()
{
-Q_ASSERT(!mAppId.isEmpty());
+Q_ASSERT(!m_appId.isEmpty());
// display= {page, popup, touch, wap}
const QString url = QString(http://api.vkontakte.ru/oauth/authorize?;
Re: playground-libs/libkvkontakte has moved to kdereview
2011/8/9 Christoph Feck christ...@maxiom.de: It doesn't compile because asserts reference undeclared variables. Attached patch shows where the errors are. I've applied your patch to authenticationdialog.cpp and reworked error reporting in allnoteslistjob.cpp and also in allmessageslistjob.cpp. Thanks! -- Alexander Potashev
