User Preference to Hide beat grid on waveform #1247

williamlemus · 2017-05-05T19:18:34Z

These changes are to enable the functionality requested in bug 1659888 : https://bugs.launchpad.net/mixxx/+bug/1659888 . I have done some testing on it and it is working. Please let me know if anything needs to be changed.

modified:   src/preferences/dialog/dlgprefwaveform.h
modified:   src/preferences/dialog/dlgprefwaveformdlg.ui
modified:   src/waveform/renderers/waveformrenderbeat.cpp
modified:   src/waveform/renderers/waveformwidgetrenderer.cpp
modified:   src/waveform/renderers/waveformwidgetrenderer.h
modified:   src/waveform/waveformwidgetfactory.cpp
modified:   src/waveform/waveformwidgetfactory.h

Added functionality to hide beat grid lines on waveform.

modified: src/preferences/dialog/dlgprefwaveform.h modified: src/preferences/dialog/dlgprefwaveformdlg.ui modified: src/waveform/renderers/waveformrenderbeat.cpp modified: src/waveform/renderers/waveformwidgetrenderer.cpp modified: src/waveform/renderers/waveformwidgetrenderer.h modified: src/waveform/waveformwidgetfactory.cpp modified: src/waveform/waveformwidgetfactory.h Added functionality to hide beat grid lines on waveform.

daschuer · 2017-05-06T00:38:37Z

src/waveform/renderers/waveformwidgetrenderer.cpp

@@ -274,6 +275,10 @@ void WaveformWidgetRenderer::setZoom(int zoom) {
    m_zoomFactor = math_clamp<double>(zoom, s_waveformMinZoom, s_waveformMaxZoom);
 }

+void WaveformWidgetRenderer::setDisplayGrid(bool set){
+    m_renderGrid = set;


Render or display?

daschuer · 2017-05-06T00:41:18Z

src/waveform/waveformwidgetfactory.cpp

@@ -223,6 +224,13 @@ bool WaveformWidgetFactory::setConfig(UserSettingsPointer config) {
        m_config->set(ConfigKey("[Waveform]","ZoomSynchronization"), ConfigValue(m_zoomSync));
    }

+    int displayGrid = m_config->getValueString(ConfigKey("[Waveform]", "beatGridLinesCheckBox")).toInt(&ok);


Later we may use other controls, but the config key remains.
how about just "showBeatgrid"?

By the way, you can use here the m_config->getValue() template function.

daschuer · 2017-05-06T00:42:34Z

src/waveform/waveformwidgetfactory.cpp

@@ -432,6 +440,22 @@ void WaveformWidgetFactory::setZoomSync(bool sync) {
    }
 }

+void WaveformWidgetFactory::setDisplayGrid(bool sync){


Use BeatGrid, to be free to introduce other type of grids.

Ok. I will update all the function to better reflect that we are referring to the Beat Grid.

daschuer · 2017-05-06T00:43:41Z

Thank you for adopting this. I have just left some renaming suggestions.
I hope I find time to test this soon.

Be-ing · 2017-05-10T07:40:27Z

src/waveform/waveformwidgetfactory.cpp

+        m_config->set(ConfigKey("[Waveform]", "beatGridLinesCheckBox"), ConfigValue(m_beatGridEnabled));
+    }
+
+    if(m_waveformWidgetHolders.size() == 0){


nitpick: please use spaces around conditional expressions:
if (m_waveformWidgetHolders.size() == 0) {

Be-ing · 2017-05-23T03:59:26Z

Thanks for working on this. I tested it and it works until Mixxx is restarted. If the option is unchecked (beatgrid is hidden) on shutdown, when Mixxx starts again, the beatgrid will be back even though the option in the preferences is unchecked.

Be-ing · 2017-05-23T04:00:12Z

src/preferences/dialog/dlgprefwaveformdlg.ui

+     <item row="12" column="1">
+      <widget class="QCheckBox" name="beatGridLinesCheckBox">
+       <property name="toolTip">
+        <string>Display beat grid lines on waveform</string>


Change this to just "Beat grid"

I will make these changes, and look into why the preference is being ignored when mixxx is restarted.

@Be-ing I've resolved the issue with the preference being ignored when starting Mixxx. Which lines am I changing to "Beat Grid" and "Show Beat Markers"? I'll push all the changes together.

Be-ing · 2017-05-23T04:01:18Z

src/preferences/dialog/dlgprefwaveformdlg.ui

+        <string>Display beat grid lines on waveform</string>
+       </property>
+       <property name="text">
+        <string>Enable Beat Grid Lines</string>


Change this to "Show beat markers"

Be-ing · 2017-06-25T21:53:16Z

Ping @williamlemus, have you continued work on this? I'd like to increase the maximum waveform zoom out level further after merging this. The reason I chose the limit I did in #1165 is because it started to look weird when zoomed out far and showing the beat markers.

williamlemus · 2017-06-26T02:39:01Z

@Be-ing Hey I haven't had a chance to figure out how to get around the issue where it's defaulting. I'll try to take a look by mid week and reach out via IRC if I'm still stuck.

… up mixxx

williamlemus · 2017-06-30T03:57:57Z

@Be-ing I've made the updates requested. Please let me know if anything else needs to be corrected.

daschuer · 2017-06-30T05:56:54Z

src/waveform/waveformwidgetfactory.cpp

@@ -223,6 +224,13 @@ bool WaveformWidgetFactory::setConfig(UserSettingsPointer config) {
        m_config->set(ConfigKey("[Waveform]","ZoomSynchronization"), ConfigValue(m_zoomSync));
    }



you can simplify the code passing the default value

int showBeatGrid = m_config->getValue("[Waveform]", "beatGridLinesCheckBox", m_showBeatGrid)

(not sure about the exact syntax though)

daschuer · 2017-06-30T06:18:54Z

src/waveform/waveformwidgetfactory.cpp

+void WaveformWidgetFactory::setDisplayBeatGrid(bool sync) {
+    m_beatGridEnabled = sync;
+    if (m_config) {
+        m_config->set(ConfigKey("[Waveform]", "beatGridLinesCheckBox"), ConfigValue(m_beatGridEnabled));


we normally do not set the value, if not required. We try to load the value and if it fails pick the default. Is it required here?

The dlgprefwaveform calls this in order to update the Config
void DlgPrefWaveform::slotSetGridLines(bool displayGrid) { WaveformWidgetFactory::instance()->setDisplayBeatGrid(displayGrid); }

Would there be a better way to do this?

I think it would make more sense to set the ConfigKey in DlgPrefWaveform than here.

Oh, I see the way you did it is consistent with the surrounding code. You don't have to clean this up if you don't have time, but it would be nice to move the setting of the ConfigKeys into DlgPrefWaveform.

Should I do this just for the display grid preference?

No, keep it consistent. Either leave it how it is or refactor how all the settings in this function are set.

Be-ing · 2017-06-30T14:47:29Z

I tested again and it works as expected. I think the UI in the Preferences could be improved. I think it would be difficult for users to discover on their own all the way at the bottom of the preference page. How about putting it between "End of track warning" and "Default zoom level"? Hopefully that will make it more obvious to users that the option exists if they choose to zoom the waveform out far and see the grid lines close together and cluttering the view. For the wording, I suggest changing it to "Beat grid" for the label in the left column and "Show beat markers" for the checkbox label in the right column.

esbrandt · 2017-06-30T19:59:17Z

Thanks for adding the feature.

If you plan to extend this any further, adding a Dim mode can be an option.
The dimmed mode is basically the normal mode, just with lower alpha 0.3 instead of 0.9 for the beat grid lines.
Traktor has this option and it is really neat. Gives access to the bead grid without obstructing the view on the waveform/markers as much as with the default beatgrid.

williamlemus · 2017-07-03T22:21:01Z

@esbrandt That sounds like a good idea. Can I do the dimming functionality as a separate PR? I would like to get this PR as I'm not sure if I'll have time the next couple of weeks to extend it.

esbrandt · 2017-07-04T20:13:36Z

@esbrandt That sounds like a good idea. Can I do the dimming functionality as a separate PR? I would like to get this PR as I'm not sure if I'll have time the next couple of weeks to extend it.

Totally up to you.
Just submit a new PR when you are ready.

…d check box for easier discover

williamlemus · 2017-07-16T22:04:48Z

I've simplified the code as requested. @Be-ing I also moved up the beat grid option and changed the text.

Please let me know if I missed anything.

Be-ing · 2017-07-17T00:40:52Z

src/waveform/waveformwidgetfactory.cpp

-    } else {
-        m_config->set(ConfigKey("[Waveform]", "beatGridLinesCheckBox"), ConfigValue(m_beatGridEnabled));
-    }
+    int showBeatGrid = m_config->getValue(ConfigKey("[Waveform]", "beatGridLinesCheckBox"), m_beatGridEnabled);


If you change this to a bool you don't need to do the static_cast on the next line.

Be-ing · 2017-07-17T00:47:15Z

The order of the items in the preferences looks odd because there are 3 checkboxes in the right column but only a label in the left column for the middle one. I think moving the new beatgrid line preference between "End of track warning" and "Default zoom level" would work better.

Be-ing · 2017-07-17T16:48:40Z

Looks good to me! Thanks.

williamlemus · 2017-07-18T12:51:23Z

Should I worry about the unsuccessful check? Also, I'll start working on the dimming, and will make a PR to adding dimming and move the set config into waveform preferences.

Be-ing · 2017-07-18T14:16:22Z

No, we use the free plans for Travis and AppVeyor that have short time limits. Randomly builds will run out of time. AppVeyor passed, so it's okay.

Also, I'll start working on the dimming, and will make a PR to adding dimming and move the set config into waveform preferences.

Cool, thanks.

daschuer · 2017-07-23T21:27:54Z

Thank you, @williamlemus and all involved. :-)

daschuer reviewed May 6, 2017

View reviewed changes

Modified naming to better reflect functionality.

7619c22

Be-ing reviewed May 10, 2017

View reviewed changes

Adding correct spacing around curly braces

c26605e

Be-ing suggested changes May 23, 2017

View reviewed changes

Resolved issue with beat grid preference being ignored upon starting…

809590c

… up mixxx

daschuer reviewed Jun 30, 2017

View reviewed changes

Simplify code to get default value in waveform widget. Move Beat Gri…

3e40760

…d check box for easier discover

Be-ing reviewed Jul 17, 2017

View reviewed changes

Update position of Beat grid checkbox. Cleared up code set beat grid.

e2f1923

Be-ing approved these changes Jul 17, 2017

View reviewed changes

daschuer merged commit 034f8d9 into mixxxdj:master Jul 23, 2017

williamlemus mentioned this pull request Oct 24, 2017

Add Dim mode to beat grid lines #1363

Merged

mixxxbot mentioned this pull request Aug 23, 2022

Feature: disable beat grid in Options > View #8777

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User Preference to Hide beat grid on waveform #1247

User Preference to Hide beat grid on waveform #1247

williamlemus commented May 5, 2017

daschuer May 6, 2017

daschuer May 6, 2017

daschuer May 6, 2017

williamlemus May 6, 2017

daschuer commented May 6, 2017

Be-ing May 10, 2017

Be-ing commented May 23, 2017 •

edited

Loading

Be-ing May 23, 2017

williamlemus May 23, 2017

williamlemus Jun 28, 2017

Be-ing May 23, 2017

Be-ing commented Jun 25, 2017

williamlemus commented Jun 26, 2017

williamlemus commented Jun 30, 2017

daschuer Jun 30, 2017

daschuer Jun 30, 2017

williamlemus Jul 16, 2017

Be-ing Jul 17, 2017

Be-ing Jul 17, 2017

williamlemus Jul 17, 2017

Be-ing Jul 17, 2017

Be-ing commented Jun 30, 2017 •

edited

Loading

esbrandt commented Jun 30, 2017

williamlemus commented Jul 3, 2017

esbrandt commented Jul 4, 2017

williamlemus commented Jul 16, 2017

Be-ing Jul 17, 2017

Be-ing commented Jul 17, 2017

Be-ing commented Jul 17, 2017 •

edited

Loading

williamlemus commented Jul 18, 2017

Be-ing commented Jul 18, 2017 •

edited

Loading

daschuer commented Jul 23, 2017

		@@ -223,6 +224,13 @@ bool WaveformWidgetFactory::setConfig(UserSettingsPointer config) {
		m_config->set(ConfigKey("[Waveform]","ZoomSynchronization"), ConfigValue(m_zoomSync));
		}

User Preference to Hide beat grid on waveform #1247

User Preference to Hide beat grid on waveform #1247

Conversation

williamlemus commented May 5, 2017

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

daschuer commented May 6, 2017

Choose a reason for hiding this comment

Be-ing commented May 23, 2017 • 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

Be-ing commented Jun 25, 2017

williamlemus commented Jun 26, 2017

williamlemus commented Jun 30, 2017

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

Be-ing commented Jun 30, 2017 • edited Loading

esbrandt commented Jun 30, 2017

williamlemus commented Jul 3, 2017

esbrandt commented Jul 4, 2017

williamlemus commented Jul 16, 2017

Choose a reason for hiding this comment

Be-ing commented Jul 17, 2017

Be-ing commented Jul 17, 2017 • edited Loading

williamlemus commented Jul 18, 2017

Be-ing commented Jul 18, 2017 • edited Loading

daschuer commented Jul 23, 2017

Be-ing commented May 23, 2017 •

edited

Loading

Be-ing commented Jun 30, 2017 •

edited

Loading

Be-ing commented Jul 17, 2017 •

edited

Loading

Be-ing commented Jul 18, 2017 •

edited

Loading