Fixes createTCO method on some classes #5699

IanCaio · 2020-10-04T16:21:00Z

Classes that inherit from Track, also inherit the createTCO method. That method takes a MidiTime position as an argument, but except on the class SampleTrack that argument was ignored. That lead to unnecessary calls to TCO->movePosition() after creating a TCO in many parts of the code base (making the argument completely redundant) and even to a bug on the BBEditor, caused by a call to createTCO that expected the position to be set on the constructor (see issue #5673 ).

That PR adds code to move the TCO to the appropriate position inside the constructor of the classes that didn't have it, fixes the code style on the SampleTrack createTCO method and removes the now unneeded calls to movePosition from source files on src/ and plugins/. On Track::loadSettings there was a call to saveJournallingState(false) followed immediately by restoreJournallingState() which was deleted because it's redundant (probably a left over from copying the code from the pasteSelection method?).

Classes that inherit from "Track", also inherit the createTCO method. That method takes a MidiTime position as an argument, but except on the class SampleTrack that argument was ignored. That lead to unnecessary calls to TCO->movePosition after creating a TCO in many parts of the codebase (making the argument completely redundant) and even to a bug on the BBEditor, caused by a call to createTCO that expected the position to be set on the constructor (see issue LMMS#5673). That PR adds code to move the TCO to the appropriate position inside the constructor of the classes that didn't have it, fixes the code style on the SampleTrack createTCO method and removes the now unneeded calls to movePosition from source files on src/ and plugins/. On Track::loadSettings there was a call to saveJournallingState(false) followed immediately by restoreJournallingState() which was deleted because it's redundant (probably a left over from copying the code from pasteSelection?).

LmmsBot · 2020-10-04T16:27:13Z

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

Linux (AppImage): lmms-1.2.3-763%2Bge8be73a-linux-x86_64.AppImage (build link)

Windows

Windows 32-bit: lmms-1.2.3-763%2Bge8be73aea-mingw-win32.exe (build link)
Windows 64-bit: lmms-1.2.3-763%2Bge8be73aea-mingw-win64.exe (build link)
Windows 32-bit: lmms-1.3.0-alpha-msvc2017-win32.exe (build link)
Windows 64-bit: lmms-1.3.0-alpha-msvc2017-win64.exe (build link)

macOS

macOS : lmms-1.2.3-763%2Bge8be73aea-mac10.13.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://10215-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-763%2Bge8be73a-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10215?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://10219-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-763%2Bge8be73aea-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10219?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://10216-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-763%2Bge8be73aea-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10216?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/p3bd8t271fmkw3e4/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36182039"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/185itjdtjduvm908/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36182039"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://10217-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-763%2Bge8be73aea-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10217?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "b9fe497a74c898c678f2cf7e6d9b8b7d1d098fcd"}

This commit reorganizes the code a little bit to make it better suited for review. The code that checked whether a selection can be merged was extracted to a separate method called TrackContentObjectView::canMergeSelection(). Two conditionals inside that code were merged into a single one, since they both resulted in the same action. Also updated a comment next to a line of code that is probably going to be removed after the PR LMMS#5699 is merged (it's currently necessary because of a bug that is fixed on that PR).

src/tracks/SampleTrack.cpp

Spekular · 2020-10-06T18:52:58Z

Code LGTM, one minor formatting concern. Do you have any test cases in mind for testers?

IanCaio · 2020-10-06T20:27:26Z

As for testing cases, I'd say all operations that involves creating TCOs, but more specifically the ones which causes calls to createTCO. From grepping the source some I could find:

Drag & Dropping Sample Tracks
Adding a sample using Shift+Enter on the File Browser
Copy and Paste (and Drag & Drop) track selections
Adding TCOs on the song editor
Drag & Dropping tracks with TCOs to the BB Editor
And particularly the bug described on my comment in Beat and bassline bug #5673 , to see if it is fixed.

Fixes code style issues on some files (except for ones where the current statements already had a different code style. In those the used code style was kept for consistency).

src/tracks/InstrumentTrack.cpp

src/tracks/AutomationTrack.cpp

Fixes code style issue on the parameter name of createTCO, where _pos was supposed to be just pos. The existing code had the old style and I ended up replicating it on the other methods.

Resolves merge conflict introduced by latest master commit.

Fixes code style in the changed lines.

IanCaio · 2020-10-24T00:18:06Z

Fixed the merge conflict and the code style on the changed lines of the code.

ryuukumar · 2020-10-25T05:02:40Z

I'll run some tests later in the day. I'll edit this comment with the results.

Testing Results

I tested based on the guidelines you provided for testing here.

Here's what I found:

Dragging & dropping sample tracks works perfectly. The sample is retained along with its properties.
Shift + Enter from the file menu cause a crash the first time (I was unable to catch the reason) but worked after that, may have been a problem on my side.
Copy & pasting selections with multiple TCOs may cause some TCOs to be place in a negative position (i.e. before the 0th bar), and although they won't be played if not visible, I think setting a check for this while pasting is the way to go.
- Note that after attempting to move that pasted block with the TCO in the negative position, it does snap back into playable space.
- Here's a GIF I recorded. Although I performed a drag & drop, the same is observed with copy & paste by right-click.

Adding TCOs works in the song editor without any issues.
Dragging & dropping tracks into BB tracks...
- preserves the TCOs when the first bar of the track has a TCO.
- does not preserve the TCOs when the track has multiple TCOs. (This may be intended behaviour though)
  - An error I found corresponding to attempting this operation is called Track::getTCO( 1 ), but TCO 1 doesn't exist
The bug in Beat and bassline bug #5673 seems to be fixed as well.

I think these are just some quick fixes, and unless something else pops up, this PR will probably be done soon.

IanCaio · 2020-10-26T10:59:17Z

Thanks for the detailed testing man!

I'm not sure about that Shift+Enter crash you experienced, couldn't replicate it here. But I'd say it's very likely unrelated. This code only changes the createTCO method and updates places that call it.

As for the dragging selection behavior, this was present before this bugfix. I looked briefly at the code and I think it could be fixed by adding tco->positionChanged() in the end of the TrackContentWidget::pasteSelection() method so it can update the TCOView. I'd have to double check and test it though.

The called Track::getTCO( 1 ), but TCO 1 doesn't exist is a warning triggered by Track::getTCO when it's given a TCO number where there aren't any. In that case it creates one anyways, I'm not sure this warning was previously used for debugging but it has been there for a while. I think if the first TCO has notes they should be kept on the first BBTrack though, even if there are other TCOs after it. This wasn't the behavior there?

superpaik · 2020-10-26T11:20:51Z

Some issues:

when "Ctrl+LMB" over a TCO one expects to drag the selected TCO to where the mouse pointer is, but sometimes it goes to another point. It happens with all type of TCO but with samples track TCO happens always, with automation and instruments seems more random
I don't know if it is related to this PR. You LMB + crtl a TCO ("Edit mode" becomes active), if you let go "Ctrl" before LMB "Draw mode" returns to active. But If you let go LMB before "Crtl", "Edit mode" keeps active.
When you drag&drop a track from the BB Editor to Song Editor, the created TCO is gray (like it's empty). If you add a note (from the piano roll) it becomes green and all notes are shown. I think this behaviour is already present in 1.2.2, but maybe it's realated to the modified code.
I had to tried the mingw version, because MSVC version crashes a lot ¿?

Spekular · 2020-10-26T11:31:25Z

The MSVC issue is unrelated to this PR, see #5683.

IanCaio · 2020-10-26T11:41:56Z

@superpaik Thanks for testing!

First issue is actually an intended behavior of the drag&drop operation, you can't drag&drop a TCO on itself, so it snaps it to the closest neighbor position. Dragging to any other position should work as intended though. Second issue (with LMB+Ctrl) is probably related to the SongEditor handling of mouse and keyboard events, would have to check the code to be sure. But it's unrelated to this PR.

The third one, is a bit trickier to explain. Notes on the BBEditor are different from the notes we add at the piano roll. They have a "negative" length if I'm not mistaken. It's a way of LMMS knowing that it should play the sample until it ends (instead of cutting it when the note ends). So when you drag a track from the BBEditor back to the SongEditor, a grey clip shows up because the paint event of the clip probably doesn't handle those special notes. They are there, but you can't see them on the song editor (if you open the piano roll they will show as very tiny notes). I don't know if there are plans to moving towards another way of representing those special notes, but for now it can get a bit confusing to the user at first when we start mixing BBEditor notes with SongEditor ones.

ryuukumar · 2020-10-26T12:12:31Z

Here's what I meant by my BB finding:

Please note, however:

The getTCO( 1 ) error occurs only the first time you try to drag over a (certain) track with multiple TCOs
Both the error and the TCOs not showing up is observed in master too, so it is either intended behaviour, or something you may want to fix (probably the former, as you have pointed out too). Either ways, it's not an issue caused by this PR, so you can ignore it if you want to.

IanCaio · 2020-10-26T15:38:25Z

@russiankumar I read the code a little further, and it seems to be purposeful behavior as seen on src/gui/editor/BBEditor.cpp line 256:

		// Ensure BB TCOs exist
		bool hasValidBBTCOs = false;
		if (t->getTCOs().size() == m_bbtc->numOfBBs())
		{
			hasValidBBTCOs = true;
			for (int i = 0; i < t->getTCOs().size(); ++i)
			{
				if (t->getTCOs()[i]->startPosition() != MidiTime(i, 0))
				{
					hasValidBBTCOs = false;
					break;
				}
			}
		}
		if (!hasValidBBTCOs)
		{
			t->deleteTCOs();
			t->createTCOsForBB(m_bbtc->numOfBBs() - 1);
		}

The TCOs from a track are only kept if they all fit perfectly within the BBTrack layout of TCOs. That means that the number of TCOs has to match the number of BBTracks and they must be perfectly lined. When you have a second TCO, because you only have one BBTrack it considers the last TCO to be a deal breaker and just removes both. If you have two BBTracks on the other hand (and make sure you have both TCOs aligned in the position necessary for them to fit the BB layout) this drag and drop operation should keep them.

ryuukumar · 2020-10-26T17:07:49Z

Alright, since it is intended, I think we should leave it as is. Didn't even know this was a thing before xD. All that's left is to add the check that you mentioned you would add for the negative position bug, and this is good to go!

src/core/Track.cpp

superpaik · 2020-10-27T09:00:52Z

The MSVC issue is unrelated to this PR, see #5683.

Ok. I tend to use the MSVC version because of the size of the .exe (mingw exe is three times bigger than MSVC). I will take it into account for future tests. Thanks.

There was a bug (already present before this PR) where dragging a selection before MidiTime 0 would result in some TCOs being placed on negative positions. This PR fixes this bug by applying the following changes: 1) TrackContentObject::movePosition now moves the TCO to positions equal or above 0 only. 2) Because of the previous change, I removed the line that calculated the max value between 0 and the new position on TrackContentObjectView::mouseMoveEvent when dragging a single TCO (and added a line updating the value to the real new position of the TCO so the label displays the position correctly). 3) Unrelated to this bug, but removed an unnecessary call to TrackContentWidget::changePosition on the left resize of sample TCOs because it will already be called when movePosition triggers the positionChanged signal. 4) Added some logic to the TrackContentWidget::pasteSelection method to find the left most TCO being pasted and make sure that the offset is corrected so it doesn't end up on a negative position (similar to the logic for the MoveSelection action). 5) Removed another line that calculated the max between 0 and the new position on Track::removeBar since it's now safe to call movePosition with negative values.

Spekular

Recent changes look good, except for one that I don't really understand what it's for. One if statement that can be a max() call instead.

src/core/Track.cpp

src/tracks/BBTrack.cpp

Spekular · 2020-11-06T21:37:52Z

Tested this because I was a bit concerned that it might block individual clips rather than the group (i.e. lose offsets if you move a selection left after it hits the start), but both pasting and dragging still seem to respect offsets. Left a new code review as well.

As suggested by Spekular, we use std::max instead of a conditional statement to correct the value of offset if it positions a TCO on a negative position.

ryuukumar · 2020-11-07T12:42:59Z

I realise that I'm already late 😅 but I've done a test, the functional part of it works.

Here, a very useful GIF

I've already approved the code. LGTM to me after you settle the review Spekular left.

You can close #5768 when this is merged, or I can do it myself.

After the bug fix from LMMS#5699, there's no need to call movePosition on a TCO that was created with a position on its constructor. The line that called movePosition was removed. Code style was also fixed.

* Fixes createTCO method on some classes Classes that inherit from "Track", also inherit the createTCO method. That method takes a MidiTime position as an argument, but except on the class SampleTrack that argument was ignored. That lead to unnecessary calls to TCO->movePosition after creating a TCO in many parts of the codebase (making the argument completely redundant) and even to a bug on the BBEditor, caused by a call to createTCO that expected the position to be set on the constructor (see issue LMMS#5673). That PR adds code to move the TCO to the appropriate position inside the constructor of the classes that didn't have it, fixes the code style on the SampleTrack createTCO method and removes the now unneeded calls to movePosition from source files on src/ and plugins/. On Track::loadSettings there was a call to saveJournallingState(false) followed immediately by restoreJournallingState() which was deleted because it's redundant (probably a left over from copying the code from pasteSelection?). * Fix code style issues Fixes code style issues on some files (except for ones where the current statements already had a different code style. In those the used code style was kept for consistency). * Fixes more code style issues * Fixes code style issues on the parameter name Fixes code style issue on the parameter name of createTCO, where _pos was supposed to be just pos. The existing code had the old style and I ended up replicating it on the other methods. * Code style fixes Fixes code style in the changed lines. * Fixes bug with dragging to negative positions There was a bug (already present before this PR) where dragging a selection before MidiTime 0 would result in some TCOs being placed on negative positions. This PR fixes this bug by applying the following changes: 1) TrackContentObject::movePosition now moves the TCO to positions equal or above 0 only. 2) Because of the previous change, I removed the line that calculated the max value between 0 and the new position on TrackContentObjectView::mouseMoveEvent when dragging a single TCO (and added a line updating the value to the real new position of the TCO so the label displays the position correctly). 3) Unrelated to this bug, but removed an unnecessary call to TrackContentWidget::changePosition on the left resize of sample TCOs because it will already be called when movePosition triggers the positionChanged signal. 4) Added some logic to the TrackContentWidget::pasteSelection method to find the left most TCO being pasted and make sure that the offset is corrected so it doesn't end up on a negative position (similar to the logic for the MoveSelection action). 5) Removed another line that calculated the max between 0 and the new position on Track::removeBar since it's now safe to call movePosition with negative values. * Uses std::max instead of a conditional statement As suggested by Spekular, we use std::max instead of a conditional statement to correct the value of offset if it positions a TCO on a negative position.

IanCaio mentioned this pull request Oct 4, 2020

Beat and bassline bug #5673

Closed

IanCaio mentioned this pull request Oct 5, 2020

Adds feature to merge Instrument Track patterns #5700

Merged

Spekular reviewed Oct 6, 2020

View reviewed changes

src/tracks/SampleTrack.cpp Outdated Show resolved Hide resolved

IanCaio added 2 commits October 6, 2020 17:43

Merge branch 'master' into bugfix/createTCOBug

561abb0

Fix code style issues

f37114b

Fixes code style issues on some files (except for ones where the current statements already had a different code style. In those the used code style was kept for consistency).

Spekular reviewed Oct 6, 2020

View reviewed changes

src/tracks/InstrumentTrack.cpp Outdated Show resolved Hide resolved

Fixes more code style issues

180a714

PhysSong reviewed Oct 12, 2020

View reviewed changes

src/tracks/AutomationTrack.cpp Outdated Show resolved Hide resolved

IanCaio added 4 commits October 12, 2020 10:12

Merge branch 'master' into bugfix/createTCOBug

13b34fb

Fixes code style issues on the parameter name

5841237

Fixes code style issue on the parameter name of createTCO, where _pos was supposed to be just pos. The existing code had the old style and I ended up replicating it on the other methods.

Merge branch 'master' into bugfix/createTCOBug

1ab05d6

Resolves merge conflict introduced by latest master commit.

Code style fixes

099f500

Fixes code style in the changed lines.

ryuukumar reviewed Oct 26, 2020

View reviewed changes

src/core/Track.cpp Show resolved Hide resolved

src/core/Track.cpp Show resolved Hide resolved

Merge branch 'master' into bugfix/createTCOBug

14ab674

ryuukumar approved these changes Nov 5, 2020

View reviewed changes

Spekular reviewed Nov 6, 2020

View reviewed changes

src/core/Track.cpp Outdated Show resolved Hide resolved

src/tracks/BBTrack.cpp Show resolved Hide resolved

Uses std::max instead of a conditional statement

b9fe497

As suggested by Spekular, we use std::max instead of a conditional statement to correct the value of offset if it positions a TCO on a negative position.

ryuukumar mentioned this pull request Nov 7, 2020

Drag&Drop or Pasting multiple TCOs in Song Editor allows negative positions #5768

Closed

Spekular approved these changes Nov 7, 2020

View reviewed changes

Spekular merged commit e2bb606 into LMMS:master Nov 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes createTCO method on some classes #5699

Fixes createTCO method on some classes #5699

IanCaio commented Oct 4, 2020

LmmsBot commented Oct 4, 2020 •

edited

Loading

Linux

Windows

macOS

Spekular commented Oct 6, 2020

IanCaio commented Oct 6, 2020

IanCaio commented Oct 24, 2020

ryuukumar commented Oct 25, 2020 •

edited

Loading

IanCaio commented Oct 26, 2020

superpaik commented Oct 26, 2020

Spekular commented Oct 26, 2020

IanCaio commented Oct 26, 2020

ryuukumar commented Oct 26, 2020 •

edited

Loading

IanCaio commented Oct 26, 2020

ryuukumar commented Oct 26, 2020 •

edited

Loading

superpaik commented Oct 27, 2020

Spekular left a comment

Spekular commented Nov 6, 2020

ryuukumar commented Nov 7, 2020

Fixes createTCO method on some classes #5699

Fixes createTCO method on some classes #5699

Conversation

IanCaio commented Oct 4, 2020

LmmsBot commented Oct 4, 2020 • edited Loading

Linux

Windows

macOS

Spekular commented Oct 6, 2020

IanCaio commented Oct 6, 2020

IanCaio commented Oct 24, 2020

ryuukumar commented Oct 25, 2020 • edited Loading

Testing Results

IanCaio commented Oct 26, 2020

superpaik commented Oct 26, 2020

Spekular commented Oct 26, 2020

IanCaio commented Oct 26, 2020

ryuukumar commented Oct 26, 2020 • edited Loading

IanCaio commented Oct 26, 2020

ryuukumar commented Oct 26, 2020 • edited Loading

superpaik commented Oct 27, 2020

Spekular left a comment

Choose a reason for hiding this comment

Spekular commented Nov 6, 2020

ryuukumar commented Nov 7, 2020

LmmsBot commented Oct 4, 2020 •

edited

Loading

ryuukumar commented Oct 25, 2020 •

edited

Loading

ryuukumar commented Oct 26, 2020 •

edited

Loading

ryuukumar commented Oct 26, 2020 •

edited

Loading