diff --git a/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs b/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs index 19bdaff6ff..736bfd8e7d 100644 --- a/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs +++ b/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs @@ -38,8 +38,13 @@ namespace osu.Game.Tests.Visual.Online private TestChatOverlay chatOverlay; private ChannelManager channelManager; + private IEnumerable visibleChannels => chatOverlay.ChannelTabControl.VisibleItems.Where(channel => channel.Name != "+"); + private IEnumerable joinedChannels => chatOverlay.ChannelTabControl.Items.Where(channel => channel.Name != "+"); private readonly List channels; + private Channel currentChannel => channelManager.CurrentChannel.Value; + private Channel nextChannel => joinedChannels.ElementAt(joinedChannels.ToList().IndexOf(currentChannel) + 1); + private Channel previousChannel => joinedChannels.ElementAt(joinedChannels.ToList().IndexOf(currentChannel) - 1); private Channel channel1 => channels[0]; private Channel channel2 => channels[1]; @@ -91,7 +96,7 @@ namespace osu.Game.Tests.Visual.Online AddStep("Join channel 1", () => channelManager.JoinChannel(channel1)); AddStep("Switch to channel 1", () => clickDrawable(chatOverlay.TabMap[channel1])); - AddAssert("Current channel is channel 1", () => channelManager.CurrentChannel.Value == channel1); + AddAssert("Current channel is channel 1", () => currentChannel == channel1); AddAssert("Channel selector was closed", () => chatOverlay.SelectionOverlayState == Visibility.Hidden); } @@ -102,12 +107,12 @@ namespace osu.Game.Tests.Visual.Online AddStep("Join channel 2", () => channelManager.JoinChannel(channel2)); AddStep("Switch to channel 2", () => clickDrawable(chatOverlay.TabMap[channel2])); - AddStep("Close channel 2", () => clickDrawable(((TestChannelTabItem)chatOverlay.TabMap[channel2]).CloseButton.Child)); + AddStep("Close channel 2", () => clickDrawable(((TestPrivateChannelTabItem)chatOverlay.TabMap[channel2]).CloseButton.Child)); AddAssert("Selector remained closed", () => chatOverlay.SelectionOverlayState == Visibility.Hidden); - AddAssert("Current channel is channel 1", () => channelManager.CurrentChannel.Value == channel1); + AddAssert("Current channel is channel 1", () => currentChannel == channel1); - AddStep("Close channel 1", () => clickDrawable(((TestChannelTabItem)chatOverlay.TabMap[channel1]).CloseButton.Child)); + AddStep("Close channel 1", () => clickDrawable(((TestPrivateChannelTabItem)chatOverlay.TabMap[channel1]).CloseButton.Child)); AddAssert("Selector is visible", () => chatOverlay.SelectionOverlayState == Visibility.Visible); } @@ -140,10 +145,67 @@ namespace osu.Game.Tests.Visual.Online var targetNumberKey = oneBasedIndex % 10; var targetChannel = channels[zeroBasedIndex]; AddStep($"press Alt+{targetNumberKey}", () => pressChannelHotkey(targetNumberKey)); - AddAssert($"channel #{oneBasedIndex} is selected", () => channelManager.CurrentChannel.Value == targetChannel); + AddAssert($"channel #{oneBasedIndex} is selected", () => currentChannel == targetChannel); } } + private Channel expectedChannel; + + [Test] + public void TestCloseChannelWhileActive() + { + AddUntilStep("Join until dropdown has channels", () => + { + if (visibleChannels.Count() < joinedChannels.Count()) + return true; + + // Using temporary channels because they don't hide their names when not active + Channel toAdd = new Channel { Name = $"test channel {joinedChannels.Count()}", Type = ChannelType.Temporary }; + channelManager.JoinChannel(toAdd); + + return false; + }); + + AddStep("Switch to last tab", () => clickDrawable(chatOverlay.TabMap[visibleChannels.Last()])); + AddAssert("Last visible selected", () => currentChannel == visibleChannels.Last()); + + // Closing the last channel before dropdown + AddStep("Close current channel", () => + { + expectedChannel = nextChannel; + chatOverlay.ChannelTabControl.RemoveChannel(currentChannel); + }); + AddAssert("Next channel selected", () => currentChannel == expectedChannel); + + // Depending on the window size, one more channel might need to be closed for the selectorTab to appear + AddUntilStep("Close channels until selector visible", () => + { + if (chatOverlay.ChannelTabControl.VisibleItems.Last().Name == "+") + return true; + + chatOverlay.ChannelTabControl.RemoveChannel(visibleChannels.Last()); + return false; + }); + AddAssert("Last visible selected", () => currentChannel == visibleChannels.Last()); + + // Closing the last channel with dropdown no longer present + AddStep("Close last when selector next", () => + { + expectedChannel = previousChannel; + chatOverlay.ChannelTabControl.RemoveChannel(currentChannel); + }); + AddAssert("Channel changed to previous", () => currentChannel == expectedChannel); + + // Standard channel closing + AddStep("Switch to previous channel", () => chatOverlay.ChannelTabControl.SwitchTab(-1)); + AddStep("Close current channel", () => + { + expectedChannel = nextChannel; + chatOverlay.ChannelTabControl.RemoveChannel(currentChannel); + }); + AddAssert("Channel changed to next", () => currentChannel == expectedChannel); + } + private void pressChannelHotkey(int number) { var channelKey = Key.Number0 + number; @@ -187,6 +249,8 @@ namespace osu.Game.Tests.Visual.Online { public Visibility SelectionOverlayState => ChannelSelectionOverlay.State.Value; + public new ChannelTabControl ChannelTabControl => base.ChannelTabControl; + public new ChannelSelectionOverlay ChannelSelectionOverlay => base.ChannelSelectionOverlay; protected override ChannelTabControl CreateChannelTabControl() => new TestTabControl(); @@ -196,12 +260,22 @@ namespace osu.Game.Tests.Visual.Online private class TestTabControl : ChannelTabControl { - protected override TabItem CreateTabItem(Channel value) => new TestChannelTabItem(value); + protected override TabItem CreateTabItem(Channel value) + { + switch (value.Type) + { + case ChannelType.PM: + return new TestPrivateChannelTabItem(value); + + default: + return new TestChannelTabItem(value); + } + } public new IReadOnlyDictionary> TabMap => base.TabMap; } - private class TestChannelTabItem : PrivateChannelTabItem + private class TestChannelTabItem : ChannelTabItem { public TestChannelTabItem(Channel channel) : base(channel) @@ -210,5 +284,15 @@ namespace osu.Game.Tests.Visual.Online public new ClickableContainer CloseButton => base.CloseButton; } + + private class TestPrivateChannelTabItem : PrivateChannelTabItem + { + public TestPrivateChannelTabItem(Channel channel) + : base(channel) + { + } + + public new ClickableContainer CloseButton => base.CloseButton; + } } } diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs index 80e03d82e2..8df75c78f5 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs @@ -399,7 +399,7 @@ namespace osu.Game.Tests.Visual.SongSelect AddStep("filter to ruleset 0", () => carousel.Filter(new FilterCriteria { Ruleset = rulesets.AvailableRulesets.ElementAt(0) }, false)); AddStep("select filtered map skipping filtered", () => carousel.SelectBeatmap(testMixed.Beatmaps[1], false)); - AddAssert("unfiltered beatmap selected", () => carousel.SelectedBeatmap.Equals(testMixed.Beatmaps[0])); + AddAssert("unfiltered beatmap not selected", () => carousel.SelectedBeatmap == null); AddStep("remove mixed set", () => { diff --git a/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs b/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs index 105d96cdfe..f1ff08b92c 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs @@ -436,6 +436,9 @@ namespace osu.Game.Tests.Visual.SongSelect changeRuleset(0); + // used for filter check below + AddStep("allow convert display", () => config.Set(OsuSetting.ShowConvertedBeatmaps, true)); + AddUntilStep("has selection", () => songSelect.Carousel.SelectedBeatmap != null); AddStep("set filter text", () => songSelect.FilterControl.ChildrenOfType().First().Text = "nonono"); @@ -446,16 +449,28 @@ namespace osu.Game.Tests.Visual.SongSelect BeatmapInfo target = null; + int targetRuleset = differentRuleset ? 1 : 0; + AddStep("select beatmap externally", () => { - target = manager.GetAllUsableBeatmapSets().Where(b => b.Beatmaps.Any(bi => bi.RulesetID == (differentRuleset ? 1 : 0))) - .ElementAt(5).Beatmaps.First(); + target = manager.GetAllUsableBeatmapSets() + .Where(b => b.Beatmaps.Any(bi => bi.RulesetID == targetRuleset)) + .ElementAt(5).Beatmaps.First(bi => bi.RulesetID == targetRuleset); Beatmap.Value = manager.GetWorkingBeatmap(target); }); AddUntilStep("has selection", () => songSelect.Carousel.SelectedBeatmap != null); + AddAssert("selected only shows expected ruleset (plus converts)", () => + { + var selectedPanel = songSelect.Carousel.ChildrenOfType().First(s => s.Item.State.Value == CarouselItemState.Selected); + + // special case for converts checked here. + return selectedPanel.ChildrenOfType().All(i => + i.IsFiltered || i.Item.Beatmap.Ruleset.ID == targetRuleset || i.Item.Beatmap.Ruleset.ID == 0); + }); + AddUntilStep("carousel has correct", () => songSelect.Carousel.SelectedBeatmap?.OnlineBeatmapID == target.OnlineBeatmapID); AddUntilStep("game has correct", () => Beatmap.Value.BeatmapInfo.OnlineBeatmapID == target.OnlineBeatmapID); @@ -576,16 +591,16 @@ namespace osu.Game.Tests.Visual.SongSelect } })); + BeatmapInfo filteredBeatmap = null; DrawableCarouselBeatmapSet.FilterableDifficultyIcon filteredIcon = null; + AddStep("Get filtered icon", () => { - var filteredBeatmap = songSelect.Carousel.SelectedBeatmapSet.Beatmaps.Find(b => b.BPM < maxBPM); + filteredBeatmap = songSelect.Carousel.SelectedBeatmapSet.Beatmaps.Find(b => b.BPM < maxBPM); int filteredBeatmapIndex = getBeatmapIndex(filteredBeatmap.BeatmapSet, filteredBeatmap); filteredIcon = set.ChildrenOfType().ElementAt(filteredBeatmapIndex); }); - int? previousID = null; - AddStep("Store current ID", () => previousID = songSelect.Carousel.SelectedBeatmap.ID); AddStep("Click on a filtered difficulty", () => { InputManager.MoveMouseTo(filteredIcon); @@ -593,7 +608,9 @@ namespace osu.Game.Tests.Visual.SongSelect InputManager.PressButton(MouseButton.Left); InputManager.ReleaseButton(MouseButton.Left); }); - AddAssert("Selected beatmap has not changed", () => songSelect.Carousel.SelectedBeatmap.ID == previousID); + + // todo: this logic is changed in follow up PR. + AddAssert("Selected beatmap not changed", () => songSelect.Carousel.SelectedBeatmap != filteredBeatmap); } private int getBeatmapIndex(BeatmapSetInfo set, BeatmapInfo info) => set.Beatmaps.FindIndex(b => b == info); diff --git a/osu.Game/Online/Chat/ChannelType.cs b/osu.Game/Online/Chat/ChannelType.cs index 7d2b661164..151efc4645 100644 --- a/osu.Game/Online/Chat/ChannelType.cs +++ b/osu.Game/Online/Chat/ChannelType.cs @@ -12,5 +12,6 @@ namespace osu.Game.Online.Chat Temporary, PM, Group, + System, } } diff --git a/osu.Game/Overlays/Chat/Tabs/ChannelSelectorTabItem.cs b/osu.Game/Overlays/Chat/Tabs/ChannelSelectorTabItem.cs index d5d9a6c2ce..e3ede04edd 100644 --- a/osu.Game/Overlays/Chat/Tabs/ChannelSelectorTabItem.cs +++ b/osu.Game/Overlays/Chat/Tabs/ChannelSelectorTabItem.cs @@ -39,6 +39,7 @@ namespace osu.Game.Overlays.Chat.Tabs public ChannelSelectorTabChannel() { Name = "+"; + Type = ChannelType.System; } } } diff --git a/osu.Game/Overlays/Chat/Tabs/ChannelTabControl.cs b/osu.Game/Overlays/Chat/Tabs/ChannelTabControl.cs index 104495ae01..a72f182450 100644 --- a/osu.Game/Overlays/Chat/Tabs/ChannelTabControl.cs +++ b/osu.Game/Overlays/Chat/Tabs/ChannelTabControl.cs @@ -41,7 +41,7 @@ namespace osu.Game.Overlays.Chat.Tabs // performTabSort might've made selectorTab's position wonky, fix it TabContainer.SetLayoutPosition(selectorTab, float.MaxValue); - ((ChannelTabItem)item).OnRequestClose += tabCloseRequested; + ((ChannelTabItem)item).OnRequestClose += channelItem => OnRequestLeave?.Invoke(channelItem.Value); base.AddTabItem(item, addToDropdown); } @@ -74,18 +74,24 @@ namespace osu.Game.Overlays.Chat.Tabs /// /// Removes a channel from the ChannelTabControl. - /// If the selected channel is the one that is beeing removed, the next available channel will be selected. + /// If the selected channel is the one that is being removed, the next available channel will be selected. /// /// The channel that is going to be removed. public void RemoveChannel(Channel channel) { - RemoveItem(channel); - if (Current.Value == channel) { - // Prefer non-selector channels first - Current.Value = Items.FirstOrDefault(c => !(c is ChannelSelectorTabItem.ChannelSelectorTabChannel)) ?? Items.FirstOrDefault(); + var allChannels = TabContainer.AllTabItems.Select(tab => tab.Value).ToList(); + var isNextTabSelector = allChannels[allChannels.IndexOf(channel) + 1] == selectorTab.Value; + + // selectorTab is not switchable, so we have to explicitly select it if it's the only tab left + if (isNextTabSelector && allChannels.Count == 2) + SelectTab(selectorTab); + else + SwitchTab(isNextTabSelector ? -1 : 1); } + + RemoveItem(channel); } protected override void SelectTab(TabItem tab) @@ -100,21 +106,6 @@ namespace osu.Game.Overlays.Chat.Tabs selectorTab.Active.Value = false; } - private void tabCloseRequested(TabItem tab) - { - int totalTabs = TabContainer.Count - 1; // account for selectorTab - int currentIndex = Math.Clamp(TabContainer.IndexOf(tab), 1, totalTabs); - - if (tab == SelectedTab && totalTabs > 1) - // Select the tab after tab-to-be-removed's index, or the tab before if current == last - SelectTab(TabContainer[currentIndex == totalTabs ? currentIndex - 1 : currentIndex + 1]); - else if (totalTabs == 1 && !selectorTab.Active.Value) - // Open channel selection overlay if all channel tabs will be closed after removing this tab - SelectTab(selectorTab); - - OnRequestLeave?.Invoke(tab.Value); - } - protected override TabFillFlowContainer CreateTabFlow() => new ChannelTabFillFlowContainer { Direction = FillDirection.Full, diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index 04c08cdbd2..92efde45ad 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -203,9 +203,6 @@ namespace osu.Game.Screens.Select /// /// Selects a given beatmap on the carousel. - /// - /// If bypassFilters is false, we will try to select another unfiltered beatmap in the same set. If the - /// entire set is filtered, no selection is made. /// /// The beatmap to select. /// Whether to select the beatmap even if it is filtered (i.e., not visible on carousel). @@ -227,25 +224,21 @@ namespace osu.Game.Screens.Select continue; if (!bypassFilters && item.Filtered.Value) - // The beatmap exists in this set but is filtered, so look for the first unfiltered map in the set - item = set.Beatmaps.FirstOrDefault(b => !b.Filtered.Value); + return false; - if (item != null) + select(item); + + // if we got here and the set is filtered, it means we were bypassing filters. + // in this case, reapplying the filter is necessary to ensure the panel is in the correct place + // (since it is forcefully being included in the carousel). + if (set.Filtered.Value) { - select(item); + Debug.Assert(bypassFilters); - // if we got here and the set is filtered, it means we were bypassing filters. - // in this case, reapplying the filter is necessary to ensure the panel is in the correct place - // (since it is forcefully being included in the carousel). - if (set.Filtered.Value) - { - Debug.Assert(bypassFilters); - - applyActiveCriteria(false); - } - - return true; + applyActiveCriteria(false); } + + return true; } return false; diff --git a/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs b/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs index 8c264ce974..6d760df065 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs @@ -25,18 +25,18 @@ namespace osu.Game.Screens.Select.Carousel { base.Filter(criteria); - if (Beatmap.BeatmapSet?.Equals(criteria.SelectedBeatmapSet) == true) - { - // bypass filtering for selected beatmap - Filtered.Value = false; - return; - } - bool match = criteria.Ruleset == null || Beatmap.RulesetID == criteria.Ruleset.ID || (Beatmap.RulesetID == 0 && criteria.Ruleset.ID > 0 && criteria.AllowConvertedBeatmaps); + if (Beatmap.BeatmapSet?.Equals(criteria.SelectedBeatmapSet) == true) + { + // only check ruleset equality or convertability for selected beatmap + Filtered.Value = !match; + return; + } + match &= !criteria.StarDifficulty.HasFilter || criteria.StarDifficulty.IsInRange(Beatmap.StarDifficulty); match &= !criteria.ApproachRate.HasFilter || criteria.ApproachRate.IsInRange(Beatmap.BaseDifficulty.ApproachRate); match &= !criteria.DrainRate.HasFilter || criteria.DrainRate.IsInRange(Beatmap.BaseDifficulty.DrainRate); diff --git a/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs b/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs index 6cd145cfef..547aeaddc6 100644 --- a/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs +++ b/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs @@ -205,7 +205,9 @@ namespace osu.Game.Screens.Select.Carousel { private readonly BindableBool filtered = new BindableBool(); - private readonly CarouselBeatmap item; + public bool IsFiltered => filtered.Value; + + public readonly CarouselBeatmap Item; public FilterableDifficultyIcon(CarouselBeatmap item) : base(item.Beatmap) @@ -214,13 +216,13 @@ namespace osu.Game.Screens.Select.Carousel filtered.ValueChanged += isFiltered => Schedule(() => this.FadeTo(isFiltered.NewValue ? 0.1f : 1, 100)); filtered.TriggerChange(); - this.item = item; + Item = item; } protected override bool OnClick(ClickEvent e) { if (!filtered.Value) - item.State.Value = CarouselItemState.Selected; + Item.State.Value = CarouselItemState.Selected; return true; } diff --git a/osu.Game/Screens/Select/SongSelect.cs b/osu.Game/Screens/Select/SongSelect.cs index 528222a89c..11c680bdb0 100644 --- a/osu.Game/Screens/Select/SongSelect.cs +++ b/osu.Game/Screens/Select/SongSelect.cs @@ -380,6 +380,8 @@ namespace osu.Game.Screens.Select { if (e.NewValue is DummyWorkingBeatmap || !this.IsCurrentScreen()) return; + Logger.Log($"working beatmap updated to {e.NewValue}"); + if (!Carousel.SelectBeatmap(e.NewValue.BeatmapInfo, false)) { // A selection may not have been possible with filters applied. @@ -446,8 +448,10 @@ namespace osu.Game.Screens.Select if (transferRulesetValue()) { - // if the ruleset changed, the rest of the selection update will happen via updateSelectedRuleset. Mods.Value = Array.Empty(); + + // required to return once in order to have the carousel in a good state. + // if the ruleset changed, the rest of the selection update will happen via updateSelectedRuleset. return; } @@ -472,7 +476,7 @@ namespace osu.Game.Screens.Select if (this.IsCurrentScreen()) ensurePlayingSelected(); - UpdateBeatmap(Beatmap.Value); + updateComponentFromBeatmap(Beatmap.Value); } } @@ -547,7 +551,7 @@ namespace osu.Game.Screens.Select if (Beatmap != null && !Beatmap.Value.BeatmapSetInfo.DeletePending) { - UpdateBeatmap(Beatmap.Value); + updateComponentFromBeatmap(Beatmap.Value); // restart playback on returning to song select, regardless. music?.Play(); @@ -610,10 +614,8 @@ namespace osu.Game.Screens.Select /// This is a debounced call (unlike directly binding to WorkingBeatmap.ValueChanged). /// /// The working beatmap. - protected virtual void UpdateBeatmap(WorkingBeatmap beatmap) + private void updateComponentFromBeatmap(WorkingBeatmap beatmap) { - Logger.Log($"working beatmap updated to {beatmap}"); - if (Background is BackgroundScreenBeatmap backgroundModeBeatmap) { backgroundModeBeatmap.Beatmap = beatmap; @@ -658,9 +660,17 @@ namespace osu.Game.Screens.Select return; // Attempt to select the current beatmap on the carousel, if it is valid to be selected. - if (!Beatmap.IsDefault && Beatmap.Value.BeatmapSetInfo?.DeletePending == false && Beatmap.Value.BeatmapSetInfo?.Protected == false - && Carousel.SelectBeatmap(Beatmap.Value.BeatmapInfo, false)) - return; + if (!Beatmap.IsDefault && Beatmap.Value.BeatmapSetInfo?.DeletePending == false && Beatmap.Value.BeatmapSetInfo?.Protected == false) + { + if (Carousel.SelectBeatmap(Beatmap.Value.BeatmapInfo, false)) + return; + + // prefer not changing ruleset at this point, so look for another difficulty in the currently playing beatmap + var found = Beatmap.Value.BeatmapSetInfo.Beatmaps.FirstOrDefault(b => b.Ruleset.Equals(decoupledRuleset.Value)); + + if (found != null && Carousel.SelectBeatmap(found, false)) + return; + } // If the current active beatmap could not be selected, select a new random beatmap. if (!Carousel.SelectNextRandom())