From 63edcddaf13453d66e86d4368e63ed90d2636962 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 12 Mar 2020 15:01:43 +0900 Subject: [PATCH 01/16] Apply ruleset filter in all cases (even when bypassing filter for selection purposes) --- .../Screens/Select/Carousel/CarouselBeatmap.cs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs b/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs index 8c264ce974..e0d59e3b18 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) + { + // bypass filtering 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); From 933a8ffc8a8152e6e5680d39557bd3e2958f407b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 12 Mar 2020 17:10:51 +0900 Subject: [PATCH 02/16] Add test coverage --- .../Visual/SongSelect/TestScenePlaySongSelect.cs | 16 +++++++++++++++- .../Carousel/DrawableCarouselBeatmapSet.cs | 8 +++++--- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs b/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs index 105d96cdfe..fb287a4f70 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,9 +449,11 @@ 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))) + target = manager.GetAllUsableBeatmapSets().Where(b => b.Beatmaps.Any(bi => bi.RulesetID == targetRuleset)) .ElementAt(5).Beatmaps.First(); Beatmap.Value = manager.GetWorkingBeatmap(target); @@ -456,6 +461,15 @@ namespace osu.Game.Tests.Visual.SongSelect 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); diff --git a/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs b/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs index 6cd145cfef..1454784f03 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; + this.Item = item; } protected override bool OnClick(ClickEvent e) { if (!filtered.Value) - item.State.Value = CarouselItemState.Selected; + Item.State.Value = CarouselItemState.Selected; return true; } From fc058f8896eb8f023b6e7702d21355167f1a78b4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 12 Mar 2020 18:03:18 +0900 Subject: [PATCH 03/16] Remove unnecessary this. prefix --- osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs b/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs index 1454784f03..547aeaddc6 100644 --- a/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs +++ b/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs @@ -216,7 +216,7 @@ 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) From 5537b279de1b3707496f4bf8aac49aa359f13cbc Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 12 Mar 2020 18:39:43 +0900 Subject: [PATCH 04/16] Fix failing test occasionally getting wrong ruleset beatmap --- osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs b/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs index fb287a4f70..55c1d8451f 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs @@ -453,8 +453,9 @@ namespace osu.Game.Tests.Visual.SongSelect AddStep("select beatmap externally", () => { - target = manager.GetAllUsableBeatmapSets().Where(b => b.Beatmaps.Any(bi => bi.RulesetID == targetRuleset)) - .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); }); From ca9cfbe51d50a7d9b95096efee9b519d8a607ae3 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 12 Mar 2020 15:52:03 +0900 Subject: [PATCH 05/16] Move selection fallback logic out of BeatmapCarousel to SongSelect --- osu.Game/Screens/Select/BeatmapCarousel.cs | 26 +++++++++----------- osu.Game/Screens/Select/SongSelect.cs | 28 +++++++++++++++------- 2 files changed, 30 insertions(+), 24 deletions(-) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index 71744d8b80..ca20b02bce 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -225,25 +225,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/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()) From bab197553e3968263cf0ee93776707d8da7c53c9 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 12 Mar 2020 15:34:58 +0900 Subject: [PATCH 06/16] Update carousel test logic to match new carousel selection behaviour --- .../Visual/SongSelect/TestSceneBeatmapCarousel.cs | 2 +- .../Visual/SongSelect/TestScenePlaySongSelect.cs | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs index 71ae47dc66..d80add3015 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 55c1d8451f..d16fd0bceb 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs @@ -591,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); @@ -608,7 +608,8 @@ 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); + + AddAssert("Selected beatmap correct", () => songSelect.Carousel.SelectedBeatmap == filteredBeatmap); } private int getBeatmapIndex(BeatmapSetInfo set, BeatmapInfo info) => set.Beatmaps.FindIndex(b => b == info); From 3f8b454ff4783b896516506b4ef192cda78134f2 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 13 Mar 2020 10:01:28 +0900 Subject: [PATCH 07/16] Reword comment to match new filtering behaviour MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Bartłomiej Dach --- osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs b/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs index e0d59e3b18..6d760df065 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs @@ -32,7 +32,7 @@ namespace osu.Game.Screens.Select.Carousel if (Beatmap.BeatmapSet?.Equals(criteria.SelectedBeatmapSet) == true) { - // bypass filtering for selected beatmap + // only check ruleset equality or convertability for selected beatmap Filtered.Value = !match; return; } From 04f1da04db8c685305b8688dd29d7d4af0985c6b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 13 Mar 2020 10:52:08 +0900 Subject: [PATCH 08/16] Remove incorrect xmldoc from SelectBeatmap function --- osu.Game/Screens/Select/BeatmapCarousel.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index ca20b02bce..34d659cc90 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -201,9 +201,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). From ba0dec891d4a576f6158bff9235bc37cbb224f14 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 13 Mar 2020 10:58:36 +0900 Subject: [PATCH 09/16] Update test temporarily --- osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs b/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs index d16fd0bceb..f1ff08b92c 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs @@ -609,7 +609,8 @@ namespace osu.Game.Tests.Visual.SongSelect InputManager.ReleaseButton(MouseButton.Left); }); - AddAssert("Selected beatmap correct", () => songSelect.Carousel.SelectedBeatmap == filteredBeatmap); + // 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); From 097bd37e37c38095113bd706717a9601afc1f3c0 Mon Sep 17 00:00:00 2001 From: TheWildTree Date: Fri, 13 Mar 2020 18:27:33 +0100 Subject: [PATCH 10/16] Fix SelectorTab crashing tests after a reload For some reason, the default channel type (Public) caused the channel manager to attempt to connect to an API, which was null at that time, after hot reloading the test environment (via dynamic compilation). Changing the channel type seems to fix that. --- osu.Game/Overlays/Chat/Tabs/ChannelSelectorTabItem.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/osu.Game/Overlays/Chat/Tabs/ChannelSelectorTabItem.cs b/osu.Game/Overlays/Chat/Tabs/ChannelSelectorTabItem.cs index d5d9a6c2ce..5fb56a3f75 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.Temporary; } } } From 8991e88039b0af4e39dd7122588542f7a916ccf8 Mon Sep 17 00:00:00 2001 From: TheWildTree Date: Fri, 13 Mar 2020 18:33:06 +0100 Subject: [PATCH 11/16] Fix active tab closing behaviour --- .../Overlays/Chat/Tabs/ChannelTabControl.cs | 33 +++++++------------ 1 file changed, 12 insertions(+), 21 deletions(-) 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, From 694e56b0d13cc90e0f82b1f14661dd06fbf03f8d Mon Sep 17 00:00:00 2001 From: TheWildTree Date: Fri, 13 Mar 2020 18:33:58 +0100 Subject: [PATCH 12/16] Add non-PM chat tabs to tests --- .../Visual/Online/TestSceneChatOverlay.cs | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs b/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs index 19bdaff6ff..8aa30c7fa3 100644 --- a/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs +++ b/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs @@ -1,4 +1,4 @@ -// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. using System; @@ -196,12 +196,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 +220,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; + } } } From 0bbae094ddf854d6e9d6ee3c3197df0577071231 Mon Sep 17 00:00:00 2001 From: TheWildTree Date: Fri, 13 Mar 2020 18:34:22 +0100 Subject: [PATCH 13/16] Add active tab closing behaviour tests --- .../Visual/Online/TestSceneChatOverlay.cs | 76 +++++++++++++++++-- 1 file changed, 70 insertions(+), 6 deletions(-) diff --git a/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs b/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs index 8aa30c7fa3..ede99c06be 100644 --- a/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs +++ b/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs @@ -1,4 +1,4 @@ -// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. using System; @@ -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(); From 8d3cab0e1696e62c3c018cab91a1ee797b8bcc03 Mon Sep 17 00:00:00 2001 From: TheWildTree Date: Fri, 13 Mar 2020 18:58:32 +0100 Subject: [PATCH 14/16] Trim whitespace --- osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs b/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs index ede99c06be..297a51c4a5 100644 --- a/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs +++ b/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs @@ -1,4 +1,4 @@ -// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. using System; @@ -107,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(((TestPrivateChannelTabItem)chatOverlay.TabMap[channel2]).CloseButton.Child)); + AddStep("Close channel 2", () => clickDrawable(((TestChannelTabItem)chatOverlay.TabMap[channel2]).CloseButton.Child)); AddAssert("Selector remained closed", () => chatOverlay.SelectionOverlayState == Visibility.Hidden); AddAssert("Current channel is channel 1", () => currentChannel == channel1); - AddStep("Close channel 1", () => clickDrawable(((TestPrivateChannelTabItem)chatOverlay.TabMap[channel1]).CloseButton.Child)); + AddStep("Close channel 1", () => clickDrawable(((TestChannelTabItem)chatOverlay.TabMap[channel1]).CloseButton.Child)); AddAssert("Selector is visible", () => chatOverlay.SelectionOverlayState == Visibility.Visible); } From 38d00c7f0ae6abb0283a366fadfca6cabb048948 Mon Sep 17 00:00:00 2001 From: TheWildTree Date: Fri, 13 Mar 2020 21:29:10 +0100 Subject: [PATCH 15/16] Revert unnecessary changes and actually trim the whitespace --- osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs b/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs index 297a51c4a5..736bfd8e7d 100644 --- a/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs +++ b/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs @@ -1,4 +1,4 @@ -// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. using System; @@ -107,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", () => 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); } @@ -189,7 +189,7 @@ namespace osu.Game.Tests.Visual.Online AddAssert("Last visible selected", () => currentChannel == visibleChannels.Last()); // Closing the last channel with dropdown no longer present - AddStep("Close last when selector next", () => + AddStep("Close last when selector next", () => { expectedChannel = previousChannel; chatOverlay.ChannelTabControl.RemoveChannel(currentChannel); @@ -198,7 +198,7 @@ namespace osu.Game.Tests.Visual.Online // Standard channel closing AddStep("Switch to previous channel", () => chatOverlay.ChannelTabControl.SwitchTab(-1)); - AddStep("Close current channel", () => + AddStep("Close current channel", () => { expectedChannel = nextChannel; chatOverlay.ChannelTabControl.RemoveChannel(currentChannel); From acd280c85552ec22073606807e75eeda663ebe11 Mon Sep 17 00:00:00 2001 From: TheWildTree Date: Sun, 15 Mar 2020 22:13:26 +0100 Subject: [PATCH 16/16] Add System channel type and use it for the ChannelSelectorTab --- osu.Game/Online/Chat/ChannelType.cs | 1 + osu.Game/Overlays/Chat/Tabs/ChannelSelectorTabItem.cs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) 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 5fb56a3f75..e3ede04edd 100644 --- a/osu.Game/Overlays/Chat/Tabs/ChannelSelectorTabItem.cs +++ b/osu.Game/Overlays/Chat/Tabs/ChannelSelectorTabItem.cs @@ -39,7 +39,7 @@ namespace osu.Game.Overlays.Chat.Tabs public ChannelSelectorTabChannel() { Name = "+"; - Type = ChannelType.Temporary; + Type = ChannelType.System; } } }