Rename BB to Pattern #6284

allejok96 · 2022-01-15T21:46:59Z

Here goes nothing (updated Jan 22)

"Beat+Bassline" -> "Pattern"
BB* -> Pattern*
BBTrackContainer -> PatternStore
Explain what PatternStore is with some ASCII art

Notable other changes:

A bit of sane code formatting, including a few static_cast
README: "Pattern-Editor for creating beats and patterns"
PatternEditor.cpp line 89: I took the liberty of changing the tooltip "Add Beat/Bassline" to "New pattern".
Removed redundant PatternEditor::removePatternView().
PatternStoreView::removePatternView()-> removeViewsForPattern()

TODO:

DataFile.cpp ;)
wiki

For an easier view of the diff: git diff -M40 --color-words='\w'

Beat+Bassline Editor -> Pattern Editor BB* -> Pattern* BeatClip -> PatternClip Does not touch DataFile

LmmsBot · 2022-01-15T21:58:04Z

🤖 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.3.0-alpha.1.179%2Bg20ec55ad0-linux-x86_64.AppImage (build link)

Windows

Windows 32-bit: lmms-1.3.0-alpha.1.179%2Bg20ec55ad0-mingw-win32.exe (build link)
Windows 64-bit: lmms-1.3.0-alpha.1.179%2Bg20ec55ad0-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.3.0-alpha.1.179%2Bg20ec55ad0-mac10.14.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://15698-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.179%2Bg20ec55ad0-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/15698?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://15699-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.179%2Bg20ec55ad0-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/15699?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://15702-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.179%2Bg20ec55ad0-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/15702?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/7ppgsh4ioa39hoh6/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/42550563"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/qci0abddwjrhfawj/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/42550563"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://15701-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.179%2Bg20ec55ad0-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/15701?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "7864f9a22cf1597c05b2efd47057e80194ad67e2"}

include/MidiClip.h

src/core/PatternClip.cpp

allejok96 · 2022-01-16T14:14:48Z

Is there a better name we could give to BBTrackContainer aka PatternTrackContainer ?

It's confusing since it can never contain a PatternTrack. And it's even worse how the GUI makes it seem like there's a PatternTrackContainer inside each PatternTrack.

If possible the name should indicate that there is only one PatternTrackContainer in a project, the same way there is only one Song (which is also a form of TrackContainer).

PatternEditorTrackContainer is way too long and boring :)

M374LX · 2022-01-16T15:44:26Z

@allejok96 Why not PatternContainer?

JohannesLorenz · 2022-01-16T15:56:41Z

Or just Pattern. No idea if that makes sense.

allejok96 · 2022-01-16T19:57:43Z

Pattern sounds like it's a single clip while in reality it's a collection. PatternContainer sounds like TrackContainer which implies that it contains instances of Pattern, but there are no such thing, it's just an abstract concept.

This is how patterns are stored. We want the programmer to visualize this when they hear the name.

It's is like a two-dimensional array, so perhaps PatternMatrix? Still not perfect because it sounds like each cell would be a pattern, when in reality a pattern is a column, but it gives you a clue of the data structure.

JohannesLorenz · 2022-01-16T20:46:08Z

I find PatternMatrix sounds really misleading, because, as you say, this might make readers think that the red things are the Patterns. ClipMatrixForBBTrack would be better.

But I think, to make the inheritation more clear, it should still be named ...TrackContainer.... So, TrackContainerOfBBTrack / TrackContainerInsideBBTrack / TrackContainerForBBTrack?

Btw, can you please draw that image as ASCII art into the header of the track container?

cyber-bridge · 2022-01-16T22:15:17Z

Looks like a table to me, maybe BBClipTable?

allejok96 · 2022-01-17T10:51:10Z

We are removing the term BB. Some more suggestions:

Song Editor edits the Song and Pattern Editor edits the PatternSet ...?

Or PatternTracks get their data from the PatternSourceContainer ...?

allejok96 · 2022-01-18T22:21:46Z

Forgive my endless naming issue. I think I have made up my mind now. Let's hear what you say.

I loved PatternSet but it gets confusing like: PatternSet::setCurrentPattern()

So what about PatternCollection? It has the same vibe but little longer...

There is just one collection in a project
The collection contains all patterns
It does not insinuate that pattern is a class as much as PatternContainer would imo
"Pattern Editor deals with the Pattern Collection"

For reasons why this is a issue in the first place, see this terrible infographic:

Why not make inheritance more clear in the name?

Well, since we are thinking in terms of "patterns" and pattern isn't even an object, I think putting the focus on tracks makes it more confusing. Makes you wonder: where is the Pattern? With a nice fuzzy name it looks good in the code and then we put that ASCII art in there so when you look it up you'll get a better grip. Besides, PatternEditorTrackContainerView would be terrible :)

JohannesLorenz · 2022-01-19T00:18:09Z

I, personally, agree to all you said in the last post. One minor question: If there would be a second PatternTrack in the Song (left side in your image), it would link to the same PatternCollection, correct?

M374LX · 2022-01-19T00:20:26Z

Another possibility is PatternStore.

allejok96 · 2022-01-19T20:00:52Z

I like PatternStore too :) it's shorter. Doesn't matter which one to me, native English speakers should decide.

allejok96 · 2022-01-19T20:04:55Z

If there would be a second PatternTrack ... it would link to the same PatternCollection?

Yes, same PatternCollection, different index/pattern/column.

src/core/Track.cpp

src/gui/editors/PatternEditor.cpp

Spekular · 2022-01-23T07:01:50Z

Did a super rudimentary review (only cpp, h, and txt files; only side-by-side comparisons). Haven't reviewed BBTrackContainer changes yet since GitHub's diff failed to show them side to side, but didn't find anything obviously wrong elsewhere (aside from the TODO'd data file upgrade).

Co-authored-by: Spekular <[email protected]>

JohannesLorenz · 2022-01-31T18:28:32Z

Thanks for the fixup. How should we advance? 2nd review? Merge? Extend?

allejok96 · 2022-01-31T18:34:43Z

At least someone should glance over the changes of BBTrackContainer with git diff -M40 (since github doesn't detect the rename) before merging

JohannesLorenz · 2022-02-03T19:03:59Z

At least someone should glance over the changes of BBTrackContainer with git diff -M40 (since github doesn't detect the rename) before merging

@allejok96 git diff 7fe2152938db49c8b8a2d04a5ab1eccfd38c821a -M40 include/PatternStore.h just shows the whole file as new. How do I use it correctly?

JohannesLorenz · 2022-02-03T19:26:24Z

Another thing I'm just noticing: If "pattern" == "bb track", then "pattern tracks" means "bb track tracks", right? So "pattern track" should nowhere be used?

Spekular · 2022-02-03T19:37:55Z

Another thing I'm just noticing: If "pattern" == "bb track", then "pattern tracks" means "bb track tracks", right? So "pattern track" should nowhere be used?

I don't think pattern = bb track though. Aren't the contents of the BB editor a pattern, and then pattern clips are placed on pattern tracks in order to loop that pattern as part of the song?

allejok96 · 2022-02-03T20:13:12Z

shows the whole file as new. How do I use it correctly?

-M sets the threshold for file renaming, so you'd have to specify both the old and new file name

git diff bbRename -M40 -- include/PatternStore.h include/BBTrackContainer.h

I don't think pattern = bb track though. Aren't the contents of the BB editor a pattern, and then pattern clips are placed on pattern tracks in order to loop that pattern as part of the song?

This is the correct understanding. @JohannesLorenz I may have created confusing with the drawings in this discussion, but I hope there is no misleading comments in the code?

JohannesLorenz · 2022-02-03T20:18:01Z

Oh, right, there are patterns and pattern tracks...

* Do not confuse the Tracks in the PatternStore with PatternTracks.
* PatternTracks are used in the Song Editor to reference a pattern.
* They do not contain any MIDI data.

Thanks for your explanations. Will continue to review.

JohannesLorenz

Just a few minor/easy comments. Aside from that, very clear PR 👍

src/tracks/PatternTrack.cpp

include/PatternClip.h

src/tracks/PatternTrack.cpp

include/PatternStore.h

include/PatternClip.h

allejok96 · 2022-02-08T21:36:09Z

Added brief descriptions to PatternTrack and PatternClip.
Renamed PatternTrack::index to patternIndex to avoid confusing code like track->index()
Renamed PatternClip::patternTrackIndex to patternIndex (it's just a call to the one above)
Renamed "Beat/Bassline" to "Pattern" in the template files. In a future PR we'll add a regex to do the same for all other old files when they are loaded.

JohannesLorenz · 2022-02-11T01:40:48Z

@allejok96 You describe PatternTrack as a "Track type used in the Song (Editor) to reference a pattern in the PatternStore". However, PatternStore's comment says that a "pattern" is a column. Do you really reference a column with a PatternTrack?

allejok96 · 2022-02-11T10:42:54Z

@JohannesLorenz yes. The "reference" is returned by (renamed) PatternTrack::patternIndex

Afaik it comes from a map of PatternTrack* and int, with is essentially a time position (bar) in the PatternStore.

JohannesLorenz

Rework is OK.

The only thing that might (optionally) be worth adding in PatternStore.h is what you mean with columns. I didn't understand that you can select the column by using that combo box from the editor.

Next step is testing before we merge.

JohannesLorenz · 2022-02-12T01:01:14Z

Thinking about what needs to be tested...

The whole pattern editor functionality (should still work as before)
Load/save and DnD with patterns (PR should have no impact)

Is that everything?

@allejok96 Also, in case you don't have merge access rights, feel free to formulate a commit message that I can put into the squash-merge.

allejok96 · 2022-02-12T08:58:23Z

That's about it. DnD untouched. Test usability of pattern-things. Check that no icons are missing or that anything looks weird (CSS).

allejok96 · 2022-02-12T09:12:13Z

Oh man I just realized something

Editor	Central widget
SongEditorWindow	SongEditor
AutomationEditorWindow	AutomationEditor
PianoRollWindow	PianoRoll
PatternEditor	PatternStoreView

They should be PatternEditorWindow and PatternEditor

JohannesLorenz · 2022-02-12T10:58:35Z

They should be PatternEditorWindow and PatternEditor

Agreeing. Shouldn't be too difficult to just rename two classes.

@Spekular , any objections?

allejok96 · 2022-02-12T11:26:47Z

PatternEditor.cpp will have the PatternEditorWindow definition at the top of the file, followed by the PatternEditor definition. Should this order be changed to match the one found in SongEditor.cpp, PianoRoll.cpp etc? git blame me for everything 😢 I guess the diff is all over the place anyhow...

JohannesLorenz · 2022-02-12T12:49:46Z

Should this order be changed to match the one found in SongEditor.cpp, PianoRoll.cpp etc?

I'd say yes (and in PatternEditor.h, too). These files are so small that the diff is probably still comprehensive. But if you want to make it clear: Complete the whole renaming stuff without swapping the definitions, give it to review again, then squash it all into 1 commit, and then add a second commit for swapping. If we do not squash-merge the result, then users can see this as 2 commits. (This could even be done in 2 PRs aswell)

Spekular · 2022-02-12T14:50:44Z

They should be PatternEditorWindow and PatternEditor

Agreeing. Shouldn't be too difficult to just rename two classes.

@Spekular , any objections?

No objection!

allejok96 · 2022-02-12T15:18:05Z

Three new commits:

Renaming the editor like discussed, plus:

Engine::getPatternStore -> Engine::patternStore (like the other getters)
Make some functions public in MidiClip instead of having it be friend class PatternEditor
Make PatternEditorWindow::m_editor public (like the other editors do)

Fix comments, how about it @JohannesLorenz ?
Switch place of editor/window (not so big diff anyway)

allejok96 · 2022-02-12T15:20:16Z

I think it's ready, finally.

Rename Beat/Bassline to Pattern

- BB* -> Pattern*
- BBTrackContainer -> PatternStore
- BBTrackContainerView -> PatternEditor
- BBEditor -> PatternEditorWindow

Does not touch save files

JohannesLorenz

LGTM

JohannesLorenz · 2022-02-12T18:16:51Z

Branch is opened for testing. The following should be tested:

The whole pattern editor functionality (should still work as before)
Everything else with patterns
Check that no icons are missing or that anything looks weird (CSS)
Load/save and DnD with patterns (PR did not touch those)

MessyBookshelf

Tested a few time and it seems to work great on my windows64 build, patterns save and load, extend, everything functions and looks good as far as I can tell 🎉

Rename BB to Pattern

bded6f5

Beat+Bassline Editor -> Pattern Editor BB* -> Pattern* BeatClip -> PatternClip Does not touch DataFile

M374LX reviewed Jan 15, 2022

View reviewed changes

include/MidiClip.h Outdated Show resolved Hide resolved

M374LX reviewed Jan 15, 2022

View reviewed changes

include/MidiClip.h Outdated Show resolved Hide resolved

M374LX reviewed Jan 15, 2022

View reviewed changes

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

Revert rename of MidiClipTypes::BeatClip

34a7493

allejok96 added 2 commits January 21, 2022 19:59

Rename PatternTrackContainer to PatternStore

884452c

Code formatting + explain about PatternStore

3226b4b

allejok96 force-pushed the bbRename branch from b75d9c3 to 3226b4b Compare January 22, 2022 15:55

Spekular reviewed Jan 23, 2022

View reviewed changes

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

src/gui/editors/PatternEditor.cpp Outdated Show resolved Hide resolved

allejok96 and others added 2 commits January 31, 2022 18:06

Update src/core/Track.cpp

12444e2

Co-authored-by: Spekular <[email protected]>

Update src/gui/editors/PatternEditor.cpp

2858094

Co-authored-by: Spekular <[email protected]>

JohannesLorenz self-requested a review February 3, 2022 18:19

JohannesLorenz mentioned this pull request Feb 6, 2022

Reorganising LMMS, renaming editors and classes and everything in between (cont. #120) #5592

Closed

56 tasks

JohannesLorenz reviewed Feb 6, 2022

View reviewed changes

allejok96 added 2 commits February 8, 2022 22:22

Rename PatternTrack::index() to patternIndex() + more comments

a7c9dc7

Rename Beat/Bassline to Pattern in the template files

86e1fbe

JohannesLorenz approved these changes Feb 11, 2022

View reviewed changes

Rename Pattern Editor classes for consistency

cf67afa

allejok96 added 2 commits February 12, 2022 15:55

Improved comments

b7a9e09

Switch place of PatternEditor and PatternEditorWindow

7864f9a

JohannesLorenz approved these changes Feb 12, 2022

View reviewed changes

MessyBookshelf reviewed Feb 13, 2022

View reviewed changes

JohannesLorenz merged commit dc73911 into LMMS:master Feb 13, 2022

allejok96 deleted the bbRename branch March 22, 2022 19:36

Veratil mentioned this pull request Nov 28, 2022

request: enhance beat bassline editor #6575

Closed

Rossmaxx mentioned this pull request Oct 24, 2023

Tentative changelog for 1.3 #6949

Open

Rename BB to Pattern #6284

Rename BB to Pattern #6284

Conversation

allejok96 commented Jan 15, 2022 • edited Loading

LmmsBot commented Jan 15, 2022 • edited Loading

Linux

Windows

macOS

allejok96 commented Jan 16, 2022

M374LX commented Jan 16, 2022

JohannesLorenz commented Jan 16, 2022

allejok96 commented Jan 16, 2022

JohannesLorenz commented Jan 16, 2022

cyber-bridge commented Jan 16, 2022

allejok96 commented Jan 17, 2022

allejok96 commented Jan 18, 2022

Why not make inheritance more clear in the name?

JohannesLorenz commented Jan 19, 2022

M374LX commented Jan 19, 2022

allejok96 commented Jan 19, 2022 • edited Loading

allejok96 commented Jan 19, 2022 • edited Loading

Spekular commented Jan 23, 2022

JohannesLorenz commented Jan 31, 2022

allejok96 commented Jan 31, 2022

JohannesLorenz commented Feb 3, 2022

JohannesLorenz commented Feb 3, 2022

Spekular commented Feb 3, 2022

allejok96 commented Feb 3, 2022

JohannesLorenz commented Feb 3, 2022

JohannesLorenz left a comment

Choose a reason for hiding this comment

allejok96 commented Feb 8, 2022

JohannesLorenz commented Feb 11, 2022

allejok96 commented Feb 11, 2022

JohannesLorenz left a comment

Choose a reason for hiding this comment

JohannesLorenz commented Feb 12, 2022

allejok96 commented Feb 12, 2022 • edited Loading

allejok96 commented Feb 12, 2022

JohannesLorenz commented Feb 12, 2022

allejok96 commented Feb 12, 2022

JohannesLorenz commented Feb 12, 2022

Spekular commented Feb 12, 2022

allejok96 commented Feb 12, 2022

allejok96 commented Feb 12, 2022

JohannesLorenz left a comment

Choose a reason for hiding this comment

JohannesLorenz commented Feb 12, 2022

MessyBookshelf left a comment

Choose a reason for hiding this comment

allejok96 commented Jan 15, 2022 •

edited

Loading

LmmsBot commented Jan 15, 2022 •

edited

Loading

allejok96 commented Jan 19, 2022 •

edited

Loading

allejok96 commented Jan 19, 2022 •

edited

Loading

allejok96 commented Feb 12, 2022 •

edited

Loading