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

fix a number of regressions in the flight waypoint list by changing t… #3191

Merged
merged 2 commits into from
Oct 11, 2023

Conversation

zhexu14
Copy link
Collaborator

@zhexu14 zhexu14 commented Oct 5, 2023

…he indexing and finding a work-around to blocking of signals

This PR addresses some (but not all) recently reported issues with the waypoints screen reported in Issue #3188 . This PR was tested by:

  • Changing the waypoint altitude and confirming it shows up correctly when reloading the waypoint list window and on the map
  • Adding a waypoint and confirming that it shows up immediately and persists on reload
  • Deleting a waypoint (except the first waypoint) and confirming that it is removed immediately and persists on reload,

Known issues: first waypoint (typically hold) cannot be deleted -- still looking into this one.

…he indexing and finding a work-around to blocking of signals
@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Merging #3191 (0027dbe) into develop (990f1c3) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #3191   +/-   ##
========================================
  Coverage    45.75%   45.75%           
========================================
  Files          474      474           
  Lines        25819    25819           
  Branches      4521     4521           
========================================
  Hits         11813    11813           
  Misses       13985    13985           
  Partials        21       21           

@Starfire13
Copy link
Contributor

My recollection was that deleting the non-nav waypoints would screw up fast forward interruption, so might want to check if that is still triggering fine. It might already have been fixed as I haven't deleted those in a while.

@DanAlbert
Copy link
Member

My recollection was that deleting the non-nav waypoints would screw up fast forward interruption, so might want to check if that is still triggering fine. It might already have been fixed as I haven't deleted those in a while.

That's not a bug. If you alter your flight plan it stops being whatever specific flight plan (DEAD , BARCAP, CAS, etc) and becomes just "custom" because it not longer meets the requirements of the other flight plan. We can't reason about custom flight plans.

@Starfire13
Copy link
Contributor

I realise that. My point is that it might not be worthwhile to look into implementing a way to delete the first waypoint if it does still break fast forward and we have to tell users not to do it anyway.

@DanAlbert
Copy link
Member

Not everyone uses fast forward.

For what it's worth, I agree, I don't think these things should be editable at all if it's going to break stuff, but that's been true forever and will be until #270 is done, and people complain if it breaks (that's what caused this PR, after all).

@zhexu14
Copy link
Collaborator Author

zhexu14 commented Oct 6, 2023

My recollection was that deleting the non-nav waypoints would screw up fast forward interruption, so might want to check if that is still triggering fine. It might already have been fixed as I haven't deleted those in a while.

I did some testing on this one and it seems that deleting both nav and non-nav waypoints will break fast-forward. The exception being raised though is in a different part of the code, so would probably be better captured in a different PR if we think it's worthwhile fixing.

@DanAlbert DanAlbert merged commit 32b3793 into dcs-liberation:develop Oct 11, 2023
7 checks passed
@zhexu14 zhexu14 deleted the issue_3188 branch November 2, 2023 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants