Fix empty MIDI clips not displaying name

[TimovVeen]

Beat clips don't render titles. checkType() always sets type to beat when there are no notes, so titles don't render on empty MIDI clips either.
I changed the default type of MidiClips on construction to MelodyClip, otherwise the name would still not show until you start adding notes. I tested to make sure this doesn't cause the opposite issue for beat clips. If you name a new beat clip before adding any notes, it will still correctly not show the name title.

Fixes #7808

[sakertooth]

Why exactly do beat clips not render titles?

[TimovVeen]

My guess is because it would obscure the clip, which is not a problem for melody clips since you edit them by opening the piano roll.

[TimovVeen]

I should add that beat clips not rendering titles is explicitly defined in MidiClipView. So at least it's not a bug, though afaik it's not documented what the reasoning is.

[sakertooth]

I find it a bit strange. Why can't a beat clip (a MIDI clip) show titles? All clips are meant to have titles, hence their name parameter. Many of the clips also have functions to change the title, like MidiClipView::changeName, which I would expect would change the title of the MIDI clip, whether its a beat clip or not.

Maybe it might be worth investigating this sometime later, but I suppose it is fine for now if for some reason this was the intention. @bratpeki can you test this PR to see if it fixes the issue you reported?

[bratpeki]

Hi, sorry, been busy! I can checks, yes.

[bratpeki]

Code-wise question, what the **** is a MelodyClip? 🤣

[bratpeki]

This seems to have broken BB clips:

---

Furthermore, empty MIDI clips are no longer grey:

[TimovVeen]

I reverted the change that causes this, thank you for testing. I missed it because it doesn't happen to the kicker plugin for some reason. The thing is, I made the change because empty midi clips being grey is also a bug. It happens because new clips are beat clips by default, until a note gets added and the type is checked. This can be checked by trying to name a newly created clip, the name will not render. This doesn't happen with other clips.

[bratpeki]

I'd argue the gray clips are a nifty functionality, helping us differentiate between empty clips and clips with content.

---

...thank you for testing.

My pleasure! Thank you for writing the PR. We'll get to the bottom of this, for sure.

The thing is, I made the change because empty midi clips being grey is also a bug. It happens because new clips are beat clips by default, until a note gets added and the type is checked.

Very odd. Why do they turn gray after deleting all notes then? Do they turn back into BeatClips?

Code-wise question, what the **** is a MelodyClip? 🤣

Still curious about this! 🤔

[bratpeki]

@sakertooth:

Why exactly do beat clips not render titles?

My question as well. Does this mean the fix is actually moving that logic into the parent Clip class, or something like that?

[TimovVeen]

Code-wise question, what the **** is a MelodyClip? 🤣

The clips in the song editor are "melody" clips, the ones you edit with the piano roll.
The clips in the pattern editor are "beat" clips (see image).

In the code both types use MidiClip classes for rendering and logic, so to differentiate between them, there is an enum type that is either BeatClip or MelodyClip. MidiClips have default type BeatClip on construction, and the type is checked and updated with the following method

void MidiClip::checkType()
{
	// If all notes are StepNotes, we have a BeatClip
	const auto beatClip = std::all_of(m_notes.begin(), m_notes.end(), [](auto note) { return note->type() == Note::Type::Step; });

	setType(beatClip ? Type::BeatClip : Type::MelodyClip);
}

The issue with this method is that if there are no notes to differentiate the type, it is still assumed to be a BeatClip.

At some point someone decided that beat clips should not show their name. (See image for how it would look)

However, since checkType() sets melody clips to type BeatClip when there are no notes, it also doesn't show the name and is colored gray, causing your issue. The change I made to checkType() fixes this, but since newly created melody clips have type BeatClip on construction, they show up gray with no name rendered, until the first note is added and the type is updated.

I'd argue the gray clips are a nifty functionality, helping us differentiate between empty clips and clips with content.

Yes I agree it should stay as a feature, but right now it's unintended behavior caused by the tight code coupling of melody and beat clips. As you mentioned, it happens because they turn into beat clips.
A better solution would be to set the correct clip type on creation, so you don't have to rely on continuously checking the type of the notes. But that's probably out of scope for this PR.

I hope this explains it a bit better, let me know if anything is unclear

[bratpeki]

I see, thank you! Sorry for not getting back to you, I'm a bit backlogged. Will write soon, I hope.

[regulus79]

My understanding is that currently:

• New MidiClips are always initialized as "BeatClips", whether they are in the Song Editor or the Pattern Editor.
• If a user starts adding normal green notes into the clip, it will automatically change itself to be a "MelodyClip", which appear with a green background.
• If instead the user specifically fills the clip only with the red beat/step notes, it will remain a "BeatClip".
• If the user deletes all the notes from the clip, it will return to being a "BeatClip" when empty.

This system means that the user can have "BeatClip"s and "MelodyClip"s exist within either the song editor or pattern editor, and can change between them by deciding whether or not to place green notes in them. I know some users appreciate this in the "legacysebb" option where they can program drums in the song editor.

My understanding is that this PR:

• Still initializes new MidiClips as grey "BeatClips" in the song editor.
• Likewise, if the user starts placing green notes, it will turn into a green "MelodyClip".
• BUT, if the user then removes all of those green notes, the empty clip will remain a "MelodyClip", not turn back into a grey "BeatClip".

I noticed an issue with this, since it means if you open a MidiClip in the pattern editor and add some notes it becomes a "MelodyClip". But if you then delete those notes, the clip does not return to being a "BeatClip", so you can't add beat/step notes like before.

A better solution would be to set the correct clip type on creation, so you don't have to rely on continuously checking the type of the notes.

I would agree, I think it would make more sense for clips to default to being "MelodyClips" when in the song editor, and "BeatClips" when in the pattern editor. The only concern would be that users who like legacysebb may be affected. (Also, perhaps some users like empty clips being differentiated by their dark color. But I don't know how serious of an issue that is.)

One second; I may be able to make some code which gives the best of both worlds.

[regulus79]

I'm not sure I can add suggestions to lines not touched by the PR from the github website, but here is a diff against master:

diff --git a/src/tracks/MidiClip.cpp b/src/tracks/MidiClip.cpp
index 73a8435b2..5c6ed948d 100644
--- a/src/tracks/MidiClip.cpp
+++ b/src/tracks/MidiClip.cpp
@@ -28,6 +28,7 @@
 #include <algorithm>
 #include <QDomElement>
 
+#include "ConfigManager.h"
 #include "GuiApplication.h"
 #include "InstrumentTrack.h"
 #include "MidiClipView.h"
@@ -42,13 +43,18 @@ namespace lmms
 MidiClip::MidiClip( InstrumentTrack * _instrument_track ) :
        Clip( _instrument_track ),
        m_instrumentTrack( _instrument_track ),
-       m_clipType( Type::BeatClip ),
+       m_clipType(m_instrumentTrack->trackContainer()  == Engine::patternStore() ? 
Type::BeatClip : Type::MelodyClip),
        m_steps( TimePos::stepsPerBar() )
 {
        if (_instrument_track->trackContainer() == Engine::patternStore())
        {
                resizeToFirstTrack();
        }
+       // If users have legacysebb enabled, that means they wish MidiClips to defau
lt to BeatClips, even in the song editor.
+       if (ConfigManager::inst()->value("ui", "legacysebb", "0").toInt())
+       {
+               setType(Type::BeatClip);
+       }
        init();
 }
 
@@ -416,10 +422,22 @@ void MidiClip::setType( Type _new_clip_type )
 
 void MidiClip::checkType()
 {
-       // If all notes are StepNotes, we have a BeatClip
-       const auto beatClip = std::all_of(m_notes.begin(), m_notes.end(), [](auto no
te) { return note->type() == Note::Type::Step; });
+       const auto onlyStepNotes = std::all_of(m_notes.begin(), m_notes.end(), [](au
to note) { return note->type() == Note::Type::Step; });
 
-       setType(beatClip ? Type::BeatClip : Type::MelodyClip);
+       // For midi clips in the pattern editor, default to a BeatClip unless there 
are non-step notes in the clip
+       // This allows users to open clips in the piano roll and add green notes to 
make it a MelodyClip, but also
+       // allows them to remove all those notes and have it return to being a BeatC
lip.
+       if (m_instrumentTrack->trackContainer() == Engine::patternStore())
+       {
+               setType(onlyStepNotes ? Type::BeatClip : Type::MelodyClip);
+       }
+       else if (!m_notes.empty())
+       {
+               // For the song editor, the type should default to a MelodyClip, exc
ept for users who
+               // have legacysebb enabled, where empty clips default to BeatClips.
+               // For the sake of those users, only update the clip type if there a
re notes within it
+               setType(onlyStepNotes ? Type::BeatClip : Type::MelodyClip);
+       }
 }
 
 

Essentially, MidiClips now default to "MelodyClips" when created in the song editor, and "BeatClips" when created in the pattern editor, except for users who have legacysebb enabled. When green notes are added to a clip, it will become a "MelodyClip", and if the clip is inside the pattern editor, when those notes are removed, it will return to being a "BeatClip". For the sake of people who use legacysebb, any "BeatClips" within the song editor will only be permanently converted to "MelodyClips" if the user adds green notes to them.

[bratpeki]

Okay, so I've been investigating this today. Here's what I've got.

1: The initial issue was that I didn't know empty MIDI clips are actually BeatClips. Thanks for figuring that out!

2: The issue with the original implementation is that it invalidates legacysebb, which some users use. If a clip is a MelodyClip by default, then that feature is gone.

2.1: Also, if that change is made, BB clips break. #7836 (comment)

3: Your proposed fix makes all newly created MIDI clips BeatClips, but then turns them into MelodyClips when a note is added. This means a note must be added first for the name to be displayed.

---

I found a very simple fix!

diff --git a/src/gui/clips/MidiClipView.cpp b/src/gui/clips/MidiClipView.cpp
index 17604eb19..2dc727b6f 100644
--- a/src/gui/clips/MidiClipView.cpp
+++ b/src/gui/clips/MidiClipView.cpp
@@ -602,8 +602,7 @@ void MidiClipView::paintEvent( QPaintEvent * )

        // Check whether we will paint a text box and compute its potential height
        // This is needed so we can paint the notes underneath it.
-       bool const drawName = !m_clip->name().isEmpty();
-       bool const drawTextBox = !beatClip && drawName;
+       bool const drawTextBox = !m_clip->name().isEmpty();

        // TODO Warning! This might cause problems if ClipView::paintTextLabel changes
        int textBoxHeight = 0;

@regulus79 would we be okay with a merge like this?

Here's the end result: