Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove "What's This?" and update tooltips. #4128

Merged
merged 18 commits into from
Jun 5, 2018
Merged

Remove "What's This?" and update tooltips. #4128

merged 18 commits into from
Jun 5, 2018

Conversation

Sawuare
Copy link
Member

@Sawuare Sawuare commented Jan 23, 2018

i
Removes the "What's This?" feature which uses the QWhatsThis class, and removes its text.
The removed translatable strings are useful and should be considered when writing the documentation/manual.

After removing the "What's This?" feature, some widgets don't have any help text (tooltips) and some have vague tooltips, so existing tooltips will be updated and new ones will be created.
Tooltips progress:

  • Main window
  • Editors:
    • Song editor
    • Piano Roll editor
    • Automation editor
  • Time line
  • Controller Rack (LFO Controller)
  • Plug-ins (mostly instruments)
  • Track and instrument

Per #4119 (comment).
Fixes #4119 and closes #896.

@@ -467,39 +467,30 @@ FreeBoyInstrumentView::FreeBoyInstrumentView( Instrument * _instrument,
m_ch1SweepTimeKnob->setHintText( tr( "Sweep Time:" ), "" );
m_ch1SweepTimeKnob->move( 5 + 4*32, 106 );
ToolTip::add( m_ch1SweepTimeKnob, tr( "Sweep Time" ) );
m_ch1SweepTimeKnob->setWhatsThis( tr( "The amount of increase or"
" decrease in frequency" ) );

m_ch1SweepRtShiftKnob = new FreeBoyKnob( this );
m_ch1SweepRtShiftKnob->setHintText( tr( "Sweep RtShift amount:" )
, "" );
m_ch1SweepRtShiftKnob->move( 5 + 3*32, 106 );
ToolTip::add( m_ch1SweepRtShiftKnob, tr( "Sweep RtShift amount" ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tooltips are very vague, I think the whatsthis text(or a shorter version of it) should become the tooltip text.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Many tooltips will be updated, and created.

@musikBear
Copy link

FYI i made whatsthis text for all missing 1.0 component, but could not add it, because i had no ..? fork.. in a branch.. of tarballs.. ?.. 🤡

  • Git did not support xp
  • Noone was willing to do the actual update
    I still have all sentences

@tresf
Copy link
Member

tresf commented Jan 23, 2018

@musikBear thanks for the information, perhaps you can help review this PR instead to make the tooltip better for users.

@musikBear
Copy link

@tresf sure But what PR exactly do you mean? - and @everyone just say if the list of WITs is needed now, i will upload the 10 pages.

@tresf
Copy link
Member

tresf commented Jan 24, 2018

@musikBear this is a Pull Request a.k.a "pull", it has code changes that are proposed.

You can see the code changes by clicking "Files changed" tab on the top. In this case there's only one commit, so you can also comment on each change if needed. This is done by clicking the + next to the change.
Click "add single comment". Here's an example:

image

@Sawuare Sawuare changed the title Remove "What's This?". Remove "What's This?" and update tooltips. Jan 25, 2018
@Sawuare Sawuare added this to the 1.3.0 milestone Jan 25, 2018
@zonkmachine
Copy link
Member

I think for 1.3 it's better to:

  1. Just remove the info knob but keep the context menu item.
  2. Improve the strings.

@musikBear
Copy link

@tresf Understood
I must have missed this. Killing the WITs feature as such, is imo not good. But tiswhatitis.
That aside i think that in most incidences the WITs are much better than the tool-tips. My thought would then be to do this pull inverted.
That is:

  • preserve WITs text, with a tiny change
  • refurbish these changed WITs as new tool-tips
  • kill all old tool-tips
    An example:
    Current block:
Knob *dk = new sidKnob( this );
 		dk->setHintText( tr("Decay:") , "" );
 		dk->move( 7 + 28, 114 + i*50 );
-		dk->setWhatsThis( tr ( "Decay rate determines how rapidly the output "
-				"falls from the peak amplitude to the selected Sustain level." ) );

new block Proposal 1:

Knob *dk = new sidKnob( this ); 		
 		dk->move( 7 + 28, 114 + i*50 );
-		dk->setHintText( tr ( "Decay. -The rate determines how rapidly the output "
-				"falls from the peak amplitude to the selected Sustain level." ) );

Proposal one has a serious flaw.
There should newer be string-information in source-code
For that reason, i would propose a new class
public class ToolTips
This class would contain ALL tooltips for ALL features

This leads to final and imo best way: (only pseudo im afraid..)
new block Proposal 2:

Knob *dk = new sidKnob( this );
 		dk->setHintText( tr(ToolTips.sid.knobDecay.hintTx));
 		dk->move( 7 + 28, 114 + i*50 );

Ofcause ToolTips.sid.knobDecay.hintTx would be

 "Decay. -The rate determines how rapidly the output "
				"falls from the peak amplitude to the selected Sustain level."

This approach has imo only advantages :

  1. the first word has the functionality of the previous used tool-tip, so it IS a tooltip -The rest of the sentence can be ignored

  2. Reading the whole sentence has deep information value, much like WITs

  3. All strings are in one class, and are very easy to maintain and change if/ when needed.
    This makes for a cleaner source-code, and wont bloat the classes size.

@tresf
Copy link
Member

tresf commented Jan 25, 2018

@musikBear thanks. Architecturally, we'll be keeping ToolTips inside the GUI classes in which they represent. Content-wise your points are spot on. It sounds like @Sawuare has a good handle on this. Perhaps a second look once he's completed his work would be more warranted at this point.

I see a few comments about this being "Removal of functionality" and that's partially correct, but as a GUI developer for other technologies, this is a foreign concept and one that has received varying quality over the years.

I strongly agree @Sawuare that the source code is not a good place for this much text. Our software should be fairly quick and easy to translate and this is a step in that direction.

I think part of the observation here is that some of our synths are so complicated that we need a novel to use them. I don't think the CPP code is the place for this novel. We don't do this for our largest synth -- Zyn, I really don't think we should do it for the others.

@musikBear
Copy link

Architecturally, we'll be keeping ToolTips inside the GUI classes in which they represent.
Im really puzzled why you want that?
Bulky strings in source-code is bad!
I feel that my argumentation for a string-class has been presented with enough quality, that at least some real argumentation for preserving current 'in-class' structure, is in place?
Besides that, yes i will do a review asa im asked to :)

@Sawuare I attach 10 pages of WITs. You are welcome to use or dispose as you like.
They are for all main windows components
WITs Main.txt

I had a thought about the actual tool-tip.
Is it possible to 'divide' the tool-tip in two tempi?
If so, then the short real tool-tip could be shown first, and then ~2 scd later, the whole WIT-like text. Its just a thought..

@Spekular
Copy link
Member

Spekular commented Jan 26, 2018 via email

@tresf
Copy link
Member

tresf commented Jan 26, 2018

Bulky strings in source-code is bad!
I feel that my argumentation for a string-class has been presented with enough quality, that at least some real argumentation for preserving current 'in-class' structure, is in place?

I'm sorry, but please don't back-seat code here. You're bringing this off topic.

@tresf
Copy link
Member

tresf commented Jan 26, 2018

Is it possible to 'divide' the tool-tip in two tempi?

Yes, but if were were to show larger tooltips, it would make more sense to do an active help-area like Ableton does, which is really out of scope of this PR and would fit better as part of the single-window-interface initiatives.

@zonkmachine
Copy link
Member

Closes #896 - More texts for "What is this"

Copy link
Member

@Wallacoloo Wallacoloo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a notable improvement to the existing code base. Left a few suggestions, but nothing show-stopping. I'll merge this in a few days regardless of how you choose to address them.

In the future, try to break apart large PRs like this if they decompose nicely. For example, this could have been one PR which removes all the whats this messages, and a separate PR which normalizes all the label casing. That would have made things easier to review.

op2_s_mdl(14.0, 0.0, 15.0, 1.0, this, tr( "Op 2 austain" ) ),
op2_r_mdl(12.0, 0.0, 15.0, 1.0, this, tr( "Op 2 release" ) ),
op2_lvl_mdl(63.0, 0.0, 63.0, 1.0, this, tr( "Op 2 level" ) ),
op2_scale_mdl(0.0, 0.0, 3.0, 1.0, this, tr( "Op 2 level Scaling" ) ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you meant to change the case of "Scaling" here, too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

m_osc1Crs( 0.0, -24.0, 24.0, 1.0, this, tr( "Osc 1 coarse detune" ) ),
m_osc1Ftl( 0.0, -100.0, 100.0, 1.0, this, tr( "Osc 1 fine detune left" ) ),
m_osc1Ftr( 0.0, -100.0, 100.0, 1.0, this, tr( "Osc 1 fine detune right" ) ),
m_osc1Spo( 0.0, -180.0, 180.0, 0.1, this, tr( "Osc 1 stereo phase-offset" ) ),
Copy link
Member

@Wallacoloo Wallacoloo Jun 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the hyphens here are not necessary. I appreciate that you've been removing hyphens from things like "VST-plugin", and I think this is the same idea: "phase" and "offset" are separate and complete words. They don't need hyphenating.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.

@@ -76,10 +76,10 @@ PeakControllerEffectControlDialog::PeakControllerEffectControlDialog(
m_tresholdKnob->setModel( &_controls->m_tresholdModel );
m_tresholdKnob->setHintText( tr( "Treshold:" ) , "" );

m_muteLed = new LedCheckBox( "Mute Effect", this );
m_muteLed = new LedCheckBox( tr( "Mute output audio" ), this );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"output audio" seems a bit redundant. I see that in peak_controller_effect_controls.cpp we name m_muteModel "Mute output", so perhaps that would work here too (or even just "Mute")?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is m_muteModel. I'll change it accordingly.

m_chorusSpeed( FLUID_CHORUS_DEFAULT_SPEED, 0.29, 5.0, 0.01, this, tr( "Chorus Speed" ) ),
m_chorusDepth( FLUID_CHORUS_DEFAULT_DEPTH, 0, 46.0, 0.05, this, tr( "Chorus Depth" ) )
m_chorusNum( FLUID_CHORUS_DEFAULT_N, 0, 10.0, 1.0, this, tr( "Lines" ) ),
m_chorusLevel( FLUID_CHORUS_DEFAULT_LEVEL, 0, 10.0, 0.01, this, tr( "Level" ) ),
Copy link
Member

@Wallacoloo Wallacoloo Jun 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't open LMMS at the moment: is the UI clear to distinguish the reverb-related knobs from the chorus-related ones? Otherwise know we have some knobs with the same name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These texts show when the knobs are automated, so you're right, "Reverb" and "Chorus" should be kept for distinction.


m_chorusNumKnob = new sf2Knob( this );
m_chorusNumKnob->setHintText( tr("Chorus Lines:"), "" );
m_chorusNumKnob->setHintText( tr("Lines:"), "" );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the nomenclature for this usually be "voices"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. FWIW, the effects Calf Multi Chorus and Multivoice Chorus use the term "voices".

"bigger this value the faster the LFO oscillates and "
"the faster the effect." ) );

m_speedKnob->setHintText( tr( "Modulation speed:" ), "" );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand trying to preserve some uniformity between "Modulation speed" and "modulation amount", but I think this is better as "LFO speed" or "LFO frequency".

It's sort of a two-step thing: you have an LFO, and you adjust its frequency (i.e. the "LFO frequency" - not "Modulation frequency") and then you modulate some other signal with it ("Modulation amount").

src/gui/LfoControllerDialog.cpp Show resolved Hide resolved


// window-toolbar
ToolButton * song_editor_window = new ToolButton(
embed::getIconPixmap( "songeditor" ),
tr( "Show/hide Song-Editor" ) + " (F5)",
tr( "Song-Editor" ) + " (F5)",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dehyphenate "Song editor" and "Piano roll" to match "Automation editor" and "FX mixer", while you're at it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. 👍

@@ -696,17 +688,11 @@ SetupDialog::SetupDialog( ConfigTabs _tab_to_open ) :

QPushButton * autoSaveResetBtn = new QPushButton(
embed::getIconPixmap( "reload" ), "", auto_save_tw );
autoSaveResetBtn->setGeometry( 290, 70, 28, 28 );
autoSaveResetBtn->setGeometry( 320, 70, 28, 28 );
connect( autoSaveResetBtn, SIGNAL( clicked() ), this,
SLOT( resetAutoSave() ) );
ToolTip::add( autoSaveResetBtn, tr( "Reset to default-value" ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dehyphenate "default value"?

@@ -590,8 +518,7 @@ void EnvelopeAndLfoView::lfoUserWaveChanged()
if( m_params->m_userWave.frames() <= 1 )
{
TextFloat::displayMessage( tr( "Hint" ),
tr( "Drag a sample from somewhere and drop "
"it in this window." ),
tr( "Drag and drop a sample in this window." ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about "Drag and drop a sample into this window"? Because, correct me if I'm wrong, but I don't think you can drag a sample out of it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct. I'll change it.

@@ -263,7 +263,7 @@ void vestigeInstrument::loadFile( const QString & _file )
{
tf = TextFloat::displayMessage(
tr( "Loading plugin" ),
tr( "Please wait while loading VST-plugin..." ),
tr( "Please wait while loading VST plugin..." ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a "the" before "VST"? In this case it's talking about a specific VST plugin that it's loading and not just any plugin.

@Sawuare Sawuare merged commit 6d46bd4 into LMMS:master Jun 5, 2018
@Sawuare
Copy link
Member Author

Sawuare commented Jun 5, 2018

Thanks @Wallacoloo and @SecondFlight for reviewing this. Sorry for squashing all this work into this PR.

@Sawuare Sawuare deleted the WhatsThis branch June 5, 2018 22:56
@Wallacoloo
Copy link
Member

@Sawuare Thanks so much for coding these changes! I wish I could have polished the wording on that last comment about the size of the PR, but I was in a slight hurry when I wrote it. Anyway, it's not much of a bother - while smaller PRs can be more manageable, large PRs are infinitely better than no PRs :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants