Create default IGC extensions list and provide option to include more, or exclude defaults

[PacketFiend]

This is my initial idea. I'm sure there's a better way to implement it, but the general idea is that by specifying -i igc,include=siu or -i igc,exclude="enl;oat", for example, a user can work with a default set of included extensions, and modify that list as they see fit.

Diff here

[tsteven4]

Alternatives:

1. have a string option with a default that can be overridden. The default could be "fxa;oat;siu;" or whatever. On the command line you can override it with whatever you want. It takes more characters to specify a long list but it is clear what you get.
2. do something like the clang-tidy checks option. In this case globs only makes sense as everything.
   igc,ext=-;fxa;oat;siu disable all defaults, add fxa, oat, siu
   igc,ext=-enl;-oat,siu use defaults except no enl, no oat, and add siu.
   igc,ext= use everything
   As clang tidy does you will need to have a way to display what extensions are actually used. Perhaps this can/is done with -D1.

[robertlipe]

The usability of checkboxes , a string a bool options, is strong. Representing string options in a GUI and error checking them isn't awesome. We know that, like most other software, the defaults are very much what people use. Be mindful if we really need a dozen knobs to turn here at all. The kinds of people willing to manage/script 200 character invocations are also the ones willing to rebuild with a constexpt bool FooFlag flipped.

…

On Mon, Apr 10, 2023, 7:47 AM tsteven4 ***@***.***> wrote: Alternatives: 1. have a string option with a default that can be overridden. The default could be "fxa;oat;siu;" or whatever. On the command line you can override it with whatever you want. It takes more characters to specify a long list but it is clear what you get. 2. do something like the clang-tidy checks option <https://clang.llvm.org/extra/clang-tidy/#id2>. In this case globs only makes sense as everything. igc,ext=- *;fxa;oat;siu disable all defaults, add fxa, oat, siu igc,ext=-enl;-oat,siu use defaults except no enl, no oat, and add siu. igc,ext=* use everything As clang tidy does you will need to have a way to display what extensions are actually used. Perhaps this can/is done with -D1. — Reply to this email directly, view it on GitHub <#1061 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD36JTQJESTFDWWDOUNDXAP6MLANCNFSM6AAAAAAWYMW45I> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[tsteven4]

3. individual boolean options for each supported igc extension. Our mapping of format options to the GUI is automated, these will end up as check boxes. The string options will end up as strings with only the description to guide the user.

[PacketFiend]

3. Our mapping of format options to the GUI is automated, these will end up as check boxes.

Oh wow, neat!, I had no idea!

I'll have to play with various ways of representing the options in the code and see how it translates to the GUI. I was dreading the thought of writing anything like this. Can you point me to an example?

1. have a string option with a default that can be overridden.

Already do, see inline static const QString ext_include_default near the top of the IgcFormat class

Edit: does this automatic mapping occur at run or compile time? (can't check right now)

[tsteven4]

Options set up in igc.h in variable igc_args. gpx_args in gpx.h is a more complete example including some options with ARGTYPE_BOOL. At run time the GUI queries the CLI and basically recovers all the *_args format variable data. It builds the format option screens from that data. The filter option screens are different, they are hand crafted.

By 1. I meant a single string option shown below. Note I specified a set of default extensions. You get these if you don't enter the option on the command line at all. If you specify ext=abc;def then that overrides the default, you just get abc and def.
{
"ext", &opt_ext,
"List of extensions to include",
"enl;tas;vat;trt;gsp;fxa", ARGTYPE_STRING, ARG_NOMINMAX, nullptr
},

[PacketFiend]

This diff theoretically implements individual boolean options for each extension and provides a set of defaults. It's getting to be a lot of QHashes and QMaps, though; perhaps a QList<tuple<...>> of some sort would be more readable.

And, that's a bit of pointing and and dereferencing that's a wee bit over my head, although I'm pretty sure I understand it. It would definitely need a second look specifically at the QMap<IgcFormat::igc_ext_type_t, char**> ext_option_map and how those values are accessed.

It also doesn't apply those defaults when the IGC options dialog is opened in the GUI, although it does very smartly construct the list of arguments with a bit of pointing and clicking. That feature is quite impressive.

[tsteven4]

If we've settled on a ARGTYPE_BOOL option for every supported IGC extension read on.

I think your trying too hard. Help is available with -h. option echoing is available with -D 1. If you want -D to print both true and false use "0" defaults as well as "1" in igc_args. So if we can skip the specific debug info you added then IgcFormat::get_extension_ingestion_2 can be simplified and we don't need the description map (which may have wanted to be hashes (https://doc.qt.io/qt-6/containers.html) or just a series of if statements. It looks like further processing of the ARGTYPE_BOOL options is not implemented yet, and that may involve ext_option_map.

Perhaps I led you down this path with "As clang tidy does you will need to have a way to display what extensions are actually used. Perhaps this can/is done with -D1.". If so, sorry. Between -h and -D 1 and the manual isn't enough feedback available? If you want to expand on the help for an option in the manual then create xmldoc/formats/options/igc-ENL.xml for the ENL option, etc.

There is a subtle difference between a ARGTYPE_BOOL of nullptr and "0" or "1". WIth a nullptr default you can just test the argval for nullptr (or just an implicit conversion to bool). With "0" you need to dereference argval and compare to '0'. An example when a default is used from nukedata.cc is " if (*nukertes != '0')". This same code will dereference a nullptr if there isn't a default and the option isn't supplied or is false.

The GUI runs the CLI, the CLI doesn't know if it is being run by a user or the GUI. The GUI only passes BOOL options that aren't the default value.

`$ ./bld/gpsbabel -h igc

    igc                   FAI/IGC Flight Recorder Data Format
      timeadj               (integer sec or 'auto') Barograph to GPS time diff
      ENL                   (0/1) Engine Noise (ENL)
      TAS                   (0/1) True Airspeed (TAS)
      VAT                   (0/1) Total Energy Vario (VAT)
      OAT                   (0/1) Outside Air Temperature (OAT)
      TRT                   (0/1) True Track (TRT)
      GSP                   (0/1) Ground Speed (GSP)
      FXA                   (0/1) Fix Accuracy (FXA)
      SIU                   (0/1) # Of Sats (SIU)
      ACZ                   (0/1) Z Acceleration (ACZ)
      GFO                   (0/1) G Force? (GFO)

tsteven4@PEDALDAMNIT:~/work/pf2$ ./bld/gpsbabel -D 2 -i igc -f reference/track/92HV66G1.igc
GPSBabel Version: 1.8.0
main: Compiled with Qt 6.5.0 for architecture x86_64-little_endian-lp64
main: Running with Qt 6.5.0 on Ubuntu 22.04.2 LTS, x86_64
main: QLocale::system() is C
main: QLocale() is C
main: QTextCodec::codecForLocale() is UTF-8, mib 106
options: module/option=value: igc/ENL="1" (=default)
options: module/option=value: igc/TAS="1" (=default)
options: module/option=value: igc/VAT="1" (=default)
options: module/option=value: igc/OAT="1" (=default)
options: module/option=value: igc/GSP="1" (=default)
options: module/option=value: igc/FXA="1" (=default)
options: module/option=value: igc/ACZ="1" (=default)
IGC: Extension flags:
IGC: Extension: 1; Description: Engine Noise (ENL); Value: True
IGC: Extension: 2; Description: True Airspeed (TAS); Value: True
IGC: Extension: 3; Description: Total Energy Vario (VAT); Value: True
IGC: Extension: 4; Description: Outside Air Temperature (OAT); Value: True
IGC: Extension: 5; Description: True Track (TRT); Value: False
IGC: Extension: 6; Description: Ground Speed (GSP); Value: True
IGC: Extension: 7; Description: Fix Accuracy (FXA); Value: True
IGC: Extension: 8; Description: G Force? (GFO); Value: False
IGC: Extension: 9; Description: # Of Sats (SIU); Value: False
IGC: Extension: 10; Description: Z Acceleration (ACZ); Value: True
IGC: I record: I083638FXA3941ENL4246TAS4751GSP5254TRT5559VAT6063OAT6467ACZ`

[PacketFiend]

You're reading a bit too much into the debug output in get_extension_ingestion_2(), I think. What's there right now is just there to make sure everything works right; as you noticed, those options aren't actually used yet. I just needed a quick and dirty way to make sure I was referencing and dereferencing everything correctly. Basically, just ignore everything in that function for now, it's just a stub to test and demonstrate usage

I like the general idea of ext_description_map because it's one place to change those strings if they get used in multiple places, which I assume will make translations and documentation easier. A QMap may well not be the best way to implement it, though.

I like the bool options because as Robert noted, this presents checkboxes in the GUI which are far easier to interact with and test than constructing a string.

[PacketFiend]

This is my next crack at this. It needs quite a bit of cleanup still, but it generally works.

With these options, we can eliminate special cases for extensions like TRT and SIU and simply default them to off. Then the end user can decide if they really want to turn that knob on.

There's got to be a better way to do this, though:

for (auto& arg : igc_args) {
  bool opt = (*arg.argval != nullptr && **arg.argval == '1');