Effect & effect parameter short names #1064

Be-ing · 2016-12-05T19:21:12Z

Allow effects and effect parameter manifests to define short names to be displayed in skins.
fixing https://bugs.launchpad.net/mixxx/+bug/1395245

rryan

Nice!

rryan · 2016-12-06T01:20:05Z

src/effects/native/reverbeffect.cpp

@@ -26,6 +26,7 @@ EffectManifest ReverbEffect::getManifest() {
    EffectManifestParameter* time = manifest.addParameter();
    time->setId("bandwidth");
    time->setName(QObject::tr("Bandwidth"));
+    time->setName(QObject::tr("Band"));


What do you think of "BW"?

rryan · 2016-12-06T01:20:27Z

src/widget/weffect.cpp

@@ -44,7 +44,11 @@ void WEffect::effectUpdated() {
        EffectPointer pEffect = m_pEffectSlot->getEffect();
        if (pEffect) {
            const EffectManifest& manifest = pEffect->getManifest();
-            name = manifest.name();
+            if (! manifest.shortName().isEmpty()) {


please drop the space after ! (and below)

rryan · 2016-12-06T01:22:50Z

src/widget/weffectparameterbase.cpp

+            setText(m_pEffectParameterSlot->shortName());
+        } else {
+            setText(m_pEffectParameterSlot->name());
+        }


I think it'd be nice to setBaseTooltip as something like <name>: <description> so it's easy to learn the full name. (same in WEffect)

Be-ing · 2016-12-06T03:02:00Z

src/widget/weffect.cpp

                name = manifest.shortName();
+                description = manifest.name() + ": " + manifest.description();


Should the ": " be translatable? @sblaisot, that should be " : " in French, correct?

you're right, the english ": " translates in french to " : " (in fact, the first space, before the colon, is a non-breaking space).
But I'm afraid this will be hard in transifex to translate that as the space after the colon will not be visible in the interface and it need some hints to be properly translated :(
@esbrandt what do you think ?

French typography rule is simple (this is the only simple thing in french): if the punctuation mark is made of a single part (like dot, comma, ...), there is no space before and one space after. If it is made of two parts (like colon, semicolon, ...), there is a non-breaking space before and a space after.
with the exception of parenthesis, brackets, ... which have a space outside and no space inside.

rryan · 2016-12-06T03:58:51Z

LGTM, thanks!

esbrandt · 2016-12-06T06:31:10Z

src/effects/native/reverbeffect.cpp

@@ -26,7 +26,7 @@ EffectManifest ReverbEffect::getManifest() {
    EffectManifestParameter* time = manifest.addParameter();
    time->setId("bandwidth");
    time->setName(QObject::tr("Bandwidth"));
-    time->setName(QObject::tr("Band"));
+    time->setShortName(QObject::tr("BW"));


Not really a common abbreviation. We cant expect translators to guess what this means.
Up to 11 characters are used for short name Moog Filter, can`t we use just Bandwidth then?

The transifex translation service allows to set a character limit. We can use that, and/or add a comment for translator in the code, hinting that they have $ characters max. and should use an abbreviation where appropriate.

"Moog Filter" is for a short effect name, this is for a short parameter name. There's less space for parameter names than effect names.

@esbrandt what's the latest way to add a hint to translators? Could we address it that way?

(by "it" here I mean the problem that "BW" is not a common abbreviation)

Do these annotations actually make it into transifex?
http://doc.qt.io/qt-4.8/i18n-source-translation.html#translator-comments

esbrandt · 2016-12-06T06:41:59Z

Thanks, any comment on the i18 issue/character limit?

…

On Dec 6, 2016, at 07:35, Be ***@***.***> wrote: @Be-ing commented on this pull request. In src/effects/native/reverbeffect.cpp <#1064>: > @@ -26,7 +26,7 @@ EffectManifest ReverbEffect::getManifest() { EffectManifestParameter* time = manifest.addParameter(); time->setId("bandwidth"); time->setName(QObject::tr("Bandwidth")); - time->setName(QObject::tr("Band")); + time->setShortName(QObject::tr("BW")); "Moog Filter" is for a short effect name, this is for a short parameter name. There's less space for parameter names than effect names. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1064>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEUPSZkHWPToJGBjRfVstVJdXej2O2H2ks5rFQIfgaJpZM4LEm5F>.

Be-ing · 2016-12-06T06:43:56Z

Let's revisit the character limit issue after new effects sections for skins are finalized.

daschuer · 2016-12-06T07:43:43Z

Thanks, any comment on the i18 issue/character limit?

I have the experience that a character count limit does not works well, unless you use a fixed width font. This is especially true if you allow non latin fonts.
In my professional work we check the rendered pixel width of a the critical texts.

We may consider to do this as a unit-test, but this is too late, since the original translator might be not reachable anymore. Does Transiflex support Plug-Ins in order to do this?

At least we should elide long texts to prevent the GUI from bursting at the seams.

…onventions

rryan · 2016-12-07T19:08:36Z

What about using text elision on effect / parameter name widgets? WLabel already supports this:

<Elide>right</Elide>

Be-ing · 2016-12-07T19:15:42Z

I think short names written by a human are clearer than a verbose name truncated with an ellipse.

Also, there's a preference option to turn off tooltips in the skins. Users with that option on can't see tooltips for elided names.

rryan · 2016-12-07T19:26:59Z

I think short names written by a human are clearer than a verbose name truncated with an ellipse.

Unless I'm misunderstanding, we're talking about the case where the short name written by a human (or its translation) is too long, right?

Be-ing · 2016-12-07T19:29:31Z

Unless I'm misunderstanding, we're talking about the case where the short name written by a human (or its translation) is too long, right?

Oh, I misunderstood. I thought the point of short names was to make that not happen.

rryan · 2016-12-07T19:40:28Z

Oh, I misunderstood. I thought the point of short names was to make that not happen.

Ah I see how what I said was ambiguous. I'm very much in favor of short names, but am wondering to @daschuer's point how to handle the case where the short name overflows (for example, badly translated into something verbose, or maybe the user's accessibility settings changes the font size on us). I think a character limit in Transifex might help to impose an upper bound on translations that would surely overflow but agree it probably wouldn't work in all cases.

daschuer · 2016-12-07T21:13:24Z

src/widget/weffect.cpp

-            description = manifest.description();
+            if (!manifest.shortName().isEmpty()) {
+                name = manifest.shortName();
+                description = manifest.name() + tr(": ") + manifest.description();


I think

//: Effect description %1 = effect name %2 = effect description tr("%1: %2").arg(manifest.name(), manifest.description());

is more translator friendly

daschuer · 2016-12-08T10:24:16Z

src/widget/weffect.cpp

@@ -46,7 +46,9 @@ void WEffect::effectUpdated() {
            const EffectManifest& manifest = pEffect->getManifest();
            if (!manifest.shortName().isEmpty()) {
                name = manifest.shortName();
-                description = manifest.name() + tr(": ") + manifest.description();
+                // %1 = effect name


only //: and /*: */ comments are picked up for the translator.

Be-ing · 2016-12-09T18:07:44Z

Ready to merge?

rryan · 2016-12-09T18:11:57Z

thanks!

Be-ing added 2 commits December 5, 2016 13:15

add support for effect & effect parameter short names

18ebc27

add effect & effect parameter short names

13c5398

Be-ing mentioned this pull request Dec 5, 2016

new Deere effects units with meta knobs #1063

Merged

rryan reviewed Dec 6, 2016

View reviewed changes

Be-ing added 2 commits December 5, 2016 20:59

include full effect & parameter names in tooltips for shortened names

199ed80

change reverb "Bandwidth" short name to "BW"

35bf933

Be-ing commented Dec 6, 2016

View reviewed changes

esbrandt reviewed Dec 6, 2016

View reviewed changes

make word separator with : translatable for different typographical c…

a5a2fdc

…onventions

Be-ing force-pushed the effect_short_names branch from 7a7d0c3 to a5a2fdc Compare December 7, 2016 18:39

daschuer reviewed Dec 7, 2016

View reviewed changes

make effect tooltip more translator friendly

4c315eb

daschuer reviewed Dec 8, 2016

View reviewed changes

make comments translator-friendly

2255a2d

rryan merged commit 7e81c30 into mixxxdj:master Dec 9, 2016

Be-ing deleted the effect_short_names branch February 1, 2017 01:16

Be-ing mentioned this pull request Jul 6, 2021

Fix CoreServices initalisation order #4058

Merged

mixxxbot mentioned this pull request Aug 22, 2022

effect short names #7670

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Effect & effect parameter short names #1064

Effect & effect parameter short names #1064

Be-ing commented Dec 5, 2016

rryan left a comment

rryan Dec 6, 2016

rryan Dec 6, 2016

rryan Dec 6, 2016

Be-ing Dec 6, 2016

sblaisot Dec 6, 2016 •

edited

Loading

sblaisot Dec 6, 2016

rryan commented Dec 6, 2016

esbrandt Dec 6, 2016

Be-ing Dec 6, 2016

rryan Dec 7, 2016

rryan Dec 7, 2016

esbrandt commented Dec 6, 2016 via email

Be-ing commented Dec 6, 2016

daschuer commented Dec 6, 2016

rryan commented Dec 7, 2016 •

edited

Loading

Be-ing commented Dec 7, 2016 •

edited

Loading

rryan commented Dec 7, 2016

Be-ing commented Dec 7, 2016

rryan commented Dec 7, 2016 •

edited

Loading

daschuer Dec 7, 2016

sblaisot Dec 7, 2016

daschuer Dec 8, 2016

Be-ing commented Dec 9, 2016

rryan commented Dec 9, 2016

		name = manifest.shortName();
		description = manifest.name() + ": " + manifest.description();

Effect & effect parameter short names #1064

Effect & effect parameter short names #1064

Conversation

Be-ing commented Dec 5, 2016

rryan left a 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

sblaisot Dec 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rryan commented Dec 6, 2016

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

esbrandt commented Dec 6, 2016 via email

Be-ing commented Dec 6, 2016

daschuer commented Dec 6, 2016

rryan commented Dec 7, 2016 • edited Loading

Be-ing commented Dec 7, 2016 • edited Loading

rryan commented Dec 7, 2016

Be-ing commented Dec 7, 2016

rryan commented Dec 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing commented Dec 9, 2016

rryan commented Dec 9, 2016

sblaisot Dec 6, 2016 •

edited

Loading

rryan commented Dec 7, 2016 •

edited

Loading

Be-ing commented Dec 7, 2016 •

edited

Loading

rryan commented Dec 7, 2016 •

edited

Loading