-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Enhance full parameter change message box #3412
base: master
Are you sure you want to change the base?
Enhance full parameter change message box #3412
Conversation
…ed parameters with previous and new value and count of total changed parameters
Did you consider what happens when 50 or 100 parameters are changed after a param load or compare ? |
Is it possible to make a scrollable list or table similar to the full param table. |
Yes, but I think it is too much hassle. The count of changed parameters is a good enough indicator. So I recommend to drop the list. |
This confirmation should definitely come up at the "are you sure you want to write parameters" stage not the "save complete stage" I like Andras' idea of just making it a counter. Maybe in the future, that could be extended so it could optionally launch a param compare dialog so you could review the new and old values for what you're about to write. |
I am trying the following things:
Lemme know your view. |
The reason I am still insisting on the list is because I have been doing tuning and many times I feel that I need to see what I changed in current iteration. and hence, I believe it will be useful for others as well when manually changing params. |
Not saying this is a bad idea, but you are aware about the "modified" checkbox which shows exactly what you want, right? |
Firstly, thanks for the info, I was not aware of that (and it does make sense), my bad. and then, I still think getting a list in a confirmation box is a good idea (for less than 20 params). anyways, this is my first time playing with mission planner code If the final product looks useful, we can integrate it in the app. If not, we can leave it, I am totally fine with that as well. |
Absolutely no worries.
Yup, good first project to try out. Always good to have more people who know their way around this codebase. |
The following changed are done in the final commitIn a separate confirmation box for parameter changes :
In the saved parameter box :
Preview
|
@robertlong13 and @EosBandi |
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.
Line 256, there is a generic are you sure question, then you asked again... I think the first question should be removed.
MainV2.comPort.MAV.param always contains the cached param list, so it is not possible that a param to change not in the array, so error counting and display "Read faliure" message is unneccessary.
Said changes are done :
|
@EosBandi, do you notice any additional changes that might be necessary? |
@robertlong13 can you give this a look when free? |
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.
Overall I like the idea, but there are some things I don't like:
- I don't like formatting this as text in a messagebox. Comparison of the before/after should really be something like a gridview, though that'd be a lot more work for you.
- Even 20 might make the window a bit too big on Android. Would need to double check that.
I'd prefer dropping the compare from this entirely and have the popup just tell you how many parameters got changed, but I don't hate it as-is (after fixing the requested changes I made in my other comments). Michael is the authority on this though.
…sionPlanner into enhance-confirmation-box
I would be very interested in trying that if you can direct me to an example of how to implement it, or any direction for where to look.
I’m unsure how to test this on Android or even how to build and test it for that platform. Could you provide guidance on this?
I’d prefer not to drop the compare functionality entirely, as the main goal of this issue is to provide visibility into what’s being changed right before committing. The requested changes have been addressed. Also, who is Michael, and how do I get this reviewed by him? |
You'd have to make a new form in designer. Could look at
Normally I grab it from the automatic builds, but it looks like they need approval to run. You can try to run them on your own fork.
He's the maintainer of Mission Planner, and the only one who can actually approve anything. My review and comments are merely symbolic. |
@meee1 |
Enhance the message box which shows "Parameters successfully saved" confirmation box to display changed parameters with previous and new value and count of total changed parameters.
This pull in solution to the following issue : Improved Confirmation Box for Raw Parameter Setup #3404
Here are a few images for reference :