Remove call to Clip::changeLength

[michaelgregorius]

Remove the call to Clip::changeLength from TrackContentWidget::changePosition as it results in bad performance, especially when the Song Editor is in auto-scroll mode and the position is changed very often.

The call in question has set the clip to the length that it already has which in turn resulted in lots of needless calls to Song::updateLength.

It also should not be necessary to update a clip length if all that we do is change the position of the TrackContentWidget.

[regulus79]

Do we have any idea as to why Clip::changeLength was put there in the first place? I agree that simply changing the position should not change the length, but are we missing something that the original author of this function was thinking when they wrote this?

[michaelgregorius]

@regulus79, it's not really possible anymore. The line is already there since 19 years. Following the blame brings this line (17 years old):

https://github.com/LMMS/lmms/blame/ddf69feebc2ee6754b897f61ae3bc9101e0df45c/src/core/Track.cpp#L1668

Which is preceded by this line (19 years old):

https://github.com/LMMS/lmms/blame/40017887e9afb4e369522923bcd5c549742353e9/src/core/track.cpp#L1317

Which in turn is preceded by this line (19 years old):

https://github.com/LMMS/lmms/blame/f700c16c339a348664c04e16b56629e588880f0e/src/core/track.cpp#L812

All these commit have originally been made in the SVN repository and have been imported into git. So they are really old. 😅

So, I'd propose to remove the line and if problems should show up to fix them going forward with better code than the removed line. See this comment why it's bad style: #7462 (comment)

Edit: Added the proposal.

[khoidauminh]

I tested the pull request and this is what happens when you zoom using the slider. The the out of bounds clips seem to glitch into view and I can interact with them.

Screencast.From.2024-11-05.14-07-59.mp4

Screencast.From.2024-11-05.14-22-44.mp4

The glitch is not visible when zooming using the mouse wheel, however.

[michaelgregorius]

Good catch, @khoidauminh! I can reproduce the issue.

I assume that it is somehow caused by this code in TrackContentWidget::changePosition which adjust the positions of the clips in the track:

lmms/src/gui/tracks/TrackContentWidget.cpp

Lines 269 to 291 in 07baf9e

	for (const auto& clipView : m_clipViews)
	{
	Clip* clip = clipView->getClip();
	clip->changeLength( clip->length() );
	const int ts = clip->startPosition();
	const int te = clip->endPosition()-3;
	if( ( ts >= begin && ts <= end ) ||
	( te >= begin && te <= end ) ||
	( ts <= begin && te >= end ) )
	{
	clipView->move(static_cast<int>((ts - begin) * ppb / TimePos::ticksPerBar()), clipView->y());
	if (!clipView->isVisible())
	{
	clipView->show();
	}
	}
	else
	{
	clipView->move(-clipView->width() - 10, clipView->y());
	}
	}

Line 273 is removed with this PR and it seems to have some surprising side effects on the clip start and end positions which affect the code that follows then.