Add vertical zoom controls to SongEditor #6776

enp2s0 · 2023-07-19T15:37:45Z

Song editor tracks could already be resized by shift-dragging them, but this was little known and hard to use. This PR adds a global zoom option that zooms all SongEditor tracks in and out. It also respects existing manual-zoom changes and scales them accordingly.

Currently supports various zoom levels between 100% and 1600%.

* Bump cmt submodule Closes LMMS#6006

enp2s0 · 2023-07-19T15:38:45Z

This is currently WIP/needs testing, not ready for merge yet.

enp2s0 · 2023-07-19T22:24:37Z

did a bit of testing and loaded a few of my projects up and everything seems to be in order. Ready for testing/merge

Veratil

New code style convention fixes.

include/SongEditor.h

include/TrackContainerView.h

src/gui/editors/SongEditor.cpp

src/gui/editors/TrackContainerView.cpp

messmerd

Just a few mostly minor things

messmerd · 2023-07-21T00:17:38Z

src/gui/editors/SongEditor.cpp

@@ -547,19 +558,19 @@ void SongEditor::wheelEvent( QWheelEvent * we )
 		{
 			z--;
 		}
-		z = qBound( 0, z, m_zoomingModel->size() - 1 );
+		z = qBound(0, z, m_zoomingXModel->size() - 1);


Suggested change

z = qBound(0, z, m_zoomingXModel->size() - 1);

z = std::clamp(z, 0, m_zoomingXModel->size() - 1);

We're trying to get away from Qt-specific functions where we can. You may need to include <algorithm> for this.

messmerd · 2023-07-21T00:20:47Z

src/gui/editors/SongEditor.cpp

+	connect(m_zoomingXModel, SIGNAL(dataChanged()), this, SLOT(zoomingChanged()));
+	connect(m_zoomingXModel, SIGNAL(dataChanged()), m_positionLine, SLOT(update()));


It'd be better to avoid the SIGNAL and SLOT macros and use the newer way of calling connect.
See: https://doc.qt.io/qt-5/signalsandslots.html

messmerd · 2023-07-21T00:21:13Z

src/gui/editors/SongEditor.cpp

@@ -242,16 +247,22 @@ SongEditor::SongEditor( Song * song ) :


 	//Set up zooming model
-	for( float const & zoomLevel : m_zoomLevels )
+	for(const auto& zoomLevel : m_zoomXLevels)


Suggested change

for(const auto& zoomLevel : m_zoomXLevels)

for (const auto& zoomLevel : m_zoomXLevels)

style

messmerd · 2023-07-21T00:22:27Z

include/TrackContainerView.h

@@ -167,6 +168,7 @@ public slots:

 protected:
 	static const int DEFAULT_PIXELS_PER_BAR = 16;
+	static const int DEFAULT_TRACK_HEIGHT = 32;


Suggested change

static const int DEFAULT_TRACK_HEIGHT = 32;

static constexpr int DEFAULT_TRACK_HEIGHT = 32;

I would remove this constant entirely - there are already MINIMAL_TRACK_HEIGHT and DEFAULT_TRACK_HEIGHT constants defined in include/Track.h.

messmerd · 2023-07-21T00:34:26Z

include/SongEditor.h

+	static const QVector<float> m_zoomXLevels;
+	static const QVector<float> m_zoomYLevels;


Suggested change

static const QVector<float> m_zoomXLevels;

static const QVector<float> m_zoomYLevels;

static constexpr std::array m_zoomXLevels = { 0.125f, 0.25f, 0.5f, 1.0f, 2.0f, 4.0f, 8.0f, 16.0f };

static constexpr std::array m_zoomYLevels = { 1.0f, 1.25f, 1.5f, 1.75f, 2.0f, 2.5f, 3.0f, 4.0f, 8.0f, 16.0f };

Can use constexpr arrays instead.

messmerd · 2023-07-21T00:35:28Z

src/gui/editors/SongEditor.cpp

+const QVector<float> SongEditor::m_zoomXLevels =
 		{ 0.125f, 0.25f, 0.5f, 1.0f, 2.0f, 4.0f, 8.0f, 16.0f };

+const QVector<float> SongEditor::m_zoomYLevels =
+		{ 1.0f, 1.25f, 1.5f, 1.75f, 2.0f, 2.5f, 3.0f, 4.0f, 8.0f, 16.0f };
+


These can be removed after making them constexpr.

messmerd · 2023-07-21T00:36:30Z

src/gui/editors/SongEditor.cpp

+	m_zoomingXComboBox->move(580, 4);
+	m_zoomingXComboBox->setModel(m_editor->m_zoomingXModel);
+	m_zoomingXComboBox->setToolTip(tr("Horizontal zooming"));
+	connect(m_editor->zoomingXModel(), SIGNAL(dataChanged()), this, SLOT(updateSnapLabel()));


The SIGNAL and SLOT macros can be removed here too

messmerd · 2023-07-21T00:38:03Z

src/gui/editors/SongEditor.cpp

+	auto zoomx_lbl = new QLabel(m_toolBar);
+	zoomx_lbl->setPixmap(embed::getIconPixmap("zoom_x"));
+	auto zoomy_lbl = new QLabel(m_toolBar);


zoomx_lbl and zoomy_lbl don't follow our naming conventions. Maybe zoomXLabel and zoomYLabel could be used.

messmerd · 2023-07-21T00:41:16Z

src/gui/editors/SongEditor.cpp

+	connect(m_zoomingXModel, SIGNAL(dataChanged()), m_positionLine, SLOT(update()));
+
+	// Set up Y-axis zooming model
+	for(const auto& zoomLevel : m_zoomYLevels)


Suggested change

for(const auto& zoomLevel : m_zoomYLevels)

for (const auto& zoomLevel : m_zoomYLevels)

style

DomClark · 2023-07-21T21:29:29Z

src/gui/editors/TrackContainerView.cpp

+		// clamp the value to prevent it from becoming smaller than the zoom level.
+		if(new_height < lround(h * DEFAULT_TRACK_HEIGHT))
+		{
+			new_height = lround(h * DEFAULT_TRACK_HEIGHT);
+		}


I would use std::max here. Also, why restrict to the zoomed default track height, rather than the absolute minimum track height? Users can still shrink tracks once zoomed in, so this will reset the height of such tracks whenever the user changes the zoom level, which sounds annoying.

DomClark · 2023-07-21T23:52:45Z

src/gui/editors/TrackContainerView.cpp

+	int height = lround(m_trackHeightScale * DEFAULT_TRACK_HEIGHT);
+	trackView->setFixedHeight(height);
+	trackView->getTrack()->setHeight(height);


This will overwrite any custom height when creating the view for a track, which will affect loading tracks from files and cloning tracks.

DomClark · 2023-07-22T16:36:43Z

Thinking more about this, I wonder if it would be better to leave the height stored in the track alone, and instead just scale the view. I think that would be a bit cleaner conceptually, since the zoom level is a property of the track container, rather than of the tracks themselves. It would also avoid any issues with saving and loading track heights.

DomClark · 2023-07-22T16:37:54Z

src/gui/editors/TrackContainerView.cpp

@@ -349,6 +356,32 @@ void TrackContainerView::setPixelsPerBar( int ppb )
 	}
 }

+void TrackContainerView::setVerticalScale(double h)


Surely there's a better name than h for this parameter?

DomClark · 2023-07-22T16:59:22Z

src/gui/editors/SongEditor.cpp

+const QVector<float> SongEditor::m_zoomYLevels =
+		{ 1.0f, 1.25f, 1.5f, 1.75f, 2.0f, 2.5f, 3.0f, 4.0f, 8.0f, 16.0f };


Any particular reason for these specific values? You may also want to look at #6664, which is changing the horizontal zoom control to a continuous slider.

tresf and others added 4 commits July 11, 2023 12:52

Bump cmt submodule (LMMS#6151)

c53f3d9

* Bump cmt submodule Closes LMMS#6006

allow y-resizing of song editor

10d9512

respect manually resized tracks

9002145

rename existing zoom variables for consistency

94361c6

bugfix: make new tracks respect scaling

fb4f762

Veratil requested changes Jul 19, 2023

View reviewed changes

DomClark self-requested a review July 19, 2023 23:31

enp2s0 added 3 commits July 20, 2023 12:38

rename for consistency

8d6d042

style/formatting fix

ee05592

one more fix

cdf0ac4

messmerd reviewed Jul 21, 2023

View reviewed changes

DomClark reviewed Jul 21, 2023

View reviewed changes

DomClark reviewed Jul 22, 2023

View reviewed changes

tresf force-pushed the master branch from c2ae07a to 5ccc8d9 Compare July 26, 2023 12:56

sakertooth added the gui label Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add vertical zoom controls to SongEditor #6776

Add vertical zoom controls to SongEditor #6776

enp2s0 commented Jul 19, 2023

enp2s0 commented Jul 19, 2023

enp2s0 commented Jul 19, 2023

Veratil left a comment

messmerd left a comment

messmerd Jul 21, 2023 •

edited

Loading

messmerd Jul 21, 2023

messmerd Jul 21, 2023

messmerd Jul 21, 2023

DomClark Jul 21, 2023

messmerd Jul 21, 2023

messmerd Jul 21, 2023

messmerd Jul 21, 2023

messmerd Jul 21, 2023

messmerd Jul 21, 2023

DomClark Jul 21, 2023

DomClark Jul 21, 2023

DomClark commented Jul 22, 2023

DomClark Jul 22, 2023

DomClark Jul 22, 2023

	z = qBound(0, z, m_zoomingXModel->size() - 1);
	z = std::clamp(z, 0, m_zoomingXModel->size() - 1);

		connect(m_zoomingXModel, SIGNAL(dataChanged()), this, SLOT(zoomingChanged()));
		connect(m_zoomingXModel, SIGNAL(dataChanged()), m_positionLine, SLOT(update()));

	for(const auto& zoomLevel : m_zoomXLevels)
	for (const auto& zoomLevel : m_zoomXLevels)

	static const int DEFAULT_TRACK_HEIGHT = 32;
	static constexpr int DEFAULT_TRACK_HEIGHT = 32;

		static const QVector<float> m_zoomXLevels;
		static const QVector<float> m_zoomYLevels;

	for(const auto& zoomLevel : m_zoomYLevels)
	for (const auto& zoomLevel : m_zoomYLevels)

		const QVector<float> SongEditor::m_zoomYLevels =
		{ 1.0f, 1.25f, 1.5f, 1.75f, 2.0f, 2.5f, 3.0f, 4.0f, 8.0f, 16.0f };

Add vertical zoom controls to SongEditor #6776

Are you sure you want to change the base?

Add vertical zoom controls to SongEditor #6776

Conversation

enp2s0 commented Jul 19, 2023

enp2s0 commented Jul 19, 2023

enp2s0 commented Jul 19, 2023

Veratil left a comment

Choose a reason for hiding this comment

messmerd left a comment

Choose a reason for hiding this comment

messmerd Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DomClark commented Jul 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

messmerd Jul 21, 2023 •

edited

Loading