Finish removing dependance to KDElibs4Support
ClosedPublic

Authored by kleag on Aug 1 2017, 6:31 PM.

Details

Summary

Mostly simple changes like suppressing includes but also a change about socket classes

Diff Detail

Repository
R413 KsirK
Branch
nokdelibs4support (branched from frameworks)
Lint
No Linters Available
Unit
No Unit Test Coverage
kleag created this revision.Aug 1 2017, 6:31 PM
Restricted Application added a reviewer: KDE Games. · View Herald TranscriptAug 1 2017, 6:31 PM
Restricted Application added a subscriber: KDE Games. · View Herald Transcript
nhirsl edited edge metadata.Aug 1 2017, 9:17 PM

At this point, my first attempt to remove kdelibs4support was almost the same. However, I haven't submitted the change. There are two main reasons for this:

  1. I haven't tested xmpp part of the game, even everything else worked good
  2. Irislib and jabber part are outdated and there are newer versions available

To cover first item I was going to start new email thread (still saved in the draft), but since we have the review, I'll discuss it here.

Testing xmpp part
Is there anything special needed for setting up xmpp game?
For this purpose I asked our sysadmins and promptly got two accounts on kdetalk (thanks to Ben!):

  1. ksirk-dev1@kdetalk.net - password 'f59ArDehbEjgvhit'
  2. ksirk-dev2@kdetalk.net - password 'C9cbf3iMxSR8jqKn'

However I couldn't setup a game. Unfortunately, didn't have time to dive deeper and find root cause.

New iris and jabber library
In the meantime, I pulled latest iris lib and with a few minor changes mostly in cmake files I compiled it and integrated into ksirk. Changes in Jabber part (from Kopete) and the rest of ksirk were also aligned and minimal. Cutestuff also changed location within the iris lib which makes diff almost unusable.
I do have all these changes in the local branch, but not tested. In this case I can verify that connecting to kdetalk.net succeeds, but the first next thing - getting roster list fails. Again (unfortunately), not sure when I can continue investigation in this direction.

Now the question is how to proceed?
Gael, have you tested xmpp part of the game?

If changes in this review work, I would vote for pushing them and than later upgrading iris and jabber (if at all). But I'm not sure what's the best option if we can't verify that iris and jabber changes are OK.

kleag added a comment.Aug 2 2017, 7:22 AM

I must admit that I did not test the xmpp part… And in fact it does not work. There is an error in connecting a socket error signal. That should at least be corrected.

But I just tried too a Jabber based game on the ksirk currently available on KDE Neon and it does not work either. But this time the console output gives useful information.

I propose to correct the evident bug to have a situation in par with the current released version and then to push these changes before trying to make the Jabber work again with the new versions you worked on. In this case, should we remove the Jabber option from the GUI while it is not working ?

P.S.: to answer your question, there is nothing special necessary to play over Jabber except a working account
P.P.S.: the Jabber stuff was an interesting experiment but I don't know if anybody ever used it for real games… So, should we do large efforts to maintain it ?

kleag updated this revision to Diff 17564.Aug 2 2017, 1:14 PM
  • Fix skin editor icon
kleag added a comment.Aug 2 2017, 1:15 PM

I by error added a new commit to this review request instead of creating a new one :-(

nhirsl added a comment.Aug 3 2017, 7:46 AM
In D7044#131173, @kleag wrote:

I must admit that I did not test the xmpp part… And in fact it does not work. There is an error in connecting a socket error signal. That should at least be corrected.

But I just tried too a Jabber based game on the ksirk currently available on KDE Neon and it does not work either. But this time the console output gives useful information.

I propose to correct the evident bug to have a situation in par with the current released version and then to push these changes before trying to make the Jabber work again with the new versions you worked on. In this case, should we remove the Jabber option from the GUI while it is not working ?

I agree. It would be ideal to have the same behavior after removal of kdelibs4support. That would make the upgrade to newer iris and jabber libs easier.
We can remove Jabber option from GUI until we fix Jabber gameplay.

P.S.: to answer your question, there is nothing special necessary to play over Jabber except a working account
P.P.S.: the Jabber stuff was an interesting experiment but I don't know if anybody ever used it for real games… So, should we do large efforts to maintain it ?

Well, that's good question. I can't say for sure for how long, but Jabber part seems to be broken for a while. There was no single bug report for Jabber game, either.
Now, what effort should we invest here?
I wanted to upgrade to newer libs which at the end:

  1. Removes need for kdelibs4support
  2. Fixes a bunch of compile time warnings and speeds up compile time
  3. Aligns with upstream

Unfortunately, solely this upgrade didn't fix KsirK integration - it is still not possible to setup a game.

So, maybe we shouldn't invest that much time now, what do you think?

The plan could be:

  1. Remove kdelibs4support as in this review
  2. Fix and align behavior with current master
  3. Remove Jabber from GUI
  4. Upgrade iris and kopete parts
  5. Try to fix gameplay

If we can't do no.5 in a reasonable time, Jabber will be disabled (non visible from the GUI) until again someone (e.g. new group of students) decide to experiment with XMPP gameplay.

Does this sound reasonable?

kleag updated this revision to Diff 17693.Aug 4 2017, 7:24 AM
  • Revert to QTcpSocket
  • Hides the button starting new Jabber game

Implements first steps of plan by @nhirsl. Next step is to update iris and kopete libs.

nhirsl accepted this revision.Aug 6 2017, 4:25 PM

Once this is completed, I'll proceed with upgrading iris and kopete libs.

This revision is now accepted and ready to land.Aug 6 2017, 4:25 PM
kleag closed this revision.Aug 7 2017, 6:52 AM