Bug 308763

Created attachment 74703 [details]
Allows to compile kdepimlibs

Could you update the patch to use QString::fromUtf8() instead?
Our source files should all be UTF-8 encoded and even if in this case all input is 7-bit ASCII only it would be better for consistency

Created attachment 74813 [details]
replaced QString::fromAscii with QString::fromUtf8

Martin, can you check/commit the changes for social feed code?

Bogdan, why is there the 

+add_definitions(-DMAKE_LIBKOAUTH_LIB)

in akonadi/socialutils/CMakeLists.txt? Is that the libkoauth in kde:scratch/mklapetek/libkoauth?

As for the rest - it moves the private data container back to the header file, which I was advised against, but if it's ok, then I'm happy with it.

MAKE_LIBKOAUTH_LIB is used in ./akonadi/socialutils/libakonadisocialutils_export.h in order to create the library akonadi-socialutils. You could move that definition in ./akonadi/socialutils/CMakeLists.txt and use:
ifdef(WIN32)
add_definitions(-DMAKE_LIBKOAUTH_LIB)
endif()

Argh, that's just lame copy-paste job. It should say "MAKE_AKONADI_SOCIALUTILS_LIB" in that _export.h file.

It should be possible to have the shared data class inside the cpp file, KABC::Addressee does that.
Maybe it needs the class forward declaration inside the outer class. Bodgan could you look at $BUILDIR/kdepimlibs/kabc/addressee.h/.cpp and check why it can have the SharedData class in the cpp instead of the header?

Martin, PostReply should probably have a d-pointer, we usually don't have public data in public classes

In $BUILDIR/kdepimlibs/akonadi/socialutils/socialfeeditem.h, class SocialFeedItem has a private data member

QSharedDataPointer<SocialFeedItemData> d;

VS requires to have the declaration of SocialFeedItemData class instead of a forward declaration.

(In reply to comment #9)
> In $BUILDIR/kdepimlibs/akonadi/socialutils/socialfeeditem.h, class
> SocialFeedItem has a private data member
> 
> QSharedDataPointer<SocialFeedItemData> d;
> 
> VS requires to have the declaration of SocialFeedItemData class instead of a
> forward declaration.

There must be a different solution as well, because KABC::Addressee has this in addressee.h

  private:
    class Private;
    QSharedDataPointer<Private> d;

And Private is defined in addresee.cpp

(In reply to comment #8)
> Martin, PostReply should probably have a d-pointer, we usually don't have
> public data in public classes

Well it's a simple container class, I wanted to avoid having 7 getters and 7 setters so I made it public. But sure, I can update.

(In reply to comment #11)
> (In reply to comment #8)
> > Martin, PostReply should probably have a d-pointer, we usually don't have
> > public data in public classes
> 
> Well it's a simple container class, I wanted to avoid having 7 getters and 7
> setters so I made it public. But sure, I can update.

we should probably discuss this on the mailing list. usually we have a policy of not doing that as it does not allow for any kind change.

Actually I'm thinking about getting rid of that class altogether and just use the SocialFeedItem directly - because the replies (comments on facebook) have half of the properties of SFI, so it doesn't really make sense to duplicate the same properties in another class. I'll investigate this tomorrow and post my findings.

So I tried using the SocialFeedItem class for the replies and thus getting rid of the PostReply container. However it won't compile if the private data class is not in the .h file (same goes for SocialFeed). Otherwise it works perfectly.

So I'd be in favor of moving it to .h file as well.

As I wrote in Comment 10, there must be a way to keep the Private, well, private.

KABC::Addressee compiles on Windows, doesn't it?

Sure, but now I'm talking about reusing the class in itself (ie. have   QList<SocialFeedItem> replies; in the private part), which I found no way to compile but with the private class in the .h file.

Digging a bit deeper I found the cause for both compilation failures (mine and Bogdan's) and also the difference between SFI and KABC::Addressee - the SocialFeedItem misses the &operator method.  Simply adding it compiles my code fine with the private class in the .cpp file.

Bogdan, can you try adding

    SocialFeedItem &operator=(const SocialFeedItem &other);

to socialfeeditem.h (under the dtor) and 

Akonadi::SocialFeedItem& Akonadi::SocialFeedItem::operator=(const Akonadi::SocialFeedItem& other)
{
  if (this == &other) return *this; //Protect against self-assignment
  d = other.d;
  return *this;
}

in the socialfeeditem.cpp and try compiling again on windows without any other changes? Should just work.

The proposed solution does not work for me. Actually, you suggest to explicitly define the copy assignment operator for SocialFeedItem class, but this one is implicitly defined by the compiler.

The idea is that if the assignment operator and copy constructors are defined in the .cpp file, they "see" the complete definition of the private.
When the compiler generates them it does not, which is why all those classes have an explicitly defined destructor as well

Basically one needs to make sure that the compiler does not generate any of those functions (default constructor, destructor, copy constructor, copy-assignment operator)

Regardless, I think the SocialFeedItem should have that copy-assignment operator implemented.

Kevin - can I commit that? Together with fix from comment #7.

I agree, it should have the copy constructor.
Maybe create a reviewboard request and add the kde-windows list so they can check if this solves the windows build problem as well

Created attachment 79049 [details]
fix kdepimlibs compilation on Windows (KDE 4.10.2)

unfortunately, this patch does not seem to have made it into KDE yet and it is a bit outdated by now

I recently stumbled upon the same compilation issues on Windows with MSVC 2010 and the root cause is not MSVC itself, but QT_NO_CAST_FROM_ASCII being set (plus one missing include) - the rest may have been fixed in the meanwhile.

Anyway, I propose a new patch for this (I tested windows compilation).
Compared to the previous one, there is a slight difference for strings like "a.url().toString().toLatin1()" - I assume that this was done on purpose ("a.url().toString()" would otherwise have been valid, too) in order to remove non-Latin1 characters. So I kept this behaviour - in this particular case, it translates into:

const QByteArray& lat1Url = a.url().toString().toLatin1();
QString::fromLatin1(lat1Url.constData(), lat1Url.size())

ok, I just saw, that kdepimlibs-4.10.0.diff includes some of these changes but with the problematic "a.label().toLatin1() -> a.label()" conversion and commenting out the tests directory which I also fixed.

Anyway, these changes should probably be upstreamed as every system with QT_NO_CAST_FROM_ASCII will be affected

actually, I think the code below is more correct for the latin1 correction:

const QByteArray lat1Url = a.url().toString().toLatin1();
QString::fromLatin1(lat1Url.constData(), lat1Url.size())

note the missing reference in the first line. If I'm not mistaken, otherwise the temporary object being created by toLatin1() may be deleted at the next sequence point (the end of this line) and the reference may become invalid.
I'll create an appropriate patch and commit it if there are no further comments...

Thank you for the bug report.

As this report hasn't seen any changes in 5 years or more, we ask if you can please confirm that the issue still persists.

If this bug is no longer persisting or relevant please change the status to resolved.

The KDE Windows Bugzilla product is no longer maintained.

If you have issues with some application on Windows, please report the issue at the bugtracker of the application.

If you have issues that still persist with Craft itself, please report them here:

https://invent.kde.org/packaging/craft/-/issues