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

Enhance full parameter change message box #3412

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

prathamEndu
Copy link

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

…ed parameters with previous and new value and count of total changed parameters
@EosBandi
Copy link
Collaborator

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

@prathamEndu
Copy link
Author

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
Copy link
Author

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

@EosBandi
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Author

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
Copy link
Collaborator

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

@prathamEndu
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Author

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

Copy link
Collaborator

@EosBandi EosBandi left a 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.

@prathamEndu
Copy link
Author

Said changes are done :

  1. Redundant question removed
  2. unnecessary error handling removed

@prathamEndu
Copy link
Author

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

@prathamEndu
Copy link
Author

@robertlong13 can you give this a look when free?

Copy link
Collaborator

@robertlong13 robertlong13 left a 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:

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

GCSViews/ConfigurationView/ConfigRawParams.cs Outdated Show resolved Hide resolved
GCSViews/ConfigurationView/ConfigRawParams.cs Outdated Show resolved Hide resolved
GCSViews/ConfigurationView/ConfigRawParams.cs Outdated Show resolved Hide resolved
GCSViews/ConfigurationView/ConfigRawParams.cs Outdated Show resolved Hide resolved
GCSViews/ConfigurationView/ConfigRawParams.cs Outdated Show resolved Hide resolved
@prathamEndu
Copy link
Author

  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
Copy link
Collaborator

robertlong13 commented Oct 14, 2024

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
Copy link
Author

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

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