From 1ddd5cc41406749a6c292d6d70a37951fb54f282 Mon Sep 17 00:00:00 2001 From: andy840119 Date: Sat, 5 Feb 2022 16:15:08 +0800 Subject: [PATCH 1/8] remove layout index copy from utils. --- osu.Game.Rulesets.Karaoke/Utils/LyricsUtils.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/osu.Game.Rulesets.Karaoke/Utils/LyricsUtils.cs b/osu.Game.Rulesets.Karaoke/Utils/LyricsUtils.cs index c84e8ac09..26c9cd259 100644 --- a/osu.Game.Rulesets.Karaoke/Utils/LyricsUtils.cs +++ b/osu.Game.Rulesets.Karaoke/Utils/LyricsUtils.cs @@ -65,7 +65,6 @@ public static Tuple SplitLyric(Lyric lyric, int splitIndex) RomajiTags = lyric.RomajiTags?.Where(x => x.StartIndex < splitIndex && x.EndIndex <= splitIndex).ToArray(), // todo : should implement time and duration Singers = lyric.Singers, - LayoutIndex = lyric.LayoutIndex, Language = lyric.Language, }; @@ -78,7 +77,6 @@ public static Tuple SplitLyric(Lyric lyric, int splitIndex) RomajiTags = shiftingTextTag(lyric.RomajiTags?.Where(x => x.StartIndex >= splitIndex && x.EndIndex > splitIndex).ToArray(), secondLyricText, -splitIndex), // todo : should implement time and duration Singers = lyric.Singers, - LayoutIndex = lyric.LayoutIndex, Language = lyric.Language, }; @@ -127,7 +125,6 @@ public static Lyric CombineLyric(Lyric firstLyric, Lyric secondLyric) StartTime = startTime, Duration = endTime - startTime, Singers = singers.Distinct().ToArray(), - LayoutIndex = firstLyric.LayoutIndex, Language = language, }; } From c488927fc58e37bc02b6b76058af3b7ea1376f4a Mon Sep 17 00:00:00 2001 From: andy840119 Date: Sat, 5 Feb 2022 16:18:53 +0800 Subject: [PATCH 2/8] should not able to select layout directly in the lyric. might have better way for mapping skin element to the lyric or other object. --- .../Edit/KaraokeSelectionHandler.cs | 26 ------------------- 1 file changed, 26 deletions(-) diff --git a/osu.Game.Rulesets.Karaoke/Edit/KaraokeSelectionHandler.cs b/osu.Game.Rulesets.Karaoke/Edit/KaraokeSelectionHandler.cs index 0c939e6a5..b8a6fb088 100644 --- a/osu.Game.Rulesets.Karaoke/Edit/KaraokeSelectionHandler.cs +++ b/osu.Game.Rulesets.Karaoke/Edit/KaraokeSelectionHandler.cs @@ -13,8 +13,6 @@ using osu.Game.Rulesets.Karaoke.Edit.ChangeHandlers.Notes; using osu.Game.Rulesets.Karaoke.Edit.Components.ContextMenu; using osu.Game.Rulesets.Karaoke.Objects; -using osu.Game.Rulesets.Karaoke.Skinning; -using osu.Game.Rulesets.Karaoke.Skinning.Elements; using osu.Game.Rulesets.Karaoke.UI.Components; using osu.Game.Rulesets.Karaoke.UI.Position; using osu.Game.Rulesets.Karaoke.UI.Scrolling; @@ -54,7 +52,6 @@ protected override IEnumerable GetContextMenuItemsForSelection(IEnumer { return new[] { - createLayoutMenuItem(), createSingerMenuItem() }; } @@ -98,29 +95,6 @@ private MenuItem createCombineNoteMenuItem() }); } - private MenuItem createLayoutMenuItem() - { - var lyrics = EditorBeatmap.SelectedHitObjects.Cast(); - var layoutDictionary = source.GetConfig>(KaraokeIndexLookup.Layout)?.Value; - var selectedLayoutIndexes = lyrics.Select(x => x.LayoutIndex).Distinct().ToList(); - int selectedLayoutIndex = selectedLayoutIndexes.Count == 1 ? selectedLayoutIndexes.FirstOrDefault() : -1; - - return new OsuMenuItem("Layout") - { - Items = layoutDictionary?.Select(x => new TernaryStateToggleMenuItem(x.Value, selectedLayoutIndex == x.Key ? MenuItemType.Highlighted : MenuItemType.Standard, state => - { - if (state != TernaryState.True) - return; - - int layoutIndex = x.Key; - var layout = source.GetConfig(new KaraokeSkinLookup(ElementType.LyricLayout, layoutIndex)).Value; - - // todo: should use another way to change the layout. - // lyricLayoutChangeHandler.ChangeLayout(layout); - })).ToArray() - }; - } - private MenuItem createSingerMenuItem() { return new SingerContextMenu(beatmap, lyricSingerChangeHandler, "Singer"); From f9a061a8748464b86c5f9b65083201a94ecc370c Mon Sep 17 00:00:00 2001 From: andy840119 Date: Sat, 5 Feb 2022 16:19:29 +0800 Subject: [PATCH 3/8] layout selection should be highlight if edit the current layout, not highlight because of the lyric. --- .../Screens/Skin/Layout/LayoutSelection.cs | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/osu.Game.Rulesets.Karaoke/Screens/Skin/Layout/LayoutSelection.cs b/osu.Game.Rulesets.Karaoke/Screens/Skin/Layout/LayoutSelection.cs index ad5f9a580..3a18c9bc6 100644 --- a/osu.Game.Rulesets.Karaoke/Screens/Skin/Layout/LayoutSelection.cs +++ b/osu.Game.Rulesets.Karaoke/Screens/Skin/Layout/LayoutSelection.cs @@ -75,6 +75,7 @@ public class LayoutSelectionItem : FillFlowContainer { private const float selection_size = (240 - layout_setting_horizontal_padding * 2 - SECTION_SPACING) / 2; + // todo: should changed into selected layout index private readonly Bindable selectedLayoutIndex = new(); private readonly DrawableLayoutPreview drawableLayoutPreview; @@ -131,20 +132,8 @@ public Lyric Lyric if (drawableLayoutPreview.Lyric == value) return; - // unbind previous event - if (Lyric != null) - { - selectedLayoutIndex.UnbindFrom(Lyric.LayoutIndexBindable); - } - // update lyric drawableLayoutPreview.Lyric = value; - - // bind layout index. - if (Lyric != null) - { - selectedLayoutIndex.BindTo(Lyric.LayoutIndexBindable); - } } } From dc11ff01a9c59d17466f6ec72b866788c0f7039e Mon Sep 17 00:00:00 2001 From: andy840119 Date: Sat, 5 Feb 2022 16:20:56 +0800 Subject: [PATCH 4/8] should trigger the skin change instead if re-assign the layout id. --- .../Screens/Skin/Layout/LayoutPreview.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/osu.Game.Rulesets.Karaoke/Screens/Skin/Layout/LayoutPreview.cs b/osu.Game.Rulesets.Karaoke/Screens/Skin/Layout/LayoutPreview.cs index 294a1c1d4..39baae925 100644 --- a/osu.Game.Rulesets.Karaoke/Screens/Skin/Layout/LayoutPreview.cs +++ b/osu.Game.Rulesets.Karaoke/Screens/Skin/Layout/LayoutPreview.cs @@ -85,7 +85,9 @@ private void load(LayoutManager manager) manager.EditLayout.BindValueChanged(v => { if (Child is PreviewDrawableLyric lyric) - lyric.HitObject.LayoutIndex = v.NewValue.ID; + { + // todo: should trigger the skin change instead. + } }, true); } From 3e627582d5012fbb4a0af3d0e2dca2c2ae71c046 Mon Sep 17 00:00:00 2001 From: andy840119 Date: Sat, 5 Feb 2022 16:24:32 +0800 Subject: [PATCH 5/8] should not have continuous property. if really want this, should add property "continuous" in the lyric class instead of using layout index == -1 . and "continuous" feature(means lyric will be at the right side of the previous lyric) is not the really important. should not break the rules for this. --- .../Edit/Lyrics/Rows/EditLyricRow.cs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/osu.Game.Rulesets.Karaoke/Edit/Lyrics/Rows/EditLyricRow.cs b/osu.Game.Rulesets.Karaoke/Edit/Lyrics/Rows/EditLyricRow.cs index f3e6e3e3a..e74fe6512 100644 --- a/osu.Game.Rulesets.Karaoke/Edit/Lyrics/Rows/EditLyricRow.cs +++ b/osu.Game.Rulesets.Karaoke/Edit/Lyrics/Rows/EditLyricRow.cs @@ -33,7 +33,6 @@ namespace osu.Game.Rulesets.Karaoke.Edit.Lyrics.Rows public class EditLyricRow : LyricEditorRow { private const int min_height = 75; - private const int continuous_spacing = 20; public EditLyricRow(Lyric lyric) : base(lyric) @@ -43,16 +42,8 @@ public EditLyricRow(Lyric lyric) protected override Drawable CreateLyricInfo(Lyric lyric) { - // todo : need to refactor this part. - bool isContinuous = lyric.LayoutIndex == -1; - int continuousSpacing = isContinuous ? continuous_spacing : 0; - return new InfoControl(lyric) { - Margin = new MarginPadding - { - Left = continuousSpacing, - }, // todo : cannot use relative size to both because it will cause size cannot roll-back if make lyric smaller. RelativeSizeAxes = Axes.X, Height = min_height, From cf723b335a9386bd03d1bd5b12b7d952734a76f7 Mon Sep 17 00:00:00 2001 From: andy840119 Date: Sat, 5 Feb 2022 16:28:18 +0800 Subject: [PATCH 6/8] remove the test case for testing layout index. --- .../Utils/LyricsUtilsTest.cs | 33 ------------------- 1 file changed, 33 deletions(-) diff --git a/osu.Game.Rulesets.Karaoke.Tests/Utils/LyricsUtilsTest.cs b/osu.Game.Rulesets.Karaoke.Tests/Utils/LyricsUtilsTest.cs index 7708d8828..78260fd45 100644 --- a/osu.Game.Rulesets.Karaoke.Tests/Utils/LyricsUtilsTest.cs +++ b/osu.Game.Rulesets.Karaoke.Tests/Utils/LyricsUtilsTest.cs @@ -138,24 +138,6 @@ public void TestSeparateLyricSinger(int[] singerIndexes, int[] firstSingerIndexe Assert.AreNotSame(secondLyric.Singers, lyric.Singers); } - [TestCase(1, 1, 1)] - [TestCase(2, 2, 2)] - [TestCase(-5, -5, -5)] // copy layout index even it's wrong. - public void TestSeparateLyricLayoutIndex(int actualLayout, int firstLayout, int secondLayout) - { - const int split_index = 2; - var lyric = new Lyric - { - Text = "karaoke!", - LayoutIndex = actualLayout - }; - - var (firstLyric, secondLyric) = LyricsUtils.SplitLyric(lyric, split_index); - - Assert.AreEqual(firstLyric.LayoutIndex, firstLayout); - Assert.AreEqual(secondLyric.LayoutIndex, secondLayout); - } - [TestCase(1, 1, 1)] [TestCase(54, 54, 54)] [TestCase(null, null, null)] @@ -309,21 +291,6 @@ public void TestCombineLyricSinger(int[] firstSingerIndexes, int[] secondSingerI Assert.AreEqual(combineLyric.Singers, actualSingerIndexes); } - [TestCase(1, 2, 1)] - [TestCase(1, 3, 1)] - [TestCase(1, -1, 1)] - [TestCase(-1, 1, -1)] - [TestCase(-5, 1, -5)] // Wrong layout index - public void TestCombineLayoutIndex(int firstLayout, int secondLayout, int actualLayout) - { - var lyric1 = new Lyric { LayoutIndex = firstLayout }; - var lyric2 = new Lyric { LayoutIndex = secondLayout }; - - // just use first lyric's layout id. - var combineLyric = LyricsUtils.CombineLyric(lyric1, lyric2); - Assert.AreEqual(combineLyric.LayoutIndex, actualLayout); - } - [TestCase(1, 1, 1)] [TestCase(54, 54, 54)] [TestCase(null, 1, null)] From 1723e248611db106cc4a00c89bc7c1ffbba10c74 Mon Sep 17 00:00:00 2001 From: andy840119 Date: Sat, 5 Feb 2022 16:28:47 +0800 Subject: [PATCH 7/8] add a todo to fix the mapping for testing. --- .../Editor/TestSceneLayoutToolTip.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/osu.Game.Rulesets.Karaoke.Tests/Editor/TestSceneLayoutToolTip.cs b/osu.Game.Rulesets.Karaoke.Tests/Editor/TestSceneLayoutToolTip.cs index e241c6409..11d9d42bc 100644 --- a/osu.Game.Rulesets.Karaoke.Tests/Editor/TestSceneLayoutToolTip.cs +++ b/osu.Game.Rulesets.Karaoke.Tests/Editor/TestSceneLayoutToolTip.cs @@ -43,7 +43,10 @@ public void TestDisplayToolTip() foreach ((int key, string value) in layouts) { - setTooltip($"Test lyric with layout {value}", lyric => { lyric.LayoutIndex = key; }); + setTooltip($"Test lyric with layout {value}", lyric => + { + // todo: should change mapping group id from the lyric. + }); } } From 84a16cda8a8c7862b58bcd866328e042928a0a48 Mon Sep 17 00:00:00 2001 From: andy840119 Date: Sat, 5 Feb 2022 16:29:14 +0800 Subject: [PATCH 8/8] guess finally it's ok to remove this property. --- osu.Game.Rulesets.Karaoke/Objects/Lyric.cs | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/osu.Game.Rulesets.Karaoke/Objects/Lyric.cs b/osu.Game.Rulesets.Karaoke/Objects/Lyric.cs index 8c4f8b7db..02a7d3bd7 100644 --- a/osu.Game.Rulesets.Karaoke/Objects/Lyric.cs +++ b/osu.Game.Rulesets.Karaoke/Objects/Lyric.cs @@ -132,19 +132,6 @@ public IList Singers } } - [JsonIgnore] - public readonly Bindable LayoutIndexBindable = new(); - - /// - /// Layout index - /// - [JsonIgnore] - public int LayoutIndex - { - get => LayoutIndexBindable.Value; - set => LayoutIndexBindable.Value = value; - } - [JsonIgnore] public readonly BindableDictionary TranslateTextBindable = new();