Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make chat ignore permanent #2234

Merged
merged 5 commits into from Sep 29, 2019
Merged

Make chat ignore permanent #2234

merged 5 commits into from Sep 29, 2019

Conversation

@Dune-jr
Copy link
Member

Dune-jr commented Sep 14, 2019

Reimplements #2043 (but without the useless virtual inheritance OOP mindfuck):

Note: this slightly conflicts with #2042 (since it modifies the mute feature)

Behaviour

This feature makes use of the existing friends system to implement a permanent chat ignore. It adds two console commands add_ignore and remove_ignore (feel free to suggest different wording...)

  • Muting or unmuting a player, from the console or the menus, is always permanent (and player+clan based)

  • An information message is displayed when

    • a previously muted player join the server
    • the player joins a server with previously muted players

%s is muted by you sounds like broken English, maybe @oy will have a better suggestion :)
image

Implementation

As mentioned above, this feature makes use of the IFriends and CFriends classes.

In order to register two IFriends interfaces, I ran into the "diamond problem" where a class inherits of two classes which themselves inherit from the same baseclass. This is solved using virtual inheritence:

class IFriends: public IInterface
class IGoodFriends: public virtual IFriends
class IBadFriends: public virtual IFriends
class CFriends : public virtual IFriends
class CGoodFriends: public CFriends, public IGoodFriends
class CBadFriends: public CFriends, public IBadFriends

Multiple inheritance is generally frowned upon, but it seems like it tends to be given a pass for interfaces.

Due to this, there can be some nasty runtime errors if you use a static_cast instead of a reinterpret_cast (you get a cryptic "virtual baseclass botch" error) in gameclient.h.

It seemed somewhat clever and avoids code redundancy but I'm as much of an OOP nerd as my neighbor's cat is a leopard and I won't mind if I have to scrap that and find another way, given that it is somewhat obscure and possibly unfitting for the Teeworlds basecode.

The class diagram now looks like this:

image

It still works fine, even after quitting the game:
screenshot_2019-09-14_20-25-42

I had to rename a lot of Friends stuff to Contacts, so the diff file is a bit unreadable. Probably should have done this rename in a separate commit :(
I am hesitating to rename the three friends.h files to contacts.h, might be more fitting as well.

@fokkonaut

This comment has been minimized.

Copy link
Contributor

fokkonaut commented Sep 20, 2019

Can we have a command like friends_ignore_clan for this? So that friends and muted players is only based on name? We had this in DDNet, and it was pretty good, because people change clans frequently, for fun or if they are in multiple clans.

@Dune-jr

This comment has been minimized.

Copy link
Member Author

Dune-jr commented Sep 20, 2019

@fokkonaut I agree with this sentiment, but believe it should be done out of score of this PR, and also affect friendship.

For example, if I add as a friend "AMN", I believe it should mark "[5%] AMN" as my friend. However, if my add "[5%] AMN", it should probably not mark "[Qi] AMN" as a friend. That is my take on this.

#include <engine/keys.h>
#include <engine/serverbrowser.h>
#include <engine/storage.h>
#include <engine/textrender.h>
#include <engine/shared/config.h>
#include <engine/client/friends.h>

This comment has been minimized.

Copy link
@oy

oy Sep 24, 2019

Member

Hm, could you move the enums to the interface? It shouldn't include sth from engine/client/ but work with the interfaces instead.

This comment has been minimized.

Copy link
@Dune-jr

Dune-jr Sep 24, 2019

Author Member

Yes, that makes sense

@oy

This comment has been minimized.

Copy link
Member

oy commented Sep 24, 2019

Yeah, friends.* could be renamed to contacts.*

@Dune-jr

This comment has been minimized.

Copy link
Member Author

Dune-jr commented Sep 27, 2019

Is it alright to leave the enums in the global namespace then?

@oy

This comment has been minimized.

Copy link
Member

oy commented Sep 28, 2019

@Dune-jr better not put it in the global namespace.
Maybe add it to to CContactInfo?

Edit:
You have to update the cmake file list to fix the failed checks.

@Dune-jr

This comment has been minimized.

Copy link
Member Author

Dune-jr commented Sep 28, 2019

@Dune-jr better not put it in the global namespace.
Maybe add it to to CContactInfo?

I can, I don't see a better way anyway

Edit:
You have to update the cmake file list to fix the failed checks.

I actually did a sed and self-congratulated for thinking about cmake for once. But it broke the alphabetical order so nope :(

@oy oy merged commit 6e48411 into teeworlds:master Sep 29, 2019
3 checks passed
3 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Dune-jr Dune-jr deleted the Dune-jr:feature-permamute-v2 branch Oct 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.