Skip to content

Conversation

@haslinghuis
Copy link
Member

@haslinghuis haslinghuis commented Sep 30, 2021

Keep isDirty state consistent with firmware sliders.

Due to firmware sliders we save the current slider state to get PIDs etc loosing state in firmware until saved. So if user does not want to save we are storing and retrieving original state at tab load or using refresh.

Button Reset all profiles resets the isDirty flag so we can change to another profile while settings may have changed in the previous profile which could load saved settings to the wrong profile.

  • set isDirty after reset profile button
  • reset flags and resulting state after using refresh button.
  • reset isDirty when changes are undone.

Problems solved:

  • use isDirty and sliderRetainConfiguration flag to keep state and prevent going to another profile without saving.
  • Fixes: PID Sliders disappear on rates manual change  #2619 reworked handlers and selectors (rates tab was using same selectors)
  • disable input on RP or RPY according to sliderMode. This will users remind to save settings after changing sliderMode.
  • Enabling PID sliders respects initial firmware setting (user can use reset profile button for setting sliders to default).

@haslinghuis haslinghuis added this to the 10.8.0 milestone Sep 30, 2021
@haslinghuis haslinghuis self-assigned this Sep 30, 2021
@haslinghuis haslinghuis changed the title Prevent changing profiles is settings are not saved Prevent changing profiles if settings are not saved Sep 30, 2021
@blckmn
Copy link
Member

blckmn commented Oct 2, 2021

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> FAIL
  • assigned to a milestone -> PASS
  • cooling off period lapsed -> PASS
  • commit count less or equal to three -> PASS
  • Don't merge label NOT found -> PASS
  • at least one RN: label found -> PASS
  • Tested label found -> PASS
  • assigned to an approver -> PASS
  • approver count at least three -> FAIL

@haslinghuis haslinghuis marked this pull request as draft October 2, 2021 02:19
@haslinghuis haslinghuis marked this pull request as ready for review October 2, 2021 03:01
@haslinghuis haslinghuis marked this pull request as draft October 2, 2021 16:04
@haslinghuis haslinghuis force-pushed the fix_reset_profiles branch 4 times, most recently from 8a8ebbc to 20cae6b Compare October 2, 2021 22:39
@haslinghuis haslinghuis marked this pull request as ready for review October 2, 2021 23:12
@haslinghuis haslinghuis force-pushed the fix_reset_profiles branch 2 times, most recently from a6ca2bd to 1eb663d Compare October 3, 2021 03:15
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 3, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ctzsnooze ctzsnooze self-requested a review October 4, 2021 00:52
@ctzsnooze
Copy link
Member

ctzsnooze commented Oct 4, 2021

This works really well! Tests fine on Mac OSX.

  1. The Reset all Profile Values button now doesn't auto-save the reset... which is MUCH better behaviour. The user can now hit that button to 'check out' what those defaults would be, without immediately and permanently losing all their current settings. If they want to save the changes from the reset, they must, like any other change, hit the SAVE button if they want to save their changes. This is much more consistent behaviour and a definite improvement.

  2. When slider modes are active, clicking on the numbers achieves nothing. This is much better. If the user wants to change a number, they must turn the slider mode off, manually. Values that are not editable are shown in grey, not black.

I'd suggest one small change here, on the Mac, at least, the 'grey' for the non-editable numbers could be made more 'pale'. It didn't stand out so much. Perhaps the same paler grey as used in the PID controller settings sub-text fields?

Apart from that minor suggestion, this is great!

It is possible that, since sliders are now active by default, 'old-timer' users may not figure out why they can't edit the numbers. Perhaps some pop-up on hover over an inactive numeric field could explain that these values are controlled by sliders? Perhaps we can wait until we find out whether this is a real problem from user feedback.

Copy link
Member

@mikeller mikeller left a comment

Choose a reason for hiding this comment

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

Tested, seems to be working fine.

@mikeller mikeller merged commit 2b98947 into betaflight:master Oct 4, 2021
@mituritsyn
Copy link
Contributor

Hi guys, seems this pr has some troubles. PID and some rates fields have became inactive.
image
image

@haslinghuis
Copy link
Member Author

@mituritsyn - please set RPY to OFF and save to be able to edit PID values manual.

Somehow Rates Roll Center and Expo are also effected (so have to check how this is happening as we use classes for selection).

@haslinghuis haslinghuis deleted the fix_reset_profiles branch October 4, 2021 13:03
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.

PID Sliders disappear on rates manual change

5 participants