add all friends to announcements #942

GrotheFAF · 2017-12-31T01:53:08Z

use new friendtracker (Wesmania) to signal friend-game events
stash signals on client start until client has settled, and push them on addChatter

coveralls · 2017-12-31T02:14:30Z

Coverage increased (+0.1%) to 27.642% when pulling 20c20ff on GrotheFAF:add-all-friends-to-announcement into f6eb7b2 on FAForever:develop.

Wesmania · 2017-12-31T13:53:52Z

src/client/_clientwindow.py

@@ -1062,6 +1045,11 @@ def _tabChanged(self, tab, curr, prev):
    def mainTabChanged(self, curr):
        self._tabChanged(self.mainTabs, curr, self._main_tab)
        self._main_tab = curr
+        # the tab change works after client has initialized
+        if not self.init_ready:
+            self.init_ready = self._main_tab == 1  # chatTab


I think it's best to just set it to True here, not everyone switches to chat tab at client start and it'll be confusing if announcements only show once you switch to the chat tab.

Oh right, you mean the popup+sound? I have that disabled ..forever, so forgot about those

Wesmania · 2017-12-31T13:54:36Z

src/client/gameannouncer.py

 from model.game import GameState
-
-from fa import maps
+import chat.friendtracker


from chat.friendtracker import build_friend_tracker should work too.

yes, yes it does.

Wesmania · 2017-12-31T13:57:59Z

src/client/gameannouncer.py

+        self._delayed_event_list = []
+
+    def _friend_event(self, player):
+        if self._client.init_ready:


We could maybe move the init_ready to this class, and set it to true once the delayed_friend_events gets called (and maybe just call it should_queue_events or something like that).

yes, yes we can. (but not that name)

Wesmania · 2017-12-31T13:59:56Z

src/client/gameannouncer.py

+            player = self._delayed_event_list.pop(0)
+            self._friend_announce(player)
+
+    def _friend_announce(self, player):


I think catching the 'event type' enum would make processing this much more readable. We can put processing each event type into a separate function, with some sanity checks to make sure things didn't change in the meantime.

_friend_announce covers changes in the meantime.
but I guess .. I have to do what you say anyway .. else no cookie ... so why argue.

I'm allergic to functions that don't fit on my 25x80 vga console :)

That's what I suspected. (cause all your lines before 80)

there is only one line over 80 (and that could be easy broken), and I do now typos and miss stuff
.. I'm adapting ;-)

coveralls · 2017-12-31T19:24:50Z

Coverage increased (+0.1%) to 27.644% when pulling 80c6046 on GrotheFAF:add-all-friends-to-announcement into f6eb7b2 on FAForever:develop.

coveralls · 2018-01-01T15:26:03Z

Coverage increased (+0.1%) to 27.644% when pulling 88daf3c on GrotheFAF:add-all-friends-to-announcement into f6eb7b2 on FAForever:develop.

coveralls · 2018-01-01T15:52:24Z

Coverage increased (+0.1%) to 27.644% when pulling da7bf6f on GrotheFAF:add-all-friends-to-announcement into 1263c12 on FAForever:develop.

coveralls · 2018-01-01T18:29:53Z

Coverage increased (+0.1%) to 27.644% when pulling e3a827c on GrotheFAF:add-all-friends-to-announcement into e34dd0b on FAForever:develop.

coveralls · 2018-01-02T20:32:49Z

Coverage increased (+0.1%) to 27.637% when pulling 5901c86 on GrotheFAF:add-all-friends-to-announcement into b1b600a on FAForever:develop.

coveralls · 2018-01-06T22:51:33Z

Coverage increased (+0.1%) to 27.643% when pulling e501e4b on GrotheFAF:add-all-friends-to-announcement into b1b600a on FAForever:develop.

coveralls · 2018-01-09T00:21:45Z

Coverage increased (+0.1%) to 27.618% when pulling 40e4279 on GrotheFAF:add-all-friends-to-announcement into b0f9e5d on FAForever:develop.

coveralls · 2018-01-09T22:03:24Z

Coverage increased (+0.1%) to 27.618% when pulling f5e9fe6 on GrotheFAF:add-all-friends-to-announcement into 99d772d on FAForever:develop.

coveralls · 2018-01-17T19:08:27Z

Coverage increased (+0.1%) to 27.611% when pulling 1918195 on GrotheFAF:add-all-friends-to-announcement into ef57cf4 on FAForever:develop.

coveralls · 2018-01-17T21:18:00Z

Coverage increased (+0.1%) to 27.611% when pulling 1cc4b62 on GrotheFAF:add-all-friends-to-announcement into 2eb640c on FAForever:develop.

coveralls · 2018-01-18T18:28:09Z

Coverage increased (+0.1%) to 27.609% when pulling ce06497 on GrotheFAF:add-all-friends-to-announcement into cd1b011 on FAForever:develop.

Wesmania · 2018-01-18T18:36:55Z

src/client/gameannouncer.py


-    def _announce_replay(self, game):
-        if not self._is_friend_host(game) or not self.announce_replays:
+    def _friend_announce(self, player, event):


I don't really like how we don't use the event type here at all - it means that the logic for figuring out what event happened to a player is in two places, here and in friend tracker. If we change something, we'll have to change code in two places. Can we instead use the event type, and only do some basic sanity checks (like if the game is open)?

We can do that ...

Wesmania · 2018-01-18T18:37:44Z

src/client/gameannouncer.py

+                return
+            activity = "is playing live"
+        else:  # GameSate.CLOSED
+            activity = "<font color='Red'>has left the building</font>"


This will confuse people. Do we even need an event for a friend leaving a game?

Well, that is the 'shouldn't happen' case, I like to have those cases signaled if it happens (makes a bigger splash than a log entry).
But I can live without it, cause I noticed you prefer coding without humor.

Humour is nice if it doesn't seep into user experience. You're not the one who will have to deal with confused users on the tech support forum.

But that's the way we'll probably hear that there's something unintended/wrong going on.
(and it's an Elvis reference, so hopefully someone will know it's not really faf context.)
If we skip the case, we'll never know, that might be fine too. If you want it gone, say so.

It's not very unusual for it to happen, since between the event firing and the queue being freed the game could've ended. Either way it's still better to log it rather than bother the user with something a dev wants to know. Since it will happen occasionally anyway, I'd rather see it gone.

the game ended case is handled on top. this is different, maybe you look at the updated code... very eventfull.

GrotheFAF · 2018-01-18T20:48:33Z

src/client/_clientwindow.py

@@ -1060,6 +1043,9 @@ def _tabChanged(self, tab, curr, prev):
    def mainTabChanged(self, curr):
        self._tabChanged(self.mainTabs, curr, self._main_tab)
        self._main_tab = curr
+        # the tab change works after client has kinda initialized
+        if self.game_announcer.delay_friend_events:
+            self.game_announcer.delayed_friend_events()



Sad, the client start announcements can still get lost, if you click Chat 'to early' while chat is still building up.
So either we find another 'chat is ready for lines'-signal place, or - the ugly way - add an extra delay in/before delayed_friend_events.

Let's not complicate things just yet. The ideal way really would be to separate chat from widgets - as in make a "chat model" with representations of channels, chatters, lines etc, and have widgets only show changes. We'd then initialize and hook up model stuff earlier and not worry about anything being lost. I did a bit of work towards this, but stalled a bit, see https://github.com/Wesmania/client/tree/chat-rewrite.

If it's not working reliably, that's not complicating things, that's just sad, and should be fixed if possible.
We don't know what percentage of users click on the Chat-Tab right away after client start.

It's not obvious to me which point in the code can be considered "chat widget fully loaded". You're welcome to hunt one down.

well, I tried that for some time already, and didn't find any better place ... so far.
I might try some more ...

I added a 2nd holdof in _chadwidget - on_join, now the announcement is delivered, but in grey (insteat of friend color), so something is still not ready. (the announced player has a name, but is not yet in chatter, chat is still at the 'one man show' level)

well, found a solution over addChatter in channel.py.
There might be room for improvement in elegance, suggestions are welcome.

coveralls · 2018-01-19T06:14:43Z

Coverage increased (+3.4%) to 27.523% when pulling 83d31e3 on GrotheFAF:add-all-friends-to-announcement into 2157502 on FAForever:develop.

coveralls · 2018-01-19T22:12:01Z

Coverage increased (+3.3%) to 27.749% when pulling b45480a on GrotheFAF:add-all-friends-to-announcement into 1afb1f0 on FAForever:develop.

Wesmania · 2018-01-20T13:14:48Z

src/client/gameannouncer.py

            return
-        self._announce(game, "hosting")
+        i = 0
+        for event in self._delayed_event_list:


I don't really like this. We make delaying player messages dependent on the chat widget loading and on IRC info about chatters. This will interfere with everything else that uses this announcer, like notification popups. I'd rather see this reverted to the original approach.

What is the exact reason why the chat widget misses some events? Missing chat tabs might be one. If we additionally queue these messages in _chatwidget and send them to crucial channels once they load, this should fix our problem.

Well, that's what the delay is for, waiting for chatwidget and chatter to be ready/there.
Once that is done, that part doesn't play any role anymore.
The chatters are not there. Once a chatter is added his message can be put out.
And queuing the message somewhere else, doesn't help, because 'once they load' is the unknown. Without the chatter added one will end up with a message without a sender/chatter. (I tried that with my 2nd source hold back, didn't work.)

For all I can tell, the message ends up in a print_raw method in channel.py. It should be able to handle a sender that's not on the chatter list, if it doesn't, then it should be fixed.

I still have an issue with this code having to do special exceptions just for chat. The chat widget as a whole should listen to these announcements and print them to channels - and if a channel is not up yet, it should queue them by itself and send them all once it's up.

Yes, it can handle a message form chatter that isn't there, but the message will get the noplayer/offline color (my 2nd source hold back from above). Is that what we find acceptable. I don't. Of course, we could 'fake' the color, as we know the source of the messages.
And the gameannouncer doesn't know anything about source of the delayed_friend_events call, it would do that for replays, so no "special exception just for chat".
But - as I'm sure you insist - let's Qt gameannouncer and move the whole delay line to chat...
(but there is no 'once it's up' (maybe you can find one, I couldn't) there is still only 'chatter is there'.)

Chat widget has an 'add_channel' method.

Doesn't do it. I checked (my 2nd source hold back from above).

It does, but check out line 350 of _chatwidget.py. The tab is added before we hook up the localBroadcast signal. Swapping these would be the quickest fix I imagine, other than the small refactor I described above.

What would that change? Would it make the chatter add earlier or faster?
Did you test it? Please do.

You mentioned before that emitting notifications when the channel was added wouldn't lead to them being displayed. I'm just suggesting this might be a reason.

Anyway, the issue of the link being displayed in a wrong color in the chat is rather a fault of the display code - the print_raw function assumed so far that it will only print messages from chatters, which makes no sense when it comes from an announcement.
I suggest we split it into two functions - actual 'print_raw_message' that takes all formatting info like a color and doesn't care about message source, and the old 'print_raw' that gets color info from the chatter as it used to and uses 'print_raw_message' to print it. We can then use the 'print_raw_message' function to print announcements.

Wesmania · 2018-01-20T13:16:00Z

src/client/gameannouncer.py

+              '(on <font color="GoldenRod">{}</font> {}{})'
+        msg = fmt.format(activity, modname, url_color, url, game.title,
+                         game.mapdisplayname, player_info, time_info)
+        self._client.forwardLocalBroadcast(player.login, msg)


For all I can tell, client's localBroadcast signal is only used by the chat widget. If we give this class its own signal that it emits here and pass the class to the chat widget in the constructor, we can get rid of an unnecessary indirection layer.

There is an additional use of localBroadcast emit("Scores", message["text"]), in handle_notice in _clientwindow. (but I can't remember ever seen that message, is it in use, does it exist?)
That the GameAnnouncer can signal itself is a nice idea, but it is not a Qt object, so how signaling?

We can add signals to GameAnnouncer exactly like to other objects, we just need to subclass it from QObject.

use new friendtracker (Wesmania) to signal friend-game events add player-info (and runtime-info on client start) stash signals on client start until chatter is added

coveralls · 2018-01-20T19:11:29Z

Coverage increased (+3.3%) to 26.494% when pulling eb2b327 on GrotheFAF:add-all-friends-to-announcement into b90bffa on FAForever:develop.

Wesmania reviewed Dec 31, 2017

View reviewed changes

GrotheFAF force-pushed the add-all-friends-to-announcement branch from 20c20ff to 80c6046 Compare December 31, 2017 19:01

GrotheFAF force-pushed the add-all-friends-to-announcement branch from 80c6046 to 88daf3c Compare January 1, 2018 14:37

GrotheFAF force-pushed the add-all-friends-to-announcement branch from 88daf3c to da7bf6f Compare January 1, 2018 15:38

GrotheFAF force-pushed the add-all-friends-to-announcement branch from da7bf6f to e3a827c Compare January 1, 2018 18:28

GrotheFAF force-pushed the add-all-friends-to-announcement branch from e3a827c to 5901c86 Compare January 2, 2018 20:30

GrotheFAF force-pushed the add-all-friends-to-announcement branch from 5901c86 to e501e4b Compare January 6, 2018 22:49

GrotheFAF force-pushed the add-all-friends-to-announcement branch from e501e4b to 40e4279 Compare January 9, 2018 00:19

GrotheFAF force-pushed the add-all-friends-to-announcement branch from 40e4279 to f5e9fe6 Compare January 9, 2018 22:00

GrotheFAF force-pushed the add-all-friends-to-announcement branch from f5e9fe6 to 1918195 Compare January 17, 2018 19:05

Wesmania added this to the 0.17.0 backlog milestone Jan 17, 2018

GrotheFAF force-pushed the add-all-friends-to-announcement branch from 1918195 to 1cc4b62 Compare January 17, 2018 21:15

GrotheFAF force-pushed the add-all-friends-to-announcement branch from 1cc4b62 to ce06497 Compare January 18, 2018 18:25

Wesmania reviewed Jan 18, 2018

View reviewed changes

GrotheFAF commented Jan 18, 2018

View reviewed changes

GrotheFAF force-pushed the add-all-friends-to-announcement branch 2 times, most recently from f31535d to 83d31e3 Compare January 19, 2018 06:12

GrotheFAF force-pushed the add-all-friends-to-announcement branch from 83d31e3 to b45480a Compare January 19, 2018 21:47

Wesmania reviewed Jan 20, 2018

View reviewed changes

add all friends to announcements

eb2b327

use new friendtracker (Wesmania) to signal friend-game events add player-info (and runtime-info on client start) stash signals on client start until chatter is added

GrotheFAF force-pushed the add-all-friends-to-announcement branch from b45480a to eb2b327 Compare January 20, 2018 19:09

add all friends to announcements #942

Are you sure you want to change the base?

add all friends to announcements #942

Conversation

GrotheFAF commented Dec 31, 2017 • edited Loading

coveralls commented Dec 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 31, 2017

coveralls commented Jan 1, 2018

coveralls commented Jan 1, 2018

coveralls commented Jan 1, 2018

coveralls commented Jan 2, 2018

coveralls commented Jan 6, 2018

coveralls commented Jan 9, 2018

coveralls commented Jan 9, 2018

coveralls commented Jan 17, 2018

coveralls commented Jan 17, 2018

coveralls commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrotheFAF Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 19, 2018

coveralls commented Jan 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 20, 2018

GrotheFAF commented Dec 31, 2017 •

edited

Loading

GrotheFAF Jan 19, 2018 •

edited

Loading