-
Notifications
You must be signed in to change notification settings - Fork 127
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
Develop igc options #1180
Develop igc options #1180
Conversation
Marking as a draft as I need a bit of input. |
The igc part of this seems tortured, and doesn't appear to do anything. Instead of messing around overwriting opt_* values let's think about how the user desires are going to change the output of the igc reader. I would suggest that this could be handled in this block of existing code around set_value by
The gui is going to let the users select any of the options, including every and none, without restriction as to the pattern. It won't know any and none are exclusive, or that they make the other values irrelevant. |
What I'm really trying to do here is have EVERY and NONE be overriden by options on the CLI, so if one wants only the ENL extension, it could be specified as The problem is that I don't know if the IGC reader is aware of whether an option was passed as a CLI option or not. This so far is a half baked attempt I pushed to solicit feedback when I hit that problem. Your method will work better if changing the defaults based on options is not possible. It might make the GUI more difficult to understand, though. I'm not completely sure how it would work there. |
BOOL options with defaults will get you a value of "0" or "1". If you get the default you can't tell if the option was given or not. BOOL options without defaults get you nullptr or "1". You can't distinguish between the option not being given and the option being set to 0/n[o]. This was done so a simple boolean conversion of the value can be used, i.e. static_cast(nullptr) = false, static_cast("1") = true. BOOL options may just be listed on the command line, or set to 0, n[o], 1, y[es]. If they are just listed they are assumed to be 1. The defaults are fixed in the arguments returned by get_args(), e.g. igc_args. They are not changeable in any way, including by the value given on the command line of other options. |
The lack of a tri-state boolean option (set to false, set to true, not set) is an issue in one other identified case, #982 Providing such an option would impact both the CLI and the GUI. We haven't been rushing to provide one. |
This removes the ALL and EVERY options and makes the default options more obvious to CLI users. Most users of the CLI will be quite able to construct argument strings if the defaults are knowable, and this also reduces confusing GUI options. This should be simpler for GUI users at the expense of some complexity for CLI users. Edit to add: I'm able to build the GUI. |
igc.h
Outdated
QMap<IgcFormat::igc_ext_type_t, QString> ext_description_map = { | ||
{IgcFormat::igc_ext_type_t::ext_rec_enl, QString("Engine Noise (ENL; default=1)")}, | ||
{IgcFormat::igc_ext_type_t::ext_rec_tas, QString("True Airspeed (TAS; default=1)")}, | ||
{IgcFormat::igc_ext_type_t::ext_rec_vat, QString("Total Energy Vario (VAT; default=1)")}, | ||
{IgcFormat::igc_ext_type_t::ext_rec_oat, QString("Outside Air Temperature (OAT; default=1)")}, | ||
{IgcFormat::igc_ext_type_t::ext_rec_trt, QString("True Track (TRT; default=0)")}, | ||
{IgcFormat::igc_ext_type_t::ext_rec_gsp, QString("Ground Speed (GSP; default=1)")}, | ||
{IgcFormat::igc_ext_type_t::ext_rec_fxa, QString("Fix Accuracy (FXA; default=1)")}, | ||
{IgcFormat::igc_ext_type_t::ext_rec_gfo, QString("G Force? (GFO; default=0)")}, | ||
{IgcFormat::igc_ext_type_t::ext_rec_siu, QString("# Of Sats (SIU; default=0)")}, | ||
{IgcFormat::igc_ext_type_t::ext_rec_acz, QString("Z Acceleration (ACZ; default=1)")}, | ||
}; |
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.
Can we just place the string literals directly in igc_args?
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.
I think I'm with you here. That sounds better.
There's generally too many maps and vectors hashes and lists for my liking right now. It's something I'd like to tackle in another PR with some kind of ext_data construct that would combine ext_description_map
, ext_option_map
, and igc_extension_map
, if that works for you.
If you insist on all the new debug verbosity I have lots of comments on implementation. |
Let's not. There's a place for debuggers.
…On Mon, Sep 25, 2023, 6:17 PM tsteven4 ***@***.***> wrote:
If you insist on all the new debug verbosity I have lots of comments on
implementation.
—
Reply to this email directly, view it on GitHub
<#1180 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCSD3ZUSO6P5DZEPBBY6KLX4IGJVANCNFSM6AAAAAA42RUNZU>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
re: the debug verbosity I assume you're looking at at where igc.cc spits out a bunch of stuff at the user regarding the extensions it found? That part is meant to be greppable, so when I'm parsing a few hundred IGC files looking for which extensions are popular/accurate and therefore which ones are worth supporting, I don't need a debugger to do it and can instead write a quick shell script. I could also increase the debug level for a lot of that stuff - as long as it's machine readable, it works for me. So, yes, I insist that it's useful. Criticisms on my implementation are welcome. |
this eliminates debug messages related to the enables. messages regarding present, supported and unsupported extensions are still available. ext_description_map is eliminated. const Qt containers are used were possible. Only enabled extensions are added to the extension list. unnecessary qualification (IgcFormat::) removed. variables moved to minimum scope. we need to expand the test cases to prove at least a subset of enables work. |
This incorporates @tsteven4's patch and adds test cases for enables/disables. It builds and passes tests against QT6 but fails to build against QT5, I'm not sure why. |
This makes Qt5 happy. |
It's building and passing, thanks, I would have never figured that out. Tests fail on my desktop due to version string mismatches in help.txt and usage.txt but I expect that will resolve on its own. I'm happy with this as it stands now, if it can be merged before 1.9 is released I'm even more happy - this is basically where I wanted the IGC capabilities to be by 1.9. |
Thanks. Sorry this missed 1.9.0, but it is in now. |
The idea here is to provide the option to individually include or exclude each IGC extension, which will allow us to eliminate a bunch of #includes and static inline constexprs. It removes some IGC specific code from the KML writer. More importantly, it provides the end user the ability to reduce clutter when viewing elevation profiles.
The options will be presented in the GUI. There's a few too many QMaps IMO, so I'm open to suggestions for simplification.
I'm not sure how to handle the Codacy complaint about C-style casting.