Suppress repetitive MessageBoxes

[roberthudson]

This addresses @tridge 's request to show "do this for all" #2519

The following approach is used: There are 2 static classes named CustomMessageBox; one in the MissionPlanner.MsgBox namespace and one in System. Both need adjusting, as one calls the other.

-The System class has a bunch of overrides. I added an optional parameter DoThisForAllText which, when set, will display a checkbox for example "Do this for all current items" -The MissionPlanner.MsgBox class, which actually renders the messagebox will add the checkbox to the bottom of the form.

If the checkbox is selected, it will store the selection in a static bool property called DoThisForAll

[AppVeyorBot]

:white_check_mark: Build MissionPlanner 1.0.2551 completed (commit https://github.com/ArduPilot/MissionPlanner/commit/e85dd602dc by @roberthudson)

[tridge]

nice! testing now

[tridge]

does this need to be enabled on the various dialogs? I see this:

[tridge]

here are some example param files for fixed wing that trigger this: http://uav.tridgell.net/tmp/testparam/ just start sitl for a plane in MP and alternately load those two param files, it gives the dialog for out of range on the RLL2SRV params

[roberthudson]

Sorry! I somehow missed adding the custom dialog to the Full Parameter Tree. This is fixed in the March 1 commit. While I was at it, I also added the ReadOnly check to Full Parameter Tree - it was missing.

[AppVeyorBot]

:white_check_mark: Build MissionPlanner 1.0.2552 completed (commit https://github.com/ArduPilot/MissionPlanner/commit/607423c496 by @roberthudson)

[tridge]

I've tested this and it works really well now, thanks!

[tridge]

just found a small issue. After I finish loading the params when I have selected the "for all" checkbox and I then go to load a different parameter file I don't get any warning dialogs at all. I think the correct behaviour would be that the state would clear after the end of the current operation

[meee1]

yeah, having a static seems wrong here, and just prone to issues.

[AppVeyorBot]

:white_check_mark: Build MissionPlanner 1.0.2553 completed (commit https://github.com/ArduPilot/MissionPlanner/commit/553f9c78db by @roberthudson)

[roberthudson]

Several fixes in this commit....

1. if there are several missing params, the messagebox wraps and limits to (default of) 25

It used to be this ridiculous mile-wide beast

2. DoThisForAll flag resets when a new config file is loaded

3. Fixes a null error when the param grid contains an empty cell. In some cases row.Cells[1].Value could be null. changed if (row.Cells[1].Value.ToString() != value) to if (row.Cells[1].Value != null && row.Cells[1].Value.ToString() != value)

(Thanks for spotting that tridge, I'm surprised this wasn't detected long ago)

4. Added a few try/catches

[roberthudson]

yeah, having a static seems wrong here, and just prone to issues.

True, I realize it's not ideal. But what's the alternative?

Yeah it's a static, but it's just a simple boolean flag. There isn't really a thread-safety issue. And the flag gets cleared each time the messagebox is displayed.

My first idea was adding some more enums to the DialogResut (adding OKToAll, YesToAll, NoToAll)

public enum DialogResult { None, OK, Cancel, Abort, Retry, Ignore, Yes, No, OKToAll, YesToAll, NoToAll }

But that won't work. The MissionPlanner.MsgBox.CustomMessageBox returns a System enum which of course is sealed. And it would break existing code.

What do you suggest? How else is the calling function going to know the user checked "Do this for all"?

[tridge]

What do you suggest? How else is the calling function going to know the user checked "Do this for all"?

I'm not a C# dev, so this may not work, but perhaps CustomMessageBox could be subclassed to CustomMessageBoxCheckAll and the functionality and static be only in the subclass. Then we could use the subclass just in the places it makes sense, such as the param interface, without risking unexpected impact on other use cases. There seems to be over 900 uses of CustomMessageBox, and I think it would be easier to opt-in the cases that matter

[meee1]

i do agree with tridge, it should be opt-in. as the dialog itself is used everywhere, and unrelated to most likely 850 of those 900 usages

[roberthudson]

Ok, I'm getting to know Mission Planner more and more. So..... I'm rethinking this issue yet again. We're still on the wrong approach.

Here's what I suggest: -set an IsLoading flag and suppress the MessageBox while the grid is populating. Then clear the flag.

-~~then, if the user tries to change a ReadOnly cell we show the dialog.~~ Lock the cell so they can't change it. Mark the cell in orange or something.

-we can scrap the Do This For All checkbox. It's not needed

-for the Out Of Range cells, mark them in some other color as a visual cue. Maybe yellow

-after populating the grid display a dialog "n values are out of range. They have been highlighted yellow"