diff --git a/src/main/java/com/faforever/client/chat/ChatController.java b/src/main/java/com/faforever/client/chat/ChatController.java index 0cc3b4d6b7..9976f52a80 100644 --- a/src/main/java/com/faforever/client/chat/ChatController.java +++ b/src/main/java/com/faforever/client/chat/ChatController.java @@ -2,13 +2,11 @@ import com.faforever.client.exception.ProgrammingError; import com.faforever.client.fx.FxApplicationThreadExecutor; -import com.faforever.client.fx.JavaFxUtil; import com.faforever.client.fx.NodeController; import com.faforever.client.net.ConnectionState; import com.faforever.client.theme.UiService; -import javafx.collections.ListChangeListener; import javafx.collections.MapChangeListener; -import javafx.collections.WeakListChangeListener; +import javafx.collections.ObservableList; import javafx.collections.WeakMapChangeListener; import javafx.scene.control.Tab; import javafx.scene.control.TabPane; @@ -22,9 +20,6 @@ import org.springframework.context.annotation.Scope; import org.springframework.stereotype.Component; -import java.util.HashMap; -import java.util.Map; - @Slf4j @Component @Scope(ConfigurableBeanFactory.SCOPE_PROTOTYPE) @@ -34,13 +29,8 @@ public class ChatController extends NodeController { private final ChatService chatService; private final UiService uiService; private final FxApplicationThreadExecutor fxApplicationThreadExecutor; + private final ChatNavigation chatNavigation; - private final Map channelToChatTabController = new HashMap<>(); - private final ListChangeListener tabListChangeListener = change -> { - while (change.next()) { - change.getRemoved().forEach(tab -> channelToChatTabController.remove((ChatChannel) tab.getUserData())); - } - }; private final MapChangeListener channelChangeListener = change -> { if (change.wasRemoved()) { onChannelLeft(change.getValueRemoved()); @@ -57,16 +47,22 @@ public class ChatController extends NodeController { public TextField channelNameTextField; public VBox disconnectedPane; + private ObservableList openedTabs; + @Override protected void onInitialize() { super.onInitialize(); + openedTabs = tabPane.getTabs(); chatService.addChannelsListener(new WeakMapChangeListener<>(channelChangeListener)); chatService.getChannels().forEach(this::onChannelJoined); chatService.connectionStateProperty().when(showing).subscribe(this::onConnectionStateChange); - - JavaFxUtil.addListener(tabPane.getTabs(), new WeakListChangeListener<>(tabListChangeListener)); + tabPane.getSelectionModel().selectedItemProperty().subscribe(tab -> { + if (tab.isClosable()) { + chatNavigation.setLastOpenedTabId(tab.getId()); + } + }); } private void onChannelLeft(ChatChannel chatChannel) { @@ -90,7 +86,8 @@ private void onDisconnected() { disconnectedPane.setVisible(true); connectingProgressPane.setVisible(false); tabPane.setVisible(false); - tabPane.getTabs().removeIf(Tab::isClosable); + openedTabs.removeIf(Tab::isClosable); + chatNavigation.clear(); }); } @@ -112,42 +109,41 @@ private void onConnecting() { } private void removeTab(ChatChannel chatChannel) { - AbstractChatTabController controller = channelToChatTabController.remove(chatChannel); - if (controller != null) { - fxApplicationThreadExecutor.execute(() -> tabPane.getTabs().remove(controller.getRoot())); - } + fxApplicationThreadExecutor.execute(() -> { + String channelName = chatChannel.getName(); + openedTabs.removeIf(tab -> channelName.equals(tab.getId())); + chatNavigation.removeTab(channelName); + }); } private void addAndSelectTab(ChatChannel chatChannel) { - if (!channelToChatTabController.containsKey(chatChannel)) { + if (!containsTab(chatChannel)) { fxApplicationThreadExecutor.execute(() -> { - AbstractChatTabController tabController; - if (chatChannel.isPrivateChannel()) { - tabController = uiService.loadFxml("theme/chat/private_chat_tab.fxml"); - } else { - tabController = uiService.loadFxml("theme/chat/channel_tab.fxml"); - } - tabController.setChatChannel(chatChannel); - channelToChatTabController.put(chatChannel, tabController); - Tab tab = tabController.getRoot(); - tab.setUserData(chatChannel); + Tab tab = createTab(chatChannel); + int index = chatService.isDefaultChannel(chatChannel) ? 0 : openedTabs.size() - 1; // Last index is `add channel` tab + openedTabs.add(index, tab); - - if (chatService.isDefaultChannel(chatChannel)) { - tabPane.getTabs().addFirst(tab); + String channelName = chatChannel.getName(); + if (chatNavigation.addTabIfMissing(channelName) || channelName.equals(chatNavigation.getLastOpenedTabId())) { tabPane.getSelectionModel().select(tab); - } else { - tabPane.getTabs().add(tabPane.getTabs().size() - 1, tab); - - if (chatChannel.isPrivateChannel() || tabPane.getSelectionModel().getSelectedIndex() == tabPane.getTabs() - .size() - 1) { - tabPane.getSelectionModel().select(tab); - } } }); } } + private boolean containsTab(ChatChannel chatChannel) { + return openedTabs.stream().anyMatch(tab -> chatChannel.getName().equals(tab.getId())); + } + + private Tab createTab(ChatChannel chatChannel) { + String fxmlFile = chatChannel.isPrivateChannel() ? "theme/chat/private_chat_tab.fxml" : "theme/chat/channel_tab.fxml"; + AbstractChatTabController tabController = uiService.loadFxml(fxmlFile); + tabController.setChatChannel(chatChannel); + Tab tab = tabController.getRoot(); + tab.setId(chatChannel.getName()); + return tab; + } + private void onConnectionStateChange(ConnectionState newValue) { switch (newValue) { case DISCONNECTED -> onDisconnected(); diff --git a/src/main/java/com/faforever/client/chat/ChatMessageViewController.java b/src/main/java/com/faforever/client/chat/ChatMessageViewController.java index 6e013f5722..2502617392 100644 --- a/src/main/java/com/faforever/client/chat/ChatMessageViewController.java +++ b/src/main/java/com/faforever/client/chat/ChatMessageViewController.java @@ -32,7 +32,6 @@ import javafx.scene.layout.HBox; import javafx.scene.layout.Priority; import javafx.scene.layout.VBox; -import javafx.scene.web.WebView; import javafx.stage.Popup; import javafx.stage.PopupWindow.AnchorLocation; import lombok.RequiredArgsConstructor; @@ -55,11 +54,6 @@ import java.util.concurrent.CompletableFuture; import java.util.stream.Collectors; -/** - * A chat tab displays messages in a {@link WebView}. The WebView is used since text on a JavaFX canvas isn't - * selectable, but text within a WebView is. This comes with some ugly implications; some logic has to be performed in - * interaction with JavaScript, like when the user clicks a link. - */ @Slf4j @RequiredArgsConstructor @Component diff --git a/src/main/java/com/faforever/client/chat/ChatNavigation.java b/src/main/java/com/faforever/client/chat/ChatNavigation.java new file mode 100644 index 0000000000..b951484793 --- /dev/null +++ b/src/main/java/com/faforever/client/chat/ChatNavigation.java @@ -0,0 +1,35 @@ +package com.faforever.client.chat; + +import org.jetbrains.annotations.Nullable; +import org.springframework.context.annotation.Lazy; +import org.springframework.stereotype.Component; + +import java.util.LinkedHashSet; + +@Component +@Lazy +public class ChatNavigation { + + private final LinkedHashSet currentTabs = new LinkedHashSet<>(); + + public boolean addTabIfMissing(String tabId) { + return currentTabs.add(new ChatTab(tabId)); + } + + public void removeTab(String tabId) { + currentTabs.removeIf(chatTab -> chatTab.getId().equals(tabId)); + } + + @Nullable + public String getLastOpenedTabId() { + return currentTabs.stream().filter(ChatTab::isSelected).findFirst().map(ChatTab::getId).orElse(null); + } + + public void setLastOpenedTabId(String tabId) { + currentTabs.forEach(tab -> tab.setSelected(tab.getId().equals(tabId))); + } + + public void clear() { + currentTabs.clear(); + } +} diff --git a/src/main/java/com/faforever/client/chat/ChatTab.java b/src/main/java/com/faforever/client/chat/ChatTab.java new file mode 100644 index 0000000000..a06f0f6d8d --- /dev/null +++ b/src/main/java/com/faforever/client/chat/ChatTab.java @@ -0,0 +1,17 @@ +package com.faforever.client.chat; + +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.RequiredArgsConstructor; +import lombok.Setter; + +@RequiredArgsConstructor +@Getter +@EqualsAndHashCode(onlyExplicitlyIncluded = true) +public class ChatTab { + + @EqualsAndHashCode.Include + private final String id; + @Setter + private boolean selected; +} diff --git a/src/test/java/com/faforever/client/chat/ChatControllerTest.java b/src/test/java/com/faforever/client/chat/ChatControllerTest.java index f72303d25e..dadfa4bd08 100644 --- a/src/test/java/com/faforever/client/chat/ChatControllerTest.java +++ b/src/test/java/com/faforever/client/chat/ChatControllerTest.java @@ -1,143 +1,197 @@ package com.faforever.client.chat; import com.faforever.client.net.ConnectionState; -import com.faforever.client.notification.NotificationService; import com.faforever.client.test.PlatformTest; import com.faforever.client.theme.UiService; -import com.faforever.client.user.LoginService; -import com.faforever.commons.api.dto.MeResult; -import javafx.beans.InvalidationListener; +import javafx.beans.property.ObjectProperty; import javafx.beans.property.SimpleObjectProperty; import javafx.collections.MapChangeListener; +import javafx.collections.ObservableList; +import javafx.collections.ObservableMap; import javafx.scene.control.Tab; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.mockito.ArgumentCaptor; import org.mockito.Captor; import org.mockito.InjectMocks; import org.mockito.Mock; +import org.mockito.Spy; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.TimeUnit; +import java.util.Set; -import static org.hamcrest.CoreMatchers.is; -import static org.hamcrest.CoreMatchers.nullValue; -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.hasSize; +import static javafx.collections.FXCollections.observableHashMap; +import static javafx.collections.FXCollections.synchronizedObservableMap; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.Mockito.doAnswer; -import static org.mockito.Mockito.doReturn; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -// TODO those unit tests need to be improved (missing verifications) public class ChatControllerTest extends PlatformTest { - public static final String TEST_USER_NAME = "junit"; - private static final String TEST_CHANNEL_NAME = "#testChannel"; - - @Mock - private ChannelTabController channelTabController; @Mock - private PrivateChatTabController privateChatTabController; - @Mock - private LoginService loginService; + private AbstractChatTabController tabController; @Mock private UiService uiService; @Mock private ChatService chatService; - @Mock - private NotificationService notificationService; + @Spy + private ChatNavigation chatNavigation; @Captor private ArgumentCaptor> channelsListener; + private final ObservableMap channels = synchronizedObservableMap(observableHashMap()); + private final ObjectProperty connectionState = new SimpleObjectProperty<>(ConnectionState.DISCONNECTED); @InjectMocks private ChatController instance; - private SimpleObjectProperty connectionState; + private ObservableList openedTabs; @BeforeEach public void setUp() throws Exception { - connectionState = new SimpleObjectProperty<>(ConnectionState.DISCONNECTED); - - lenient().when(uiService.loadFxml("theme/chat/private_chat_tab.fxml")).thenReturn(privateChatTabController); - lenient().when(uiService.loadFxml("theme/chat/channel_tab.fxml")).thenReturn(channelTabController); - lenient().when(loginService.getUsername()).thenReturn(TEST_USER_NAME); - lenient().when(loginService.getOwnUser()).thenReturn(new MeResult()); lenient().when(chatService.connectionStateProperty()).thenReturn(connectionState); + lenient().when(uiService.loadFxml("theme/chat/private_chat_tab.fxml")).thenReturn(tabController); + lenient().when(uiService.loadFxml("theme/chat/channel_tab.fxml")).thenReturn(tabController); + lenient().when(tabController.getRoot()).thenAnswer(invocation -> new Tab()); + loadFxml("theme/chat/chat.fxml", clazz -> instance); + openedTabs = instance.tabPane.getTabs(); + } + private void setChannelsListener() { verify(chatService).addChannelsListener(channelsListener.capture()); + channels.addListener(channelsListener.getValue()); + } + + @Test + public void testOnlyOneAddChannelTabAfterInitialized() { + assertEquals(1, openedTabs.size()); + assertFalse(openedTabs.getFirst().isClosable()); } @Test - public void testOnDisconnected() throws Exception { - connectionState.set(ConnectionState.DISCONNECTED); + public void testOnChannelJoined() { + setChannelsListener(); + + requestChannel("aeolus", true); + requestChannel("channel"); + requestChannel("player"); + + assertContainsTab("aeolus"); + assertContainsTab("channel"); + assertContainsTab("player"); } @Test - public void testGetRoot() throws Exception { - assertThat(instance.getRoot(), is(instance.chatRoot)); - assertThat(instance.getRoot().getParent(), is(nullValue())); + public void testAddedTabShouldBeSelected() { + setChannelsListener(); + requestChannel("channel"); + + assertEquals("channel", chatNavigation.getLastOpenedTabId()); + assertEquals("channel", instance.tabPane.getSelectionModel().getSelectedItem().getId()); } @Test - public void testOnChannelsJoinedRequest() throws Exception { - channelJoined(TEST_CHANNEL_NAME); + public void testSelectedTabShouldBeRemembered() { + setChannelsListener(); + requestChannel("channel1"); + requestChannel("channel2"); + + assertEquals("channel2", chatNavigation.getLastOpenedTabId()); + assertEquals("channel2", instance.tabPane.getSelectionModel().getSelectedItem().getId()); - connectionState.set(ConnectionState.DISCONNECTED); + runOnFxThreadAndWait(() -> instance.tabPane.getSelectionModel().selectPrevious()); + + assertEquals("channel1", chatNavigation.getLastOpenedTabId()); + assertEquals("channel1", instance.tabPane.getSelectionModel().getSelectedItem().getId()); } - private void channelJoined(String channel) { - MapChangeListener.Change testChannelChange = mock(MapChangeListener.Change.class); - channelsListener.getValue().onChanged(testChannelChange); + private void assertContainsTab(String channel) { + assertTrue(openedTabs.stream().anyMatch(tab -> tab.getId().equals(channel))); } @Test - @Disabled("Flaky test") - public void testOnJoinChannelButtonClicked() throws Exception { - assertEquals(instance.tabPane.getTabs().size(), 1); - - Tab tab = new Tab(); - tab.setId(TEST_CHANNEL_NAME); - - when(channelTabController.getRoot()).thenReturn(tab); - when(loginService.getUsername()).thenReturn(TEST_USER_NAME); - doAnswer(invocation -> { - MapChangeListener.Change change = mock(MapChangeListener.Change.class); - when(change.wasAdded()).thenReturn(true); - doReturn(new ChatChannel(invocation.getArgument(0))).when(change).getValueAdded(); - channelsListener.getValue().onChanged(change); - return null; - }).when(chatService).joinChannel(anyString()); - - instance.channelNameTextField.setText(TEST_CHANNEL_NAME); - instance.onJoinChannelButtonClicked(); - - verify(chatService).joinChannel(TEST_CHANNEL_NAME); - - CountDownLatch tabAddedLatch = new CountDownLatch(1); - instance.tabPane.getTabs().addListener((InvalidationListener) observable -> tabAddedLatch.countDown()); - tabAddedLatch.await(2, TimeUnit.SECONDS); - - assertThat(instance.tabPane.getTabs(), hasSize(2)); - assertThat(instance.tabPane.getTabs().getFirst().getId(), is(TEST_CHANNEL_NAME)); + public void testOnDisconnected() { + setChannelsListener(); + + connectionState.set(ConnectionState.CONNECTED); + requestChannel("aeolus", true); + + runOnFxThreadAndWait(() -> connectionState.set(ConnectionState.DISCONNECTED)); + assertTrue(instance.disconnectedPane.isVisible()); + assertFalse(instance.connectingProgressPane.isVisible()); + assertFalse(instance.tabPane.isVisible()); + assertEquals(1, openedTabs.size()); + assertNull(chatNavigation.getLastOpenedTabId()); } @Test - public void testOnJoinChannelButtonClickedInvalidChannel() throws Exception { - assertEquals(instance.tabPane.getTabs().size(), 1); + public void testOnConnected() { + assertEquals(ConnectionState.DISCONNECTED, connectionState.getValue()); - Tab tab = new Tab(); - tab.setId(TEST_CHANNEL_NAME); + ChatChannel chatChannelMock1 = mock(ChatChannel.class); + ChatChannel chatChannelMock2 = mock(ChatChannel.class); + when(chatChannelMock1.getName()).thenReturn("1"); + when(chatChannelMock2.getName()).thenReturn("2"); + when(chatService.getChannels()).thenReturn(Set.of(chatChannelMock1, chatChannelMock2)); - instance.channelNameTextField.setText(TEST_CHANNEL_NAME.replace("#", "")); - instance.onJoinChannelButtonClicked(); + runOnFxThreadAndWait(() -> connectionState.set(ConnectionState.CONNECTED)); - verify(chatService).joinChannel(TEST_CHANNEL_NAME); + assertFalse(instance.disconnectedPane.isVisible()); + assertFalse(instance.connectingProgressPane.isVisible()); + assertTrue(instance.tabPane.isVisible()); + assertEquals(3, openedTabs.size()); + } + + @Test + public void testOnConnecting() { + assertEquals(ConnectionState.DISCONNECTED, connectionState.getValue()); + runOnFxThreadAndWait(() -> connectionState.set(ConnectionState.CONNECTING)); + assertFalse(instance.disconnectedPane.isVisible()); + assertTrue(instance.connectingProgressPane.isVisible()); + assertFalse(instance.tabPane.isVisible()); + } + + private void requestChannel(String channel) { + requestChannel(channel, false); + } + + private void requestChannel(String channel, boolean isDefaultChannel) { + ChatChannel chatChannelMock = mock(ChatChannel.class); + when(chatChannelMock.getName()).thenReturn(channel); + when(chatService.isDefaultChannel(chatChannelMock)).thenReturn(isDefaultChannel); + channels.putIfAbsent(channel, chatChannelMock); + waitFxEvents(); + } + + @Test + public void testOnChannelLeft() { + setChannelsListener(); + requestChannel("aeolus", true); + requestChannel("channel", false); + + Tab tab = openedTabs.stream().filter(tab1 -> tab1.getId().equals("aeolus")).findFirst().orElseThrow(); + runOnFxThreadAndWait(() -> openedTabs.remove(tab)); + assertEquals(2, openedTabs.size()); + } + + @Test + public void testOnJoinChannelButtonClicked() { + runOnFxThreadAndWait(() -> { + instance.channelNameTextField.setText("newChannel"); + instance.onJoinChannelButtonClicked(); + }); + + verify(chatService).joinChannel("#newChannel"); + assertTrue(instance.channelNameTextField.getText().isEmpty()); + } + + @Test + public void testOnConnectButtonClicked() { + runOnFxThreadAndWait(() -> instance.onConnectButtonClicked()); + verify(chatService).connect(); } } diff --git a/src/test/java/com/faforever/client/test/PlatformTest.java b/src/test/java/com/faforever/client/test/PlatformTest.java index d3f91a6325..8b99dfa31a 100644 --- a/src/test/java/com/faforever/client/test/PlatformTest.java +++ b/src/test/java/com/faforever/client/test/PlatformTest.java @@ -115,6 +115,10 @@ protected URL getThemeFileUrl(String file) throws IOException { protected void runOnFxThreadAndWait(Runnable runnable) { Platform.runLater(runnable); + waitFxEvents(); + } + + protected void waitFxEvents() { WaitForAsyncUtils.waitForFxEvents(); }