Effect selector widget by Be-ing · Pull Request #1135 · mixxxdj/mixxx

Be-ing · 2017-01-19T18:41:30Z

Add a new QComboBox widget to select an effect. This has several advantages:

Next/previous effect and eject effect buttons can be removed from skins, freeing valuable space
If the user wants to load a specific effect, they do not need to cycle through effects to do so.
The list of available effects is visible to the user.
The list of available effects is in predictable alphabetical order.

It is easy to replace EffectName elements in skin XML with the new EffectSelector element. All that is required is changing the element name. The skin will need to define styles for WEffectSelector to make the colors match the skin instead of the system Qt colors.

Be-ing · 2017-01-19T18:43:53Z

I am thinking we should remove the effect full name versus short name distinction. I think we only need short names and long descriptions.

daschuer · 2017-01-20T10:32:01Z

I am thinking we should remove the effect full name versus short name distinction. I think we only need short names and long descriptions.

The issue now is that we have no rule, what is short and what is long.
I can imagine when that a skin can make use of the short names, when labeling knobs and the space is relay limited. In this case the Lables can be abbreviated up to just before unrecognizable.
On the other hand a well recognizable name is very useful if some the space is there, like the Shade Effect selector.

Can we come to a rule, that is usable for Translators and considered proportional fonts?
// Trans late not longer than "XXXXXXX"

daschuer · 2017-01-20T10:33:59Z


-const QList<QString> EffectsManager::getAvailableEffects() const {
-    QList<QString> availableEffects;
+bool alphabetizeEffectNameIdPairs(const QPair<QString, QString>& pair1,


Using QPair is an antipattern. Please create your own proper named struct or class.

Even as a typedef: using NameIdPair = QPair<QString, QString>

I think I'll refactor this so EffectManager keeps a QList<EffectManifest> sorted alphabetically. But yeah, I don't understand what the issue with QPair is.

QPair is bad, because you cannot see what .first and .second is.
It requires explanation on every usage.
class NameIdPair {
public:
QString Id;
QString Name;
}

would be self explainig

In this case you can just use a typedef though...

You cannot get rid of the meaningless first and second members.

True, I agree.

daschuer · 2017-01-20T10:35:42Z

-    QList<QString> availableEffects;
+bool alphabetizeEffectNameIdPairs(const QPair<QString, QString>& pair1,
+                                 const QPair<QString, QString>& pair2) {
+    return pair1.second < pair2.second;


Use QString::localeAwareCompare()

daschuer · 2017-01-20T10:38:15Z

+// Each returned QPair has the effect ID and full name.
+const QList<QPair<QString, QString> > EffectsManager::getEffectNamesFiltered(
+        EffectManifestFilterFnc filter) const {
+    QList<QPair<QString, QString> > filteredEffectNames;


Oy! QPair ;-)

daschuer · 2017-01-20T10:45:19Z

-    int index = effects.indexOf(effectId);
-    if (++index >= effects.size()) {
+    int index;
+    for (index = 0; index < idNamePairs.size(); ++index) {


This can become a range based loop

Oh no, I see.

daschuer · 2017-01-20T10:49:04Z

 }

 QString EffectsManager::getPrevEffectId(const QString& effectId) {
-    const QList<QString> effects = getAvailableEffects();


I think you can move out the common inner part of the Prev and Next functions.

That's so little code that I don't think it would be worth it.

daschuer · 2017-01-20T10:57:28Z

I think the effect selector is a candidate for the long names by default.
If someone has no space, an option for the short names would be nice.

Is it possible to show the short names in the Box and the long names and short in the selector?
"Short (Long Name)"

A tool tip showing the description might be also usable.

Be-ing · 2017-01-20T14:35:55Z

I can imagine when that a skin can make use of the short names, when labeling knobs and the space is relay limited. In this case the Lables can be abbreviated up to just before unrecognizable.

You are conflating effect names and parameter names. I think short names for parameters should stay, but we should move to one short name for effects.

Is it possible to show the short names in the Box and the long names and short in the selector?
"Short (Long Name)"

That was what I intended to do initially. However, I realized it would be a bit confusing to see the text change when selecting a new effect versus seeing the selected effect name. "Short (Long Name)" would be overcomplicated and unnecessarily long. The default behavior of QComboBox is to expand to the width of the longest item in the list. I think this behavior makes sense and should be kept, so the list items should be kept short. Thus the rule for translators would be, "translate this to as short of a string as possible that is still understandable".

A tool tip showing the description might be also usable.

That is already implemented in this PR, which is why I think long names are unnecessary.

Be-ing · 2017-01-20T14:40:03Z

You are conflating effect names and parameter names. I think short names for parameters should stay, but we should move to one short name for effects.

Actually, the same applies to effect parameters. Space will always be limited where they are displayed, so I don't think there is ever a need for an extended name. The description is sufficient.

rryan · 2017-01-22T21:14:56Z

+        addItem(effectPair.second, QVariant(effectPair.first));
+    }
+    //: Displayed when no effect is loaded
+    addItem(tr("None"), QVariant(nullEffectId));


Instead of having a null string constant, you can use the fact the QVariant and QString both have isNull methods for when their no-args constructor is used.

QString("").isNull() == false and QString().isNull() == true

rryan · 2017-01-22T21:16:14Z

+    }
+}
+
+void WEffectSelector::slotEffectSelected(int newIndex) {


check for null m_pChainSlot early and exit?

rryan · 2017-01-22T21:20:15Z

+    const QString id = itemData(newIndex).toString();
+    EffectPointer pEffect;
+
+    bool loadNew = false;


I think a widget shouldn't have the instantiate/replace logic in it -- could you add a EffectRack::loadEffect(chainslotnumber, effectslotnumber, effectid) (alongside loadNextEffect/loadPrevEffect) method, then call it here?

rryan · 2017-01-22T21:21:58Z

+          WBaseWidget(pParent),
+          m_pEffectsManager(pEffectsManager) {
+
+    setInsertPolicy(QComboBox::InsertAlphabetically);


It turns out this only applies to user-entered data, not programmatically entered data -- so you can remove it.

daschuer · 2017-01-22T22:01:49Z

moved

daschuer · 2017-01-22T22:04:46Z

Ups, I commented the wrong PR, moving my comment.

…Selector

and remove use of QPair

get rid of QPairs

Be-ing · 2017-01-26T05:42:18Z

Now that I have gone through the trouble of removing the QPairs, I understand why it is not a good idea to use them.

rryan

Looks great! LGTM other than a couple nits.

rryan · 2017-01-26T17:37:15Z

+void EffectsManager::slotBackendRegisteredEffect(EffectManifest manifest) {
+    m_availableEffectManifests.append(manifest);
+    qSort(m_availableEffectManifests.begin(), m_availableEffectManifests.end(),
+          alphabetizeEffectManifests);


To avoid having to re-sort the list you can find the right spot to insert with qLowerBound.

auto insertion_point = qLowerBound(m_availableEffectManifests.begin(), m_availableEffectManifests.end(), manifest, alphabetizeEffectManifests); m_availableEffectManifests.insert(insertion_point, manifest);

rryan · 2017-01-26T17:42:59Z

+    for (const auto& manifest : availableEffectManifests) {
+        QListWidgetItem* pItem = new QListWidgetItem();
+        pItem->setData(Qt::UserRole, manifest.id());
+        // Use short names to match order and appearance in WEffectSelector


Since this shortName/name logic appears in 3 places (the sorting function, here, and WEffectSelector) should this be a method on the manifest that is called in these places?

rryan · 2017-01-26T17:55:14Z

-const char* kDefaultMasterEqId = "none";
+// This is necessary so effect selections set to None are saved
+// and restored across restarts instead of reset to default.
+const char* kNullEffectId = "none";


Using null values for strings is kind of a Qt anti-pattern because Qt has a built-in null value for strings which is distinct from empty. I think you need this because ConfigObject::getValueString does not distinguish between null vs empty strings (and the existing code below uses it). ConfigObject::getValue (the replacement for getValueString) does distinguish between these cases (I added a test to confirm that empty entries survive across restarts here: 63a25e6)

I think instead of using this placeholder, you could use the new getValue method and everything should work. Empty strings in the user config will be left as no effect, while null entries will use the specified default.

rryan · 2017-01-27T19:14:38Z

@daschuer I think all of your concerns were addressed (?) so I'll merge this once CI is green.

daschuer · 2017-01-27T19:27:03Z

@Be-ing:
If you like, you could brake long lines, some are leading to vertical scrolling even in GitHub?

Am manual test can't hurt. Do you have a testing skin?
But I do not want to delay this ... LGTM!
Thank you for the work.

Be-ing · 2017-01-27T20:42:14Z

I'll update #1063 after this is merged.

daschuer · 2017-01-27T21:03:51Z

I have just tested it and it works nice.

I have noticed three small issues:

There is no elide when the Effect name is longer than the box. In th other effect selector we use a middle elide which works good.
The effect description is displayed as a single line tooltip, which continues outside small resolution displays. It would be nice to break them at a reasonable width.
The developer tooltips are missing.

Be-ing · 2017-01-27T21:10:05Z

There is no elide when the Effect name is longer than the box. In th other effect selector we use a middle elide which works good.

Is this because there are not translated shortNames yet? shortNames should not need eliding.

I'll look into the other issues.

daschuer · 2017-01-27T21:27:41Z

I think the box should elide in any case. This will be required for LV2 effects.

Be-ing · 2017-01-27T21:36:44Z

Okay. When LV2 support is introduced we may consider adding shortNames for them.

Middle eliding seems kinda odd. Shouldn't it elide at the end?

Be-ing · 2017-01-27T22:30:17Z

Without eliding, the width of the widget automatically expands to the width of the widest item, at least in the context of replacing the <Effect> in #1063 with <EffectSelector>. I think this is appropriate behavior. It is then up to translators to make sure displayed names are not ridiculously long.

daschuer · 2017-01-27T22:33:32Z

We cannot add names for LV2 effects, because we do not know them before.
The user could do it, but we should not force him.
The middle elide it the best solution for "Fancy Effect 1" and "Fancy Effect 2" which is the case for some LV2 effects:

This is my output of lv2ls -n:

Calf Analyzer
Calf Bass Enhancer
Calf Compensation Delay Line
Calf Compressor
Calf Deesser
Calf Emphasis
Calf Equalizer 12 Band
Calf Equalizer 5 Band
Calf Equalizer 8 Band
Calf Exciter
Calf Filter
Calf Filterclavier
Calf Flanger
Calf Fluidsynth
Calf Gate
Calf Limiter
Calf Mono Compressor
Calf Mono Input
Calf Monosynth
Calf Multi Chorus
Calf Multiband Compressor
Calf Multiband Gate
Calf Multiband Limiter
Calf Organ
Calf Phaser
Calf Pulsator
Calf Reverb
Calf Rotary Speaker
Calf Saturator
Calf Sidechain Compressor
Calf Sidechain Gate
Calf Stereo Tools
Calf Tape Simulator
Calf Transient Designer
Calf Vintage Delay
Calf Wavetable
Calf X-Over 2 Band
Calf X-Over 3 Band
Calf X-Over 4 Band
Invada Compressor (mono)
Invada Compressor (stereo)
Invada Delay Munge (mono in)
Invada Delay Munge (sum L+R in)
Invada Early Reflection Reverb (mono in)
Invada Early Reflection Reverb (sum L+R in)
Invada High Pass Filter (mono)
Invada High Pass Filter (stereo)
Invada Low Pass Filter (mono)
Invada Low Pass Filter (stereo)
Invada Input Module
Invada Meter
Invada Stereo Phaser (mono in)
Invada Stereo Phaser (stereo in)
Invada Stereo Phaser (sum L+R in)
Invada Test Tones
Invada Tube Distortion (mono)
Invada Tube Distortion (stereo)
Einfacher Verstärker
Example Metronome
Example MIDI Gate
Example Sampler
Example Scope (Mono)
Example Scope (Stereo)
A-Law Compressor
Aliasing
Allpass delay line, cubic spline interpolation
Allpass delay line, linear interpolation
Allpass delay line, noninterpolating
AM pitchshifter
Simple amplifier
Analogue Oscillator
Artificial latency
Auto phaser
Glame Bandpass Analog Filter
Glame Bandpass Filter
Bode frequency shifter
Bode frequency shifter (CV)
GLAME Butterworth Highpass
GLAME Butterworth Lowpass
Glame Butterworth X-over Filter
Chebyshev distortion
Comb Filter
Comb Splitter
Comb delay line, cubic spline interpolation
Comb delay line, linear interpolation
Comb delay line, noninterpolating
Constant Signal Generator
Crossover distortion
DC Offset Remover
Exponential signal decay
Decimator
Declipper
Simple delay line, cubic spline interpolation
Simple delay line, linear interpolation
Simple delay line, noninterpolating
Delayorama
Diode Processor
Audio Divider (Suboctave Generator)
DJ flanger
DJ EQ
DJ EQ (mono)
Dyson compressor
Fractionally Addressed Delay Line
Fast Lookahead limiter
Flanger
FM Oscillator
Foldover distortion
4 x 4 pole allpass
Fast overdrive
Frequency tracker
Gate
Giant flange
Gong model
Gong beater
GVerb
Hard Limiter
Harmonic generator
Hermes Filter
Glame Highpass Filter
Hilbert transformer
Non-bandlimited single-sample impulses
Inverter
Karaoke
L/C/R Delay
LFO Phaser
Lookahead limiter
Lookahead limiter (fixed latency)
Glame Lowpass Filter
LS Filter
Matrix: MS to Stereo
Matrix Spatialiser
Matrix: Stereo to MS
Multiband EQ
Modulatable delay
Multivoice Chorus
Higher Quality Pitch Scaler
Plate reverb
Pointer cast distortion
Rate shifter
Retro Flanger
Reverse Delay (5s max)
Ringmod with LFO
Ringmod with two inputs
Barry's Satan Maximiser
SC1
SC2
SC3
SC4
SE4
Wave shaper
Signal sifter
Sine + cosine oscillator
Single band parametric
Sinus wavewrapper
Smooth Decimator
Mono to Stereo splitter
Surround matrix encoder
State Variable Filter
Tape Delay Simulation
Transient mangler
Triple band parametric with shelves
μ-Law Compressor
Valve saturation
Valve rectifier
VyNil (Vinyl Effect)
Wave Terrain Oscillator
Crossfade
Crossfade (4 outs)
z-1
Kn0ck0ut Vocal Remover

Be-ing · 2017-01-27T22:38:09Z

Hm, the widget shows the English shortNames for effects that have them when I run Mixxx with fr_FR locale. Does every effect need a shortName declared in its manifest to let translators provide shortNames even if a shortName is not needed for that particular effect's English name?

Be-ing · 2017-01-27T22:43:11Z

-        setItemData(i, QVariant(description), Qt::ToolTipRole);
+        // The <span/> is a hack to get Qt to treat the string as rich text so
+        // it automatically wraps long lines.
+        setItemData(i, QVariant("<span/>" + description), Qt::ToolTipRole);


This wraps at a rather narrow width, but I don't think there's a way to set Qt to wrap at a wider width. It's a little awkward, but it's better than having very wide tooltips.

I tried using <div style=\"width: 450px;\"> + description + </div> but that did not change the wrapping width.

daschuer · 2017-01-27T23:17:08Z

Hm, the widget shows the English shortNames for effects that have them when I run Mixxx with fr_FR locale. Does every effect need a shortName declared in its manifest to let translators provide shortNames even if a shortName is not needed for that particular effect's English name?

Good point! I would say yes.

Did you consider to add a comment that gives a hint what short and long is? How about the proposed "XXXXX" measure?

Be-ing · 2017-01-27T23:23:08Z

Did you consider to add a comment that gives a hint what short and long is?

I'm not sure what hint we'd give. What length it would actually cut off depends on the font face, font size, and width allowed for the WEffectSelector by the skin.

How about the proposed "XXXXX" measure?

What proposal are you referring to?

Be-ing · 2017-01-27T23:26:16Z

If every effect will have a shortName, I think the new EffectManifest::displayName method can be removed and shortName can be used in its place.

daschuer · 2017-01-27T23:50:15Z

Something like this:
//: Limit the length of your translation to the rendered pixel width of "XXXXXXX"

"XXXXXXX"
"Fancy Effect" -> too long
"Fancy Ef." -> fit even if there are more charater.
"花式效果" -> Fit
"花式特殊效果" does not fit even if there are less character.

rryan · 2017-01-28T05:02:20Z

Let's continue to work on the effect name lengths and translation questions -- but let's not hold up this PR.

ronso0 · 2017-03-07T16:30:49Z

I noticed that the checkmark in opened drop-down list has enormous padding on the left/right.
This cuts off effect names which would actually fit in the closed state.

You're already very much into styling this widget; any idea how to fix that?

Be-ing · 2017-03-07T16:35:12Z

Not off the top of my head, but take a look at the QComboBox styling documentation.

Be-ing force-pushed the effect_selector_widget branch from 355bbc6 to 2b05042 Compare January 19, 2017 20:14

daschuer reviewed Jan 20, 2017

View reviewed changes

rryan added the effects label Jan 22, 2017

rryan reviewed Jan 22, 2017

View reviewed changes

Be-ing added 5 commits January 25, 2017 23:39

add effect selector widget

d7a7e99

ensure next/previous effects follow alphabetical order of short names

ff5025a

alphabetize effect list in preferences by short name to match WEffect…

d41e99e

…Selector

set tooltips for each item in WEffectSelector

456fe95

and remove use of QPair

DlgPrefEq and EffectsManager cleanup

8d6bd7b

get rid of QPairs

Be-ing force-pushed the effect_selector_widget branch from 234ded1 to 8d6bd7b Compare January 26, 2017 05:41

use const where possible

b431177

rryan requested changes Jan 26, 2017

View reviewed changes

Be-ing added 3 commits January 27, 2017 12:08

add EffectManifest::displayName method

6c40e61

don't resort entire QList to insert new EffectManfest

cff28f8

use QString::isNull instead of a placeholder string

608350c

rryan approved these changes Jan 27, 2017

View reviewed changes

break long lines

b379fdd

Be-ing added 2 commits January 27, 2017 16:41

middle elide effect displayNames in WEffectSelector

38f7f79

wrap long tooltips for WEffectSelector

9b2541b

Be-ing commented Jan 27, 2017

View reviewed changes

rryan merged commit f0c8076 into mixxxdj:master Jan 28, 2017

Be-ing deleted the effect_selector_widget branch February 6, 2017 21:29

Be-ing mentioned this pull request Feb 11, 2017

new Deere effects units with meta knobs #1063

Merged

esbrandt mentioned this pull request Jun 24, 2017

WIP Manual 2.1.x updates mixxxdj/manual#59

Merged

37 tasks

mixxxbot mentioned this pull request Aug 23, 2022

drop down list widget to select effects #8729

Closed

Uh oh!

Conversation

Be-ing commented Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Be-ing commented Jan 19, 2017

Uh oh!

daschuer commented Jan 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Be-ing Jan 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daschuer commented Jan 20, 2017

Uh oh!

Be-ing commented Jan 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Be-ing commented Jan 20, 2017

Uh oh!

rryan Jan 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daschuer commented Jan 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daschuer commented Jan 22, 2017

Uh oh!

Be-ing commented Jan 26, 2017

Uh oh!

rryan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rryan commented Jan 27, 2017

Uh oh!

daschuer commented Jan 27, 2017

Uh oh!

Be-ing commented Jan 27, 2017

Uh oh!

daschuer commented Jan 27, 2017

Uh oh!

Be-ing commented Jan 19, 2017 •

edited

Loading

Be-ing Jan 21, 2017 •

edited

Loading

Be-ing commented Jan 20, 2017 •

edited

Loading

rryan Jan 22, 2017 •

edited

Loading

daschuer commented Jan 22, 2017 •

edited

Loading

daschuer commented Jan 27, 2017 •

edited

Loading