-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Conversation
plugins/FreeBoy/FreeBoy.cpp
Outdated
@@ -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" ) ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.. ?.. 🤡
|
@musikBear thanks for the information, perhaps you can help review this PR instead to make the tooltip better for users. |
@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 |
I think for 1.3 it's better to:
|
@tresf Understood
new block Proposal 1:
Proposal one has a serious flaw. This leads to final and imo best way: (only pseudo im afraid..)
Ofcause ToolTips.sid.knobDecay.hintTx would be
This approach has imo only advantages :
|
@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. |
@Sawuare I attach 10 pages of WITs. You are welcome to use or dispose as you like. I had a thought about the actual tool-tip. |
I would argue against tined tooltips:
1. If the user wants to read the short text, they're put under time
pressure.
2. If the user wants to read the long text, they have to wait.
If the short tooltips differ in length to a significant degree, a single
timer would work even worse. If we're offering two texts (which as far as I
can tell we don't want to do as we'd rather use the wiki) it makes more
sense to offer a "What's This?" in the context menu.
Speaking of the wiki & the context menu, it might make sense to offer links
to the wiki from within LMMS to replace the What's This text? If this is
implemented as a call to the default browser* then LMMS doesn't need to
access the internet itself.
*I'm not sure what to call this, but for example links on webpages can
launch Spotify (open/play track) or Discord (join server) and IIRC that can
happen in the opposite direction as well.
…On Jan 26, 2018 18:32, "musikBear" ***@***.***> wrote:
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 <https://github.com/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 <https://github.com/LMMS/lmms/files/1668581/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..
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#4128 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AIgVmsAbFuAremoI1fUizBbF6imC3ld3ks5tOgw8gaJpZM4RpdpF>
.
|
I'm sorry, but please don't back-seat code here. You're bringing this off topic. |
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. |
Closes #896 - More texts for "What is this" |
There was a problem hiding this 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.
plugins/OpulenZ/OpulenZ.cpp
Outdated
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" ) ), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
plugins/monstro/Monstro.cpp
Outdated
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" ) ), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ); |
There was a problem hiding this comment.
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")?
There was a problem hiding this comment.
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.
plugins/sf2_player/sf2_player.cpp
Outdated
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" ) ), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
plugins/sf2_player/sf2_player.cpp
Outdated
|
||
m_chorusNumKnob = new sf2Knob( this ); | ||
m_chorusNumKnob->setHintText( tr("Chorus Lines:"), "" ); | ||
m_chorusNumKnob->setHintText( tr("Lines:"), "" ); |
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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".
src/gui/LfoControllerDialog.cpp
Outdated
"bigger this value the faster the LFO oscillates and " | ||
"the faster the effect." ) ); | ||
|
||
m_speedKnob->setHintText( tr( "Modulation speed:" ), "" ); |
There was a problem hiding this comment.
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/MainWindow.cpp
Outdated
|
||
|
||
// window-toolbar | ||
ToolButton * song_editor_window = new ToolButton( | ||
embed::getIconPixmap( "songeditor" ), | ||
tr( "Show/hide Song-Editor" ) + " (F5)", | ||
tr( "Song-Editor" ) + " (F5)", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. 👍
src/gui/SetupDialog.cpp
Outdated
@@ -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" ) ); |
There was a problem hiding this comment.
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." ), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
plugins/vestige/vestige.cpp
Outdated
@@ -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..." ), |
There was a problem hiding this comment.
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.
Thanks @Wallacoloo and @SecondFlight for reviewing this. Sorry for squashing all this work into this PR. |
@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 :-) |
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:
Per #4119 (comment).
Fixes #4119 and closes #896.