Message filter - [merged] #148

Closed jdonkervliet closed 4 years ago

In GitLab by @swabbur on May 20, 2020, 18:27

Merges feature/message-filter -> development

Adds a message filter that allows published messages to be filtered before being forwarded to subscribers. Also fixes the graphical glitch caused by BlockBreakAnimationMessages being broadcasted to all players.

In GitLab by @swabbur on May 20, 2020, 18:30

added 1 commit

a6fe0bdb - Fix line length issue

Compare with previous version

In GitLab by @swabbur on May 20, 2020, 18:40

added 1 commit

946c8c45 - Completely fix BlockBreakAnimation graphical glitch

Compare with previous version

In GitLab by @swabbur on May 20, 2020, 18:41

changed the description

In GitLab by @swabbur on May 20, 2020, 20:12

added 1 commit

355232e1 - Fix unused import

Compare with previous version

In GitLab by @larsdetombe on May 20, 2020, 20:31

Commented on src/test/java/net/glowstone/messaging/filters/PlayerFilterTest.java line 31

Shouldn't these tests have javadoc? So far we have done this in all the test classes I have seen, so it might be better to stay consistent and add javadoc to these tests.

In GitLab by @swabbur on May 20, 2020, 20:32

Commented on src/test/java/net/glowstone/messaging/filters/PlayerFilterTest.java line 31

Okay, I'll do it right away.

In GitLab by @swabbur on May 20, 2020, 20:37

added 2 commits

ae03e3b7 - Document PlayerFilter tests
8e2a8710 - Fix scope of test class members

Compare with previous version

In GitLab by @swabbur on May 20, 2020, 20:37

resolved all threads

In GitLab by @wubero on May 20, 2020, 21:08

Commented on src/test/java/net/glowstone/messaging/MessagingSystemTest.java line 44

This whiteline and the one above the variable declaration are not needed in my opinion.

In GitLab by @swabbur on May 20, 2020, 21:09

Commented on src/test/java/net/glowstone/messaging/MessagingSystemTest.java line 44

changed this line in version 6 of the diff

In GitLab by @swabbur on May 20, 2020, 21:09

added 1 commit

a49b688b - Remove redundant whiteline from test setup

Compare with previous version

In GitLab by @wubero on May 20, 2020, 21:10

resolved all threads

In GitLab by @JimVliet on May 20, 2020, 21:15

Commented on src/main/java/net/glowstone/messaging/filters/PlayerFilter.java line 16

Previously we never checked if the message was null, what was your reason for adding it now?

In GitLab by @swabbur on May 20, 2020, 21:22

Commented on src/main/java/net/glowstone/messaging/filters/PlayerFilter.java line 16

I didn't have a specific purpose in mind, but it was a great way to test if the filter worked. Also, I figured it was as good a place as any to provide a little extra security from receiving invalid values from the external libraries/APIs used to implement brokers (which is why I left it in after testing if it worked).

In GitLab by @JimVliet on May 20, 2020, 21:24

Commented on src/test/java/net/glowstone/messaging/filters/PlayerFilterTest.java line 41

Since the BlockBreakAnimationMessage is easily constructed, I am in favour of replacing the mocked object with the real object.

In GitLab by @JimVliet on May 20, 2020, 21:34

Commented on src/main/java/net/glowstone/messaging/filters/PlayerFilter.java line 16

The filter is only for outgoing messages right? I am just wondering how null messages are currently handled in Opencraft. I suspect that they are already handled properly, which would make this check kind of unnecessary.

In GitLab by @swabbur on May 20, 2020, 21:56

Commented on src/test/java/net/glowstone/messaging/filters/PlayerFilterTest.java line 41

Okay, I'll change it once I get back. The idea behind using the mock was that the fields, other than id, were irrelevant to the test, and changes to those fields would not require altering the test.

In GitLab by @swabbur on May 20, 2020, 21:59

Commented on src/main/java/net/glowstone/messaging/filters/PlayerFilter.java line 16

The filter is for incoming messages, as it requires both the message and recipient. While I agree that Opencraft should already handle null-messages, the callback given to the messaging system need not necesarrily reference an existing part of Opencraft (e.g. Session.send). It could also forward messages to the dynamic conits or any other future message processing that may be required.

I'll just remove it, as other (future) parts should be able to handle null-messages and the broker implementations shouldn't be broadcasting null messages anyways.

In GitLab by @swabbur on May 20, 2020, 22:00

Commented on src/main/java/net/glowstone/messaging/filters/PlayerFilter.java line 16

I think I got incoming and outgoing confused haha

In GitLab by @swabbur on May 21, 2020, 10:45

Commented on src/test/java/net/glowstone/messaging/filters/PlayerFilterTest.java line 31

changed this line in version 7 of the diff

In GitLab by @swabbur on May 21, 2020, 10:45

Commented on src/main/java/net/glowstone/messaging/filters/PlayerFilter.java line 16

changed this line in version 7 of the diff

In GitLab by @swabbur on May 21, 2020, 10:45

Commented on src/test/java/net/glowstone/messaging/filters/PlayerFilterTest.java line 41

changed this line in version 7 of the diff