Skip to content

Conversation

@rubenp02
Copy link
Contributor

@rubenp02 rubenp02 commented Dec 2, 2025

Add automatic mission start/resume popups setting

Description

Added a FlyView setting to control whether mission start/resume popups appear automatically. This option is enabled by default to match the previous behavior.

Checklist:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copilot AI review requested due to automatic review settings December 2, 2025 11:26
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a new FlyView setting to control whether mission start/resume confirmation popups appear automatically. The setting (enableAutomaticMissionConfirmPopups) is enabled by default to preserve existing behavior. The implementation follows QGC's Settings Framework pattern by adding the setting definition in the header, implementation file, and JSON metadata, then exposing it in the UI and using it to conditionally show popups in the GuidedActionsController.

Key Changes:

  • Added enableAutomaticMissionConfirmPopups boolean setting to FlyViewSettings (default: true)
  • Created UI toggle in FlyViewSettings.qml for user control
  • Modified GuidedActionsController.qml to conditionally show mission start/resume popups based on the setting

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/Settings/FlyViewSettings.h Adds DEFINE_SETTINGFACT macro for the new setting
src/Settings/FlyViewSettings.cc Adds DECLARE_SETTINGSFACT macro for the setting implementation
src/Settings/FlyView.SettingsGroup.json Adds JSON metadata defining the setting's properties (type, description, default value)
src/UI/AppSettings/FlyViewSettings.qml Adds UI checkbox control to allow users to toggle the setting
src/FlightDisplay/GuidedActionsController.qml Uses the setting to conditionally show confirmation popups for mission start/resume actions

Critical Issues Found:

  • The property name in GuidedActionsController.qml is incorrect (enableAutomaticConfirmPopups instead of enableAutomaticMissionConfirmPopups), which will cause the feature to fail at runtime
  • The Fact's value is not being accessed properly (missing .rawValue accessor)
  • Minor naming inconsistency and redundant property declaration in FlyViewSettings.qml

Comment on lines 122 to 124
fact: _enableAutomaticMissionConfirmPopups
visible: _enableAutomaticMissionConfirmPopups.visible
property Fact _enableAutomaticMissionConfirmPopups: _flyViewSettings.enableAutomaticMissionConfirmPopups
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

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

This property declaration is redundant since _enableAutomaticConfirmPopups is already defined as a property at line 42 with the same value (_flyViewSettings.enableAutomaticMissionConfirmPopups). The redundant declaration inside the FactCheckBoxSlider should be removed to follow the pattern used by other similar controls in this file (e.g., line 115 for showJoystickIndicatorInToolbar, which directly uses _flyViewSettings.showJoystickIndicatorInToolbar without a local property).

Suggested change
fact: _enableAutomaticMissionConfirmPopups
visible: _enableAutomaticMissionConfirmPopups.visible
property Fact _enableAutomaticMissionConfirmPopups: _flyViewSettings.enableAutomaticMissionConfirmPopups
fact: _flyViewSettings.enableAutomaticMissionConfirmPopups
visible: _flyViewSettings.enableAutomaticMissionConfirmPopups.visible

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how you want this, @DonLakeFlyer? I did it this way for consistency with the other settings.

Copy link
Contributor

Choose a reason for hiding this comment

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

There should be a property Fact at the root level if that specifci Fact ends up being referenced by more than one control. If only reference by a single control then it should be be defined in that control.

@rubenp02 rubenp02 marked this pull request as draft December 2, 2025 12:07
Added a FlyView setting to control whether mission start/resume popups
appear automatically. This option is enabled by default to match the
previous behavior.
@rubenp02 rubenp02 force-pushed the feature/add-auto-confirm-popups-setting branch from 837087e to 1d46e13 Compare December 2, 2025 12:16
@rubenp02 rubenp02 requested a review from Copilot December 2, 2025 12:16
@rubenp02 rubenp02 changed the title Add automatic mission confirm. popups setting Add automatic mission start/resume popups setting Dec 2, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

@rubenp02 rubenp02 marked this pull request as ready for review December 2, 2025 12:20
@DonLakeFlyer
Copy link
Contributor

I've been kinda heading towards the fact that QGC exposes way to many configurable settings to the user. It's just a massive set of stuff. I'm fine with the ability of the new setting to be controlled from a custom build which wants this off all the time. But I'm not sold and this setting being exposed to the user in regular QGC.

@DonLakeFlyer
Copy link
Contributor

But I'm not sold and this setting being exposed to the user in regular QGC.

Happy to be convinced otherwise if there is a compelling reason.

@rubenp02
Copy link
Contributor Author

rubenp02 commented Dec 3, 2025

I've been kinda heading towards the fact that QGC exposes way to many configurable settings to the user. It's just a massive set of stuff. I'm fine with the ability of the new setting to be controlled from a custom build which wants this off all the time. But I'm not sold and this setting being exposed to the user in regular QGC.

Mostly agree, the settings GUI is getting cluttered. That said, I don’t think there’s really such a thing as "too much user customization". I personally view the base QGC as the complete distribution that should include everything for every possible use case, and it's the forks and custom builds' job to tone that down.

One possible solution would be to offer "basic" and "advanced" modes for the app settings. Not the current "advanced mode", which is barely a thing in base QGC and is mostly used by custom builds to hide options (that one might be better named "full mode"). I’m thinking more of a simple, global dropdown in the app settings GUI that toggles the visibility of the more "weirdly specific" settings such as this.

Another solution would be to document how to manually edit the config. files to access settings that cannot be found in the GUI, so that at least there aren't features that are implemented but not available to the user at all. If something is only there to be taken advantage of by custom builds, it might as well be fully implemented in the custom build only.

In the meantime, or if you're not convinced, I'm happy to remove the GUI side of this PR.

@DonLakeFlyer
Copy link
Contributor

One possible solution would be to offer "basic" and "advanced" modes for the app settings.

Ok, let me think about that a bit. For now this is fine then after the dual prop fixes.

@DonLakeFlyer DonLakeFlyer reopened this Dec 4, 2025
@DonLakeFlyer
Copy link
Contributor

I've changed my mind on this. Fine with the setting, but don't want to expose it in the ui. Also can you explain why you want/need this? Do you have some scenario where it's not valid. Want to understand if there is some usability problem here.

@rubenp02
Copy link
Contributor Author

rubenp02 commented Dec 7, 2025

I've changed my mind on this. Fine with the setting, but don't want to expose it in the ui. Also can you explain why you want/need this? Do you have some scenario where it's not valid. Want to understand if there is some usability problem here.

This makes sense for a mission-centric operation, and admittedly some of our clients just fly missions in full Auto, but most take-off and fly on Cruise or Guided until its time to land. Since of course there's still a mission loaded (the landing sequence) this popup shows up despite being irrelevant for this use case, and the action to launch it is quite accessible anyway. It just felt pointless for most of our clients' use cases.

@DonLakeFlyer
Copy link
Contributor

Since of course there's still a mission loaded (the landing sequence) this popup shows up despite being irrelevant for this use case

Hmm, ok. That's just a crappy experience then. I would say that should be fixed somehow for everyone. For a fixed wing, it should be pretty straightforward to know if the mission has no takeoff in it. And then in that case, don't start Start Mission. Would that cover your usage?

@rubenp02
Copy link
Contributor Author

rubenp02 commented Dec 8, 2025

Since of course there's still a mission loaded (the landing sequence) this popup shows up despite being irrelevant for this use case

Hmm, ok. That's just a crappy experience then. I would say that should be fixed somehow for everyone. For a fixed wing, it should be pretty straightforward to know if the mission has no takeoff in it. And then in that case, don't start Start Mission. Would that cover your usage?

Would cover part of it but not all, because another common use case in my experience is to take off in a mission to navigate somewhere else, perform the manual operation (pausing the mission), then resuming the mission to come back. Right now the design sort of assumes that if you've "paused" the mission in any way (changing mode, clicking "Go here", using the Pause action) you're going to be resuming it right away, which more often than not isn't the case.

@DonLakeFlyer
Copy link
Contributor

Right now the design sort of assumes that if you've "paused" the mission in any way (changing mode, clicking "Go here", using the Pause action) you're going to be resuming it right away, which more often than not isn't the case.

The I would say given that explanation, that is wrong as well. When you pause the Continue thing is crappy user experience as well for most users. Especially given it's trivial to go back into mission mode.

@DonLakeFlyer
Copy link
Contributor

What I would say is only auto-pop start mission if there is a Takeoff (for FW/MR/VTOL). And then on a pause don't auto-pop anything. For folks that that get flight modes, Continue Mission should still be in the Actions button.

@DonLakeFlyer
Copy link
Contributor

Another question: Is a problem with these popups also that the take up visual real estate from the map/video display when they are shown. Hence in the cases where they might not be quite correct, they are doubly annoying?

@rubenp02
Copy link
Contributor Author

rubenp02 commented Dec 8, 2025

Another question: Is a problem with these popups also that the take up visual real estate from the map/video display when they are shown. Hence in the cases where they might not be quite correct, they are doubly annoying?

Not really, despite the fact that we use pretty small screens. IMO it's just the fact that it pops up. A common pattern in avionics displays is that stuff only pops up or changes on its own if something's wrong. Obviously this is an user facing app but I feel like it's still a valuable general philosophy.

@DonLakeFlyer
Copy link
Contributor

A common pattern in avionics displays is that stuff only pops up or changes on its own if something's wrong.

Seems reasonable. But this pops up in response to a mission being uploaded. I get what you are saying, but if you take that to the extreme you end up with a interface which is harder to use for consumers with varying levels of experience.

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.

2 participants