ArduPilot / MissionPlanner

Mission Planner Ground Control Station for ArduPilot (c# .net)
http://ardupilot.org/planner/
GNU General Public License v3.0
1.8k stars 2.42k forks source link

Enhance full parameter change message box #3412

Open prathamEndu opened 2 months ago

prathamEndu commented 2 months ago

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 :

Existing message box existing message box

Updated message box updated message box

EosBandi commented 2 months ago

Did you consider what happens when 50 or 100 parameters are changed after a param load or compare ?

prathamEndu commented 2 months ago

Hmm, I see the point The text is clipped at the bottom, the box is larger than the screen size at the bottom, 'ok' button comes above the screen end.

Can you give me any idea how to tackle this issue??

here is an image of 150+ parameter change simultaneously :

Screenshot 2024-09-18 172018 Screenshot 2024-09-18 172345

prathamEndu commented 2 months ago

Is it possible to make a scrollable list or table similar to the full param table.

EosBandi commented 2 months ago

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.

robertlong13 commented 2 months ago

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.

prathamEndu commented 2 months ago

I am trying the following things:

1) Make a actual confirmation box with yes/no instead of just showing it after doing the changes. 2) I will try to make a scrollable list of changes if possible 3) if "2" doesn't work then I will show the changed param lists only if <20 or <30 params are changed at a time (because these changes are probably individually done, too many changes are usually done from some param files or new config in which case we dont need to show all the params from our side)

Lemme know your view.

prathamEndu commented 2 months ago

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.

robertlong13 commented 2 months ago

Not saying this is a bad idea, but you are aware about the "modified" checkbox which shows exactly what you want, right?

prathamEndu commented 2 months ago

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 Just as a learning exercise, I will make the changes.

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.

robertlong13 commented 2 months ago

Firstly, thanks for the info, I was not aware of that (and it does make sense), my bad.

Absolutely no worries.

anyways, this is my first time playing with mission planner code Just as a learning exercise, I will make the changes.

Yup, good first project to try out. Always good to have more people who know their way around this codebase.

prathamEndu commented 2 months ago

The following changed are done in the final commit

In a separate confirmation box for parameter changes : 1) 0 parameter changes are ignored 2) 1-20 parameter changes are shown with details (a separate loop is run for collecting these details) 3) more than 20 parameter changes are shown just as a number

In the saved parameter box : 1) 0 parameter change message updated 2) more than 0 parameter change show the count

Preview

existing message box

Fig : existing message box

less than 20 changes

Fig : less than or equal to 20 changes confirmation box

more than 20 changes

Fig : more than 20 changes confirmation box

no param change

Fig : no param change saved box

saved message

Fig : one or more param change saved box

prathamEndu commented 2 months ago

@robertlong13 and @EosBandi Can you please review the changes if you get some time.

prathamEndu commented 1 month ago

Said changes are done : 1) Redundant question removed 2) unnecessary error handling removed

prathamEndu commented 1 month ago

@EosBandi, do you notice any additional changes that might be necessary?

prathamEndu commented 1 month ago

@robertlong13 can you give this a look when free?

prathamEndu commented 1 month ago
  1. 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.

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.

  1. Even 20 might make the window a bit too big on Android. Would need to double check that.

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 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.

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?

robertlong13 commented 1 month ago

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.

You'd have to make a new form in designer. Could look at paramcompare.cs as an example, but it's not really necessary.

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?

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.

Also, who is Michael, and how do I get this reviewed by him?

He's the maintainer of Mission Planner, and the only one who can actually approve anything. My review and comments are merely symbolic.

prathamEndu commented 5 days ago

@meee1 When you have some free time, could you please review this and suggest any necessary changes?