Move WOverview::drawNextPixmapPart() into new class WOverviewLMH (Low Mid High), also add new HSV waveform overview and interface, to select woverview type #17

xorik · 2013-06-20T07:03:02Z

This helps to change interface of woverview widget in the future (e.g. replace it with HSV, or other new types)

daschuer · 2013-06-20T11:08:22Z

src/widget/woverview.h

+    float m_waveformPeak;
+
+    int m_diffGain;
+


I know it is not your fault, but please reorder like http://www.mixxx.org/wiki/doku.php/coding_guidelines#class_declaration

Hmm, I can't see, which rule is not satisfied?

Only thing I see is /* ... */ comment, is there more errors here?

@daschuer what exact you want I need to fix?

The order of the declaration blocks
public:
private:
protected:

I would prefer

public:
protected:
private:

So the most commenly used stuff is on top and not top+bottom

fixed in last commit

+1 to Max -- that's what we already use in most places.

daschuer · 2013-06-20T11:31:14Z

Thank you for your work!

For me it is in a merge-able state, because it does not change the current behavior, but is fist step for a configurable waveform overview. But since this is the result from our teamwork, I would like to have a confirm from an other team member before merge.

In the meanwhile it would be nice if you could check your changes against http://www.mixxx.org/wiki/doku.php/coding_guidelines. There are some braces at start of a line and some /* */ comments.

xorik · 2013-06-20T11:48:46Z

Should I need to create bug on launchpad for this?

daschuer · 2013-06-20T12:42:32Z

Should I need to create bug on launchpad for this?

For what? All team member will already get a copy of this discussion by mail.

ywwg · 2013-06-20T13:29:45Z

Bugs are only for code committed in trunk or another release branch. Since your code is still in review and in your personal branch, no bug is needed. Just go ahead and fix it! 👍

xorik · 2013-06-25T06:50:06Z

Hi guys! I just added combobox in the preferences dialog, and add functions to select and store waveform overview type, please check it

daschuer · 2013-06-25T07:39:30Z

src/dlgprefcontrols.cpp

+    waveformOverviewComboBox->setCurrentIndex(
+            m_pConfig->getValueString(ConfigKey("[Waveform]","WaveformOverviewType"), "0").toInt());
+    connect(waveformOverviewComboBox,SIGNAL(currentIndexChanged(int)),
+            this,SLOT(slotSetWaveformOverviewType(int)));


Please add commends to clarify what "0" is and to clarify the relation between config options and ComboBox items/index.

Please add commends to clarify what "0" is

I wrote "By default we set filtered woverview", I must write something more?

and to clarify the relation between config options and ComboBox items/index.

Hmm... I don't understand, can you give an example?

// Waveform overview init
waveformOverviewComboBox->addItem( tr("Filtered") ); // "0"
waveformOverviewComboBox->addItem( tr("HSV") ); // "1"

// By default we set "Filtered" woverview = "0"
waveformOverviewComboBox->setCurrentIndex(
m_pConfig->getValueString(ConfigKey("[Waveform]","WaveformOverviewType"), "0").toInt());

daschuer · 2013-06-25T08:00:47Z

Although you have just moved most of the commented lines, you have inherit the code clean up job. Sorry ;-)

daschuer · 2013-06-25T08:55:16Z

src/skin/legacyskinparser.cpp

-    WOverview* overviewWidget = new WOverview(pSafeChannelStr, m_pConfig, m_pParent);
+    WOverview* overviewWidget = NULL;
+
+    // HSV or LMH waveform overview type


// HSV = "1" or "Filtered" = "0" (LMH) waveform overview type

xorik · 2013-06-25T09:11:33Z

Although you have just moved most of the commented lines, you have inherit the code clean up job. Sorry ;-)

no problem :)

xorik · 2013-06-25T09:42:52Z

@daschuer please check last commits

daschuer · 2013-06-25T11:46:12Z

Thank you, it looks good to me now, except some comment style violations:
//disabed_code()
// comment

IMHO we can commit after this is fixed. But lets give the other a chance for review.

xorik · 2013-06-25T12:04:40Z

Done

daschuer · 2013-06-25T12:47:14Z

Thank you very much!

rryan · 2013-06-25T13:46:51Z

src/widget/woverviewhsv.cpp

+}
+
+bool WOverviewHSV::drawNextPixmapPart() {
+    ScopedTimer t("WOverview::drawNextPixmapPart");


Please change to WOverviewHSV so that timing stats are reported separately for the two overviews.

rryan · 2013-06-25T13:49:44Z

LGTM to me as well @xorik (other than the 2 ScopedTimer changes I just commented on) -- thanks!

xorik · 2013-06-25T15:15:39Z

@rryan, done

Move WOverview::drawNextPixmapPart() into new class WOverviewLMH (Low Mid High), also add new HSV waveform overview and interface, to select woverview type

rryan · 2013-06-25T16:05:04Z

Thanks @xorik -- merged.

…h sync When loading a track that is not yet present in the library (and thus doesn't have any BPM because it hasn't been analyzed yet) while another deck is playing and both decks have sync enabled, a debug assertion is triggered: DEBUG ASSERT: "isValid()" in function double mixxx::Bpm::value() const at src/track/bpm.h:53 Aborted (core dumped) The backtrace looks as follows: #0 0x00007f175c87234c in __pthread_kill_implementation () at /usr/lib/libc.so.6 #1 0x00007f175c8254b8 in raise () at /usr/lib/libc.so.6 #2 0x00007f175c80f534 in abort () at /usr/lib/libc.so.6 #3 0x00007f175cf05ee4 in qt_assert(char const*, char const*, int) () at /usr/lib/libQt5Core.so.5 #4 0x000055deb2e67e1c in mixxx::(anonymous namespace)::handleMessage(QtMsgType, QMessageLogContext const&, QString const&) (type=<optimized out>, context=<optimized out>, input=<optimized out>) at src/util/logging.cpp:355 #5 0x00007f175cf47128 in () at /usr/lib/libQt5Core.so.5 #6 0x00007f175cf3fd8a in () at /usr/lib/libQt5Core.so.5 #7 0x00007f175cf06526 in QMessageLogger::critical(char const*, ...) const () at /usr/lib/libQt5Core.so.5 #8 0x000055deb2e5c720 in mixxx_debug_assert(char const*, char const*, int, char const*) (assertion=assertion@entry=0x55deb39bd0db "isValid()", file=file@entry=0x55deb39bbf30 "src/track/bpm.h", line=line@entry=53, function=function@entry=0x55deb39bbf08 "double mixxx::Bpm::value() const") at gsrc/util/assert.h:9 #9 0x000055deb2ee7e7e in mixxx_debug_assert_return_true(char const*, char const*, int, char const*) (function=0x55deb39bbf08 "double mixxx::Bpm::value() const", line=53, file=0x55deb39bbf30 "src/track/bpm.h", assertion=0x55deb39bd0db "isValid()") at gsrc/util/assert.h:18 #10 mixxx::Bpm::value() const (this=<synthetic pointer>) at src/track/bpm.h:53 #11 mixxx::operator*(mixxx::Bpm, double) (multiple=1, bpm=...) at src/track/bpm.h:160 #12 SyncControl::setLocalBpm(mixxx::Bpm) (this=<optimized out>, localBpm=...) at src/engine/sync/synccontrol.cpp:567 #13 0x000055deb34c7ba3 in EngineBuffer::postProcess(int) (this=0x55deb56eb060, iBufferSize=2048) at src/engine/enginebuffer.cpp:1318 #14 0x000055deb3139023 in EngineMaster::processChannels(int) (this=0x55deb5449440, iBufferSize=<optimized out>) at src/engine/enginemaster.cpp:383 #15 0x000055deb31394f7 in EngineMaster::process(int) (this=0x55deb5449440, iBufferSize=iBufferSize@entry=2048) at src/engine/enginemaster.cpp:410 #16 0x000055deb2f91d0b in SoundManager::onDeviceOutputCallback(long) (this=<optimized out>, iFramesPerBuffer=iFramesPerBuffer@entry=1024) at src/soundio/soundmanager.cpp:596 #17 0x000055deb32dd794 in SoundDevicePortAudio::callbackProcessClkRef(long, float*, float const*, PaStreamCallbackTimeInfo const*, unsigned long) (this=0x55deb553e6b0, framesPerBuffer=1024, out=<optimized out>, in=<optimized out>, timeInfo=<optimized out>, statusFlags=<optimized out>) at src/soundio/sounddeviceportaudio.cpp:965 This happens because `newLocalBpm` is invalid when `localBpm` is invalid. Trying to do sync decks while no tempo information is available does not make sense, so we only synchronize decks if the local BPM is available.

2.1 beta announcement

xorik added 2 commits June 20, 2013 10:58

Move some needed vars into protected part of WOverview class

70425b2

Move WOverview::drawNextPixmapPart() into new class WOverviewLMH

c44dd80

daschuer reviewed Jun 20, 2013
View reviewed changes

Cosmetic cleanup in woverview.h

d482e9c

HSV renderer for the woverview widget

1f3d43b

xorik added 5 commits June 24, 2013 11:06

Add combobox for waveform overview type into interface preferences

6825d0a

Add controls for waveform overview combobox

95602c3

Select HSV or LMH waveform overview type, depending on the user settings

2018475

Use getValueString() function with default value

701b1f5

rebootMixxxView() after changing waveform overview type

c19f67f

daschuer reviewed Jun 25, 2013
View reviewed changes

xorik added 2 commits June 25, 2013 13:39

Add some comments about waveformOverviewComboBox

7dc7818

Cosmetic fix

6caf25d

Fixing comments style for woverview widget

1e9694c

rryan reviewed Jun 25, 2013
View reviewed changes

Change ScopedTimer description for WOverviewLMH and WOverviewHSV

82b453e

rryan added a commit that referenced this pull request Jun 25, 2013

Merge pull request #17 from xorik/test

34dcb1b

Move WOverview::drawNextPixmapPart() into new class WOverviewLMH (Low Mid High), also add new HSV waveform overview and interface, to select woverview type

rryan merged commit 34dcb1b into mixxxdj:master Jun 25, 2013

esbrandt mentioned this pull request Dec 15, 2013

RGB waveform display #118

Merged

ywwg mentioned this pull request Dec 20, 2013

Refactor the engine to process samples in the range of [-1.0, 1.0]. Fixes Bug #1204039. #126

Merged

daschuer mentioned this pull request Dec 28, 2014

Spinny updates #440

Merged

daschuer mentioned this pull request Jan 8, 2015

New SoundSource/AudioSource API #411

Merged

daschuer mentioned this pull request Aug 13, 2017

Multi-broadcasting #1300

Merged

19 tasks

daschuer mentioned this pull request Jun 11, 2018

Library Layout Redesign Single pane #1117

Closed

11 tasks

uklotzde mentioned this pull request Jan 16, 2020

Replace deprecated QSignalMapper with lambda expressions #2427

Merged

ronso0 mentioned this pull request Sep 4, 2022

Effects: allow clearing chains with empty '---' preset #10859

Merged

ronso0 mentioned this pull request May 9, 2023

qopenglwindow-based replacement of qglwidget, full glsl shader based waveforms, vumeters and spinnies #10989

Merged

Swiftb0y mentioned this pull request Aug 7, 2023

GNOME 44 (Wayland): Context menu on tree items in side bar closes immediately #11576

Closed

m0dB pushed a commit to m0dB/mixxx that referenced this pull request Jan 21, 2024

Merge pull request mixxxdj#17 from Be-ing/2.1_beta

b037747

2.1 beta announcement

acolombier mentioned this pull request May 22, 2024

Simplify waveform preferences panel #13226

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move WOverview::drawNextPixmapPart() into new class WOverviewLMH (Low Mid High), also add new HSV waveform overview and interface, to select woverview type #17

Move WOverview::drawNextPixmapPart() into new class WOverviewLMH (Low Mid High), also add new HSV waveform overview and interface, to select woverview type #17

xorik commented Jun 20, 2013

daschuer Jun 20, 2013

xorik Jun 20, 2013

xorik Jun 20, 2013

xorik Jun 20, 2013

daschuer Jun 20, 2013

kain88-de Jun 20, 2013

xorik Jun 20, 2013

rryan Jun 25, 2013

daschuer commented Jun 20, 2013

xorik commented Jun 20, 2013

daschuer commented Jun 20, 2013

ywwg commented Jun 20, 2013

xorik commented Jun 25, 2013

daschuer Jun 25, 2013

xorik Jun 25, 2013

daschuer Jun 25, 2013

daschuer commented Jun 25, 2013

daschuer Jun 25, 2013

xorik commented Jun 25, 2013

xorik commented Jun 25, 2013

daschuer commented Jun 25, 2013

xorik commented Jun 25, 2013

daschuer commented Jun 25, 2013

rryan Jun 25, 2013

rryan commented Jun 25, 2013

xorik commented Jun 25, 2013

rryan commented Jun 25, 2013

Move WOverview::drawNextPixmapPart() into new class WOverviewLMH (Low Mid High), also add new HSV waveform overview and interface, to select woverview type #17

Move WOverview::drawNextPixmapPart() into new class WOverviewLMH (Low Mid High), also add new HSV waveform overview and interface, to select woverview type #17

Conversation

xorik commented Jun 20, 2013

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

daschuer commented Jun 20, 2013

xorik commented Jun 20, 2013

daschuer commented Jun 20, 2013

ywwg commented Jun 20, 2013

xorik commented Jun 25, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Jun 25, 2013

Choose a reason for hiding this comment

xorik commented Jun 25, 2013

xorik commented Jun 25, 2013

daschuer commented Jun 25, 2013

xorik commented Jun 25, 2013

daschuer commented Jun 25, 2013

Choose a reason for hiding this comment

rryan commented Jun 25, 2013

xorik commented Jun 25, 2013

rryan commented Jun 25, 2013