-
-
Notifications
You must be signed in to change notification settings - Fork 875
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
RC smoothing auto factor tooltip text for 4.3 #2714
Conversation
397341c
to
44332f1
Compare
This message will need version control. The existing message is correct for 4.2. We will need a different message for 4.3. Perhaps for 4.3 we could say:
|
c6a4e56
to
222d5b1
Compare
src/js/tabs/receiver.js
Outdated
@@ -631,6 +631,9 @@ TABS.receiver.initialize = function (callback) { | |||
$('.tab-receiver .rcSmoothing-auto-factor').hide(); | |||
} | |||
|
|||
const tooltip = i18n.getMessage(semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_44) ? "receiverRcSmoothingAutoFactorHelp2" : "receiverRcSmoothingAutoFactorHelp"); | |||
$('.receiverRcSmoothingAutoFactorHelp').attr('title', tooltip); |
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.
No need to attr here the tootltip for version < 1.44 since it is already defined and named in the html part.
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.
Now changed the values to be defaults using version control to minimize translation work
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.
The other numbers, where we mention higher values, won't be correct for 4.2.
I think it is better to have entirely separate tooltips for 4.2 and 4.3.
These situations arise from time to time.
We shouldn't change the 4.2 tooltip.
We probably have a similar issue with antigravity gain. In 4.2 it could be pushed to 9 or 10 and would only increase I. In 4.3 it increases P also, and has inherently higher gain, and typically should NOT be pushed to 9 or 10. I haven't had time to check all these tooltips but we will need separate tooltips, or a tooltip that explains "in 4.2, do X, in 4.3 do Y".
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.
We are free to add some extra defaults for those values 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.
This need a lotof maintain @ctzsnooze , we should use generic tooltips for all versions
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, I think it would be simpler to just have the text for the tooltip. I don't think we need the code that pulls the default value by version. Just change the tooltip, if needed, by version.
If we think about antigravity, say, at one version we make a big change, then for many versions after, most likely no further change.
But when we do make a change, it is not just the default value that changes, but perhaps the way the code works, or the tuning information.
So my feeling is that we should keep tooltips as simple as possible, but version control them so that when they change significantly, we make a new tooltip for the newer version.
We can't have the same tooltip for all versions, it would just be useless in many settings. Eg transition. Now we have jitter reduction we don't recommend transition anymore, so we should change that tooltip to explain the new situation in 4.3, and that tooltip would be different from the tooltip for transition before 4.3
Fix target help
222d5b1
to
10da45c
Compare
Tested, works correctly.
|
I have seen that we have a lot of "default" values in the tooltips for Betaflight 4.3. This that seems a good idea will be a nightmare of maintenance in the future, when something "changes" and we start recommending different values. |
@McGiverGim Agreed. We need more generic messages. Adding a defaults struct in fc.js we could make defaults independent from translated messages. @ctzsnooze adapted your latest suggestion to use a variable for default firmware version. Is the message generic enough to use for 4.2 and later firmware? |
@chmelevskij please check as it's working but I don't like / understand the console output:
|
@McGiverGim tooltips will need to change when the a parameter becomes functionally different. By that I mean, when it works a differently, or needs to be adjusted in a different way, compared to earlier versions. This is relatively infrequent, but when it happens, the user needs a tooltip that is relevant to the code they are running, and doesn't give them bad advice. Two examples in 4.3 are antigravity gain and rc smoothing auto factor. We don't need explicitly to include default values in tooltips, but it is very helpful to include advice on how to set the parameter to a range that works effectively. With Presets, a user may get parameter values that they did not enter, and may wonder if the value is at default or not. I think it would be very helpful to be able to include the default value in tooltips, or to have some other generic means to show what the default value is, so the user can check if the value is default or not. There is no simple way, otherwise, of doing this. Hence, having some method to easily display the default could be very useful. If the text for "Default: " could be referenced to a specific message, and the value derived cleverly, there would be no translation effort. However in practice, when we do make changes that require different tooltips, we will need either a singe 'combined' tooltip in the form, "for firmware after N.N, our advice is P, for earlier firmware, our advice is Q", or, we provide separate tooltips so for 4.2 "our advice is P" and for 4.3 "our advice is Q". The second seems better to me, especially where significant changes arise, and this will be the case in 4.3 for at least some parameters. |
595ccac
to
2e724d6
Compare
AUTOMERGE: (FAIL)
|
2e724d6
to
10da45c
Compare
Kudos, SonarCloud Quality Gate passed!
|
Fixes: #2711