Rename TCO and related to Clip #6226

M374LX · 2021-11-28T19:52:21Z

Rebasing #6044.

Renaming "pattern" to something else on the data file is a task for another PR. Thus, this one does not touch the data file.

As in #6044, this PR does not include renaming BB to Pattern.

LmmsBot · 2021-11-28T20:28:43Z

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

Windows

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)

🤖

{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/360d3o9twib9n5n3/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/42135894"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/ao20uxtxxaah3mo0/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/42135894"}]}, "commit_sha": "dd2e4ea60d9abc5da96e020a5911729f156d1158"}

JohannesLorenz · 2021-12-01T20:43:31Z

Thanks for your contribution. Is this ready to review?

M374LX · 2021-12-02T01:20:00Z

It is mostly ready for review, except that the locales are pending.

Should we leave the locales for another PR?

JohannesLorenz · 2021-12-02T19:26:12Z

I don't know what you mean by locales. Will changing those locales cause many conflicts for other PRs?

M374LX · 2021-12-02T22:39:22Z

The locales I refer to are the files in data/locale. Commits between e4dab80 and e2ae1fa update the file names on the locales, but fully replacing "pattern" with "clip" on the locales where appropriate is yet to be done.

JohannesLorenz · 2021-12-07T13:48:10Z

I asked in our Discord, but got no answer. I'm not sure if this can/must be done here or in Transifex. I wish anyone could help here...

Spekular · 2021-12-13T20:42:20Z

Since we've now got an answer...

You could use sed to fix that, or you could tell me and I will do the fix and sync with Transifex

... this PR can either be merged, or updated to upgrade the locales as well, correct?

JohannesLorenz · 2021-12-13T22:50:26Z

this PR can either be merged, or updated to upgrade the locales as well, correct?

I'd say let's upgrade this PR (if @M374LX has time). And then, a review will be required.

M374LX · 2021-12-13T23:11:55Z

@Spekular It is possibly not appropriate to replace every single occurrence of "pattern" with "clip" on the locales, which means that simply running sed -i 's/pattern/clip/g' data/locale/*.ts is not a good idea.

I plan to check this in the next days.

M374LX · 2021-12-20T18:59:39Z

I think the task of updating the locales should be left for someone with more experience on Transifex.

Update: I am going to update the locales directly just in case (done on fffa09b).

JohannesLorenz · 2021-12-21T13:58:05Z

I am going to update the locales directly just in case

OK, but what is left to do on Transifex then?

M374LX · 2021-12-21T15:32:29Z

Possibly just update the line numbers on the locale files in case Transifex has this ability.

JohannesLorenz · 2021-12-21T16:38:48Z

Update which line numbers? You didn't change line number in fffa09b, did you?

M374LX · 2021-12-21T17:46:01Z

No line numbers have been changed by fffa09b, but some line number references on the locale files are outdated due to previous changes (not necessarily from this PR) that caused tr() calls to be moved to different lines.

JohannesLorenz · 2021-12-23T12:24:59Z

Somehow, git difftool tells me that data/themes/classic/pat_rec.png was changed, but I can't see the difference. Do you know what that means?

M374LX · 2021-12-23T20:13:48Z

pat_rec.png has been renamed to clip_rec.png.

JohannesLorenz · 2021-12-26T11:36:13Z

I'll review this.

What's the intended scope? I guess it's point 1-3 from the reorg point, so:

Rename TCO -> Clip
Rename Pattern -> MidiClip
Rename *TCO and *TCOView -> *Clip and *ClipView

And renaming BB to Pattern is not done yet because the PR blocks itself (i.e. there will be a following PR).

Is this all correct?

M374LX · 2021-12-26T21:49:06Z

That is all correct, @JohannesLorenz.

JohannesLorenz · 2021-12-27T20:27:10Z

I just see that the locales still contain TrackContentWidget. Is that wanted?

Edit: Now I see that the code contains them, too. Wanted?

M374LX · 2021-12-27T22:04:41Z

TrackContentWidget has not been renamed. It refers to the area on which clips (previously track content objects) are shown. Unless someone suggests a better name, I would say it is wanted.

c5f17bf fixes two small oversights in comments.

JohannesLorenz

Review finished. I added comments to the code. Despite from those comments:

Some remaining "Pattern" are left. They are shown using git grep -i pattern. I see that in at least in 3 Plugins. Not all might be pattern in sense of "clip", but at least, in "plugins/MidiExport/MidiExport.h", I guess writeBBPattern should be changed.
The Wiki should be checked for TCO and others, too (submodule in doc/wiki).

src/gui/ClipView.cpp

include/MidiClip.h

include/NotePlayHandle.h

include/Song.h

include/BasicFilters.h

include/ClipView.h

src/core/DataFile.cpp

src/core/midi/MidiPort.cpp

src/gui/LmmsStyle.cpp

JohannesLorenz · 2022-01-01T11:56:11Z

@M374LX Looks like you resolved all my comments. Should I re-review already, or still wait?

M374LX · 2022-01-01T21:58:19Z

No problem in reviewing the code again, but I have not yet reviewed the remaining occurrences of "pattern" with git grep -i pattern and the Wiki.

JohannesLorenz · 2022-01-02T01:56:56Z

No problem in reviewing the code again, but I have not yet reviewed the remaining occurrences of "pattern" with git grep -i pattern and the Wiki.

Oh, OK. I'll wait. Let me know when you're through with everything.

JohannesLorenz

Thanks for your time! Only 4 quick points:

I think you forgot to revert those occurrences of "clips" strings:

src/gui/ClipView.cpp:   QDomElement clips = dataFile.content().firstChildElement( "clips" );
src/gui/widgets/TrackContentWidget.cpp: QDomElement clipParent = dataFile.content().firstChildElement( "clips" );
src/gui/widgets/TrackContentWidget.cpp: QDomElement clipParent = dataFile.content().firstChildElement( "clips" );

Fix pattern in HydrogenImport and MidiExport plugins? At least, plugins/HydrogenImport/HydrogenImport.cpp has it partially changed, but partially not. Is that really wanted? And plugins/MidiExport/MidiExport.h should probably treated in the same way as HydrogenImport (they are both very similar).
Change pattern here?
src/gui/ClipView.cpp: "elidedPatternName"?
src/gui/editors/PianoRoll.cpp: "detuningPattern"?
Wiki: Can you please edit the files directly or push them into the submodule and submit them there?

M374LX · 2022-01-11T23:59:44Z

I suggest keeping "pattern" in HydrogenImport because this is the term used by Hydrogen project files (example: https://github.com/hydrogen-music/hydrogen/blob/master/data/demo_songs/TR808kit-demo.h2song).

JohannesLorenz · 2022-01-12T02:02:04Z

Before I can review, there is still a build error. I copied this from the "appveyor" build:

--   Fetching doc/wiki @ --depth 100
Submodule 'doc/wiki' (https://github.com/lmms/lmms.wiki.git) registered for path 'doc/wiki'
Cloning into 'C:/projects/lmms/doc/wiki'...
fatal: remote error: upload-pack: not our ref 069fa1fc1a1784e944629431100c979d50b9f0a3
fatal: the remote end hung up unexpectedly
Fetched in submodule path 'doc/wiki', but it did not contain 069fa1fc1a1784e944629431100c979d50b9f0a3. Direct fetching of that commit failed.

Did you forget to push your wiki submodule?

M374LX · 2022-01-12T02:39:20Z

This is the message when trying to push the wiki submodule:

remote: Resolving deltas: 100% (247/247), completed with 2 local objects.
To https://github.com/M374LX/lmms.wiki.git
 ! [remote rejected] clip-rename -> clip-rename (shallow update not allowed)
error: failed to push some refs to 'https://github.com/M374LX/lmms.wiki.git'

PhysSong · 2022-01-12T02:48:09Z

@M374LX Could you try git fetch --unshallow on the wiki submodule before pushing?

M374LX · 2022-01-12T02:56:02Z

@PhysSong The message disappeared after trying the suggested command.

@JohannesLorenz Did it solve the problem?

PhysSong · 2022-01-12T02:59:56Z

@M374LX I think you pushed to the wiki on your fork. I pushed your changes to LMMS/lmms.

JohannesLorenz · 2022-01-13T02:39:17Z

Works here. Will review it in the next days.

JohannesLorenz · 2022-01-13T21:25:35Z

Perfect. Now, we need a good commit message, so we can squash all commits into one. Proposal:

Rename TCO and related to Clip (without savefiles)

This PR renames

    TCO -> Clip
    Pattern -> MidiClip
    *TCO and *TCOView -> *Clip and *ClipView

The savefiles are not yet modified by this PR

@M374LX Feel free to modify and tell me when we can squash-merge it.

M374LX · 2022-01-13T23:29:27Z

@JohannesLorenz The suggested commit message is fine.

The PR is ready for merge.

Spekular

This breaks two old upgrades, and lacks its own upgrade routine.

src/core/DataFile.cpp

M374LX · 2022-01-14T12:00:04Z

@Spekular I am going to open a new PR to fix the breakage soon. As for the lack of its own upgrade routine, it is because this PR is not intended to touch the data files (the breakage you pinpointed is a slip).

Spekular · 2022-01-14T12:19:02Z

it is because this PR is not intended to touch the data files

I think this is a mistake, unless there's some benefit that I'm not seeing. IMHO data file upgrades should come with the PRs that need them, because otherwise we get commits (which are basically versions) with broken backwards compatibility.

Until the upgrade PR is created and merged, anyone building master will be unable to load any projects.
If we had nightlies up and running they'd be useless until the upgrade routine came in.
Anyone bisecting an issue where they need to load a project will be inconvenienced if they land on these commits.

JohannesLorenz · 2022-01-14T15:15:10Z

Oh no, how could we oversee this! Sorry. Should we force-push-remove the merge commit in this case? It's a bit late, but seems the most clean to me.

Alternatively, we should push a fix really fast. Though that does not help with your bisecting argument.

M374LX · 2022-01-14T15:33:16Z

As it is easy to fix, I see no reason for a force-push-remove. I can do the fix later today.

JohannesLorenz · 2022-01-14T16:26:16Z

Thanks. I should be able to review and merge your fixes today, too. We can still force-push later if that should ever be wanted.

allejok96 · 2022-01-14T17:58:48Z

src/gui/ClipView.cpp

-	m_tco->movePosition( pos );
-	AutomationPattern::resolveAllIDs();
+	TimePos pos = m_clip->startPosition();
+	QDomElement clips = dataFile.content().firstChildElement( "tcos" ); //TODO: rename "tcos" to "clips"


Any reason tcos was kept here?

Because the PR intends to not change anything in the Datafile?

This is just a dummy datafile tho, for clipboard and drag-n-drop, should be safe... here's a better link

allejok96 · 2022-01-15T09:21:08Z

lmms/src/core/DataFile.cpp

Line 57 in 55d361f

{ "sampleclip", {"src"} },

This breaks loading of old project bundles with sample tracks

Spekular · 2022-01-16T12:18:09Z

[...] data file upgrades should come with the PRs that need them, because otherwise we get commits [...] with broken backwards compatibility.

Or in this case, it apparently would have worked fine if the existing upgrade routines hadn't been affected: #5592 (comment)

Apologies for the untested (incorrect) claims, it never occurred to me that LMMS would ignore the tag names on load.

While we're on the topic, I saw a lot of lines with "tco" and one with "pattern" reintroduced in #6279, these will be changed back to "clip" in a later PR, right? (e.g. the data file upgrade PR?)

JohannesLorenz · 2022-01-16T13:54:34Z

While we're on the topic, I saw a lot of lines with "tco" and one with "pattern" reintroduced in #6279, these will be changed back to "clip" in a later PR, right? (e.g. the data file upgrade PR?)

Yes, that's the plan.

This was referenced Nov 28, 2021

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

Closed

Rename TCO and related to Clip #6044

Closed

M374LX changed the title ~~(WIP) Rename TCO and related to Clip~~ Rename TCO and related to Clip Nov 28, 2021

M374LX marked this pull request as ready for review November 28, 2021 23:21

JohannesLorenz self-requested a review December 26, 2021 11:30

JohannesLorenz requested changes Dec 28, 2021

View reviewed changes

M374LX force-pushed the clip-rename branch from 846cecb to 885e3cb Compare December 31, 2021 17:21

M374LX requested a review from JohannesLorenz January 9, 2022 20:14

JohannesLorenz requested changes Jan 10, 2022

View reviewed changes

M374LX added 4 commits January 11, 2022 20:22

Fixes

53cbd58

Fixes

7b6d92f

Update Wiki

eeed1d3

Fixes (MidiExport)

cd6bb76

M374LX requested a review from JohannesLorenz January 12, 2022 00:04

Update Wiki

ab5596c

JohannesLorenz approved these changes Jan 13, 2022

View reviewed changes

JohannesLorenz merged commit 55d361f into LMMS:master Jan 14, 2022

Spekular reviewed Jan 14, 2022

View reviewed changes

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

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

allejok96 reviewed Jan 14, 2022

View reviewed changes

M374LX mentioned this pull request Jan 14, 2022

Fix broken data file upgrade #6279

Merged

Rename TCO and related to Clip #6226

Rename TCO and related to Clip #6226

Conversation

M374LX commented Nov 28, 2021 • edited Loading

LmmsBot commented Nov 28, 2021 • edited Loading

Windows

JohannesLorenz commented Dec 1, 2021

M374LX commented Dec 2, 2021

JohannesLorenz commented Dec 2, 2021

M374LX commented Dec 2, 2021 • edited Loading

JohannesLorenz commented Dec 7, 2021

Spekular commented Dec 13, 2021

JohannesLorenz commented Dec 13, 2021

M374LX commented Dec 13, 2021 • edited Loading

M374LX commented Dec 20, 2021 • edited Loading

JohannesLorenz commented Dec 21, 2021

M374LX commented Dec 21, 2021

JohannesLorenz commented Dec 21, 2021

M374LX commented Dec 21, 2021

JohannesLorenz commented Dec 23, 2021

M374LX commented Dec 23, 2021

JohannesLorenz commented Dec 26, 2021

M374LX commented Dec 26, 2021

JohannesLorenz commented Dec 27, 2021 • edited Loading

M374LX commented Dec 27, 2021

JohannesLorenz left a comment

Choose a reason for hiding this comment

JohannesLorenz commented Jan 1, 2022

M374LX commented Jan 1, 2022

JohannesLorenz commented Jan 2, 2022

JohannesLorenz left a comment

Choose a reason for hiding this comment

M374LX commented Jan 11, 2022

JohannesLorenz commented Jan 12, 2022

M374LX commented Jan 12, 2022

PhysSong commented Jan 12, 2022

M374LX commented Jan 12, 2022

PhysSong commented Jan 12, 2022

JohannesLorenz commented Jan 13, 2022

JohannesLorenz commented Jan 13, 2022

M374LX commented Jan 13, 2022

Spekular left a comment

Choose a reason for hiding this comment

M374LX commented Jan 14, 2022

Spekular commented Jan 14, 2022

JohannesLorenz commented Jan 14, 2022

M374LX commented Jan 14, 2022

JohannesLorenz commented Jan 14, 2022

allejok96 Jan 14, 2022

Choose a reason for hiding this comment

JohannesLorenz Jan 14, 2022

Choose a reason for hiding this comment

allejok96 Jan 14, 2022

Choose a reason for hiding this comment

allejok96 commented Jan 15, 2022

Spekular commented Jan 16, 2022

JohannesLorenz commented Jan 16, 2022

M374LX commented Nov 28, 2021 •

edited

Loading

LmmsBot commented Nov 28, 2021 •

edited

Loading

M374LX commented Dec 2, 2021 •

edited

Loading

M374LX commented Dec 13, 2021 •

edited

Loading

M374LX commented Dec 20, 2021 •

edited

Loading

JohannesLorenz commented Dec 27, 2021 •

edited

Loading