Move widget files, add folder for track and instrument #6374

allejok96 · 2022-04-14T20:23:07Z

Moving files in src/gui, no change to their source. Updated translations accordingly.

gui/clips for *ClipView
gui/editors for editors and their child widgets
gui/instrument for InstrumentTrackWindow and child widgets
gui/tracks for *TrackView and child widgets
gui/widgets for generic widgets only

allejok96 · 2022-04-14T20:28:09Z

I didn't touch LinkedModelGroupViews, Controls and ControlLayout. I wonder if @JohannesLorenz thinks these should be moved out of widgets ?

LmmsBot · 2022-04-14T20:33:29Z

🤖 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.1.189+gef9e92a91-mingw-win32.exe (build link)
Windows 64-bit: lmms-1.3.0-alpha.1.189+gef9e92a91-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)

Linux

Linux (AppImage): lmms-1.3.0-alpha.1.189+gef9e92a91-linux-x86_64.AppImage (build link)

macOS

macOS : lmms-1.3.0-alpha.1.189+gef9e92a91-mac10.14.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/f1bacb40-4840-4e39-aeba-30782cc04215/artifacts/0/lmms-1.3.0-alpha.1.189+gef9e92a91-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/16899?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/4a2226c3-09a5-4e9c-9ee4-d7cbac59fbb8/artifacts/0/lmms-1.3.0-alpha.1.189+gef9e92a91-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/16897?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/eo4fqj24n8o3qd0u/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/43635922"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/b5enk7ejxk6trce0/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/43635922"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://output.circle-artifacts.com/output/job/1a4f2f60-c4a3-4cea-b437-c715d4813a30/artifacts/0/lmms-1.3.0-alpha.1.189+gef9e92a91-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/16895?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://output.circle-artifacts.com/output/job/a0b411cc-2ccd-429a-9c92-02609b81d05b/artifacts/0/lmms-1.3.0-alpha.1.189+gef9e92a91-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/16898?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "dfb051efdcd9050094622943080ee01f64b56ae7"}

Spekular

A few thoughts

PositionLine is only used in editors, right? Maybe it should live in the editors folder.
I'm not sure how I feel about ClipViews being in the track folder. I understand the reasoning, but that's not where I would expect them to be if I were searching for them.

Looks good otherwise

allejok96 · 2022-04-15T14:46:27Z

I'm not sure how I feel about ClipViews being in the track folder.

I share that feeling 😅 but it would be nice if they lived together in a directory, even if it's only 5 of them.

PositionLine is only used in editors, right? Maybe it should live in the editors folder.

I could see that. Same goes for TimeLineWidget, StepRecorderWidget and Rubberband.

JohannesLorenz · 2022-04-15T17:03:57Z

I didn't touch LinkedModelGroupViews, Controls and ControlLayout.

I designed Controls and ControlLayout as generic-purpose widgets, so I think they can stay in widgets.

For LinkedModelGroupViews, this is only useful if you have a dynamic set of controls (e.g. TripleOscillator is static, but e.g. Lv2, LADSPA etc. are dynamic, because the controls depend on the loaded plugin). Due to their complexity and their rare use case (Lv2 currently, possibly LADSPA, VST), I'd rather say not widgets?

allejok96 · 2022-04-15T19:37:30Z

Okay so LinkedModelGroupViews goes.

I'd like to move out ControlLayout too since it's not a QWidget even if it's general purpose (similarly to ActionGroup).

JohannesLorenz · 2022-04-15T19:52:01Z

I'd like to move out ControlLayout too since it's not a QWidget even if it's general purpose (similarly to ActionGroup).

Sounds good!

allejok96 · 2022-04-15T20:33:13Z

I don't know what the consensus is for more small widgets in the editors folder and a separate clips folder (like @Spekular insinuated), but I've made those changes. Now let's see what the people think.

PhysSong · 2022-04-18T10:46:02Z

Why is InstrumentTrackWindow.cpp in gui/instrument while SampleTrackWindow.cpp is in gui and InstrumentTrackView.cpp is in gui/tracks?
I think InstrumentTrackWindow.cpp should stay in gui unless we move SampleTrackWindow.cpp to gui/tracks.

allejok96 · 2022-04-18T16:37:18Z

I understand how it can get confusing with many classes named "instrument". The instrument directory should really be called instrumentwindow to avoid all confusion...

Why is InstrumentTrackWindow.cpp in gui/instrument while SampleTrackWindow.cpp is in gui?

I thought it made sense to put the window in the same directory as all its children, but I'll reverse it if I get a second opinion.

Why is InstrumentTrackView.cpp is in gui/tracks?

InstrumentTrackView is a track (view)

... unless we move SampleTrackWindow.cpp to gui/tracks

SampleTrackWindow is not a track, and not a child to a track (view)

allejok96 · 2022-04-20T14:51:21Z

I need some opinions before I can continue (will have to fix a lot of strings)

Is instrument a good name for the directory that holds the instrument window and it's stuff?
Should TrackContainerView be in tracks or editors? (it's actually a base class of SongEditor)
Is it ok to put editor specific widgets, like TimeLineWidget, into editors?

Spekular · 2022-04-20T22:06:20Z

Makes more sense to put TrackContainerView in editors than tracks, IMO, since it's the parent to both the song and pattern editors. I lean towards yes for the other two questions.

JohannesLorenz · 2022-04-21T17:38:24Z

My opinion would be a copy of Spekular's:

Yes. What other name would fit better if it has instrument specific GUI stuff?
In editors. Because if our philosophy is that everything in tracks is a widget used inside tracks, then TrackContainerView would not fit SongEditor (which contains tracks). Also, it's more like an editor to me.
Yes. I think we said everything inside "editors" should be a widget used inside editors.

allejok96 · 2022-04-22T17:57:17Z

Thank goodness for the Unix toolbox. This oneliner finds all renamed files since a given commit and updates all strings to use the new path (works only for paths relative to base of repo). Maybe you might find it useful.

git ls-tree -rz --name-only HEAD | xargs -0 sed -i -f <(git diff --raw ENTER_COMMIT_ID_HERE | sed -En 's/.*R[0-9]+\t(.*)\t(.*)/s^\1^\2^g/p')

allejok96 · 2022-04-22T18:10:18Z

I've squashed the commits, ready for review.

JohannesLorenz · 2022-04-23T06:29:06Z

@allejok96 While users are reviewing, if you rebase a local copy of this branch on #6379 (or wait until it's in master), you can check your locale updates.

allejok96 · 2022-04-24T07:16:07Z

@JohannesLorenz Strings look alright, except for PatternClipView which will cause a conflict with #6379 if I rename. Either I wait for check-strings to be merged, or you resolve the conflict later.

JohannesLorenz · 2022-04-25T11:55:07Z

@JohannesLorenz Strings look alright, except for PatternClipView which will cause a conflict with #6379 if I rename. Either I wait for check-strings to be merged, or you resolve the conflict later.

Both is OK to me. I'd say first come first serve?

allejok96 · 2022-04-26T20:36:24Z

I wait until the renames of this PR are approved.

JohannesLorenz · 2022-05-08T14:21:05Z

If it's just for moving forward, I'd help you by reviewing this PR, but I'm rarely involved in GUI stuff, so I'm not a good candidate. I might try a review if no one else does it in the next days... 🥲

allejok96 · 2022-05-08T14:37:04Z

@JohannesLorenz and @Spekular's suggestions has been added. @PhysSong had some concerns - maybe we can get a second opinion? Or else we're good to go, as long as it builds 🙃

JohannesLorenz

Most widgets reside where I'd expect them. A few points:

MixerLineLcdSpinbox sounds a bit non-generic. Should it be in src/gui?
I wonder if SubWindow is so generic that it should go down into widgets?
Should we add a folder src/gui/base-classes? (AutomatableButton, SideBarWidget, ControllerDialog...)
We have a lot of ...Views. Extra folder src/gui/views?
I wonder why we have an extra directory for editors, but leave the non-modal dialogs (EffectControlDialog) in src/gui. After all, aren't they both some kind of windows inside the MainWindow?

Anyways, these are all just suggestions. The PR is OK.

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

I wonder why we have an extra directory for editors, but leave the non-modal dialogs (EffectControlDialog) in src/gui.

I aimed to group things by concept rather than if it's a base/derived class, if it contains a specific word, or if it's located at a specific level in the GUI. I find this approach more helpful when looking for where a certain feature is implemented. So editors are for the main editors of LMMS and their specialized widgets. You'd never call EffectControlDialog an editor, so it's excluded.

MixerLineLcdSpinbox sounds a bit non-generic. Should it be in src/gui?

Perhaps. I just put it where all of the other Lcd* siblings were, since there are a few.

I wonder if SubWindow is so generic that it should go down into widgets?

On the other hand it's mostly used by MainWindow. Personally I wouldn't look for it in widgets.

JohannesLorenz · 2022-05-22T10:09:08Z

I'd say let's merge it, then?

PhysSong · 2022-05-23T01:00:23Z

I'm still not sure where InstrumentTrackWindow.cpp should be, but I'm okay with the others.

JohannesLorenz · 2022-05-23T18:36:00Z

Note: As I merged the check-strings branch prior to this PR, it might be good if you rebase on master and see if CI still passes.

Add clips, tracks and instrument directory, clean up widgets.

allejok96 · 2022-05-23T18:45:27Z

PatternClipView which will cause a conflict

Lol I forgot to update my own remark. Well, now we know that CI works 😄

Spekular reviewed Apr 15, 2022

View reviewed changes

allejok96 force-pushed the widgetsRename branch from c2451d6 to 2b1144c Compare April 22, 2022 18:08

JohannesLorenz mentioned this pull request Apr 24, 2022

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

Closed

56 tasks

JohannesLorenz self-requested a review May 20, 2022 17:03

JohannesLorenz approved these changes May 20, 2022

View reviewed changes

Reorganize files in src/gui

fd3e59b

Add clips, tracks and instrument directory, clean up widgets.

Update translations after moving files

2139afb

allejok96 force-pushed the widgetsRename branch from 2b1144c to 2139afb Compare May 23, 2022 18:41

Fix strings for PatternClipView

dfb051e

DomClark merged commit d2dd7e3 into LMMS:master May 29, 2022

JohannesLorenz mentioned this pull request Jun 6, 2022

Remove some Qt from core #6426

Draft

allejok96 deleted the widgetsRename branch July 2, 2022 18:06

Rossmaxx mentioned this pull request Jul 17, 2022

mallets broken on master build #6464

Closed

sakertooth pushed a commit to sakertooth/lmms that referenced this pull request May 30, 2023

Move widget files, add folder for track and instrument (LMMS#6374)

b3abd62

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move widget files, add folder for track and instrument #6374

Move widget files, add folder for track and instrument #6374

allejok96 commented Apr 14, 2022 •

edited

Loading

allejok96 commented Apr 14, 2022

LmmsBot commented Apr 14, 2022 •

edited

Loading

Windows

Linux

macOS

Spekular left a comment

allejok96 commented Apr 15, 2022

JohannesLorenz commented Apr 15, 2022

allejok96 commented Apr 15, 2022

JohannesLorenz commented Apr 15, 2022

allejok96 commented Apr 15, 2022

PhysSong commented Apr 18, 2022

allejok96 commented Apr 18, 2022

allejok96 commented Apr 20, 2022 •

edited

Loading

Spekular commented Apr 20, 2022

JohannesLorenz commented Apr 21, 2022

allejok96 commented Apr 22, 2022

allejok96 commented Apr 22, 2022

JohannesLorenz commented Apr 23, 2022

allejok96 commented Apr 24, 2022

JohannesLorenz commented Apr 25, 2022

allejok96 commented Apr 26, 2022

JohannesLorenz commented May 8, 2022

allejok96 commented May 8, 2022

JohannesLorenz left a comment

allejok96 commented May 21, 2022

JohannesLorenz commented May 22, 2022

PhysSong commented May 23, 2022

JohannesLorenz commented May 23, 2022

allejok96 commented May 23, 2022

Move widget files, add folder for track and instrument #6374

Move widget files, add folder for track and instrument #6374

Conversation

allejok96 commented Apr 14, 2022 • edited Loading

allejok96 commented Apr 14, 2022

LmmsBot commented Apr 14, 2022 • edited Loading

Windows

Linux

macOS

Spekular left a comment

Choose a reason for hiding this comment

allejok96 commented Apr 15, 2022

JohannesLorenz commented Apr 15, 2022

allejok96 commented Apr 15, 2022

JohannesLorenz commented Apr 15, 2022

allejok96 commented Apr 15, 2022

PhysSong commented Apr 18, 2022

allejok96 commented Apr 18, 2022

allejok96 commented Apr 20, 2022 • edited Loading

Spekular commented Apr 20, 2022

JohannesLorenz commented Apr 21, 2022

allejok96 commented Apr 22, 2022

allejok96 commented Apr 22, 2022

JohannesLorenz commented Apr 23, 2022

allejok96 commented Apr 24, 2022

JohannesLorenz commented Apr 25, 2022

allejok96 commented Apr 26, 2022

JohannesLorenz commented May 8, 2022

allejok96 commented May 8, 2022

JohannesLorenz left a comment

Choose a reason for hiding this comment

allejok96 commented May 21, 2022

JohannesLorenz commented May 22, 2022

PhysSong commented May 23, 2022

JohannesLorenz commented May 23, 2022

allejok96 commented May 23, 2022

allejok96 commented Apr 14, 2022 •

edited

Loading

LmmsBot commented Apr 14, 2022 •

edited

Loading

allejok96 commented Apr 20, 2022 •

edited

Loading