GPSBabel / gpsbabel

GPSBabel: convert, manipulate, and transfer data from GPS programs or GPS receivers. Open Source and supported on MacOS, Windows, Linux, and more. Pointy clicky GUI or a command line version...
https://www.gpsbabel.org
GNU General Public License v2.0
473 stars 126 forks source link

support optional start elements in xmlstreamwriter. #1193

Closed tsteven4 closed 10 months ago

tsteven4 commented 10 months ago

this adds methods to xmlstreamwriter to allow conditional writing of a start element. The conditional element will only be written to the stream if it has children, which is deteremined when the condtional element is ended. This simplifies writing of such elements as the writer does not need to know if it will subsequently write children or not.

This is groundwork for further possible improvements in the gpx writer extension handling. For example, we merge garmin_fs_xml_fprint and gpx_write_common_extensions, only writing the extensions when gpx garminextensions option is selected? Another example is the use of TrackPointExtension v2 instead of v1, allowing speed and course to be written in gpx 1.1. Shouldn't we make the default gpx write version 1.1, it has been out since 8/2004!

tsteven4 commented 10 months ago

I think this fixes one or more bugs related to the current necessity to check to see if we have data for every possible extension element we might want to write before we write the extensions element. Then, subsequently we also have to check if we have data for each extension element to see if we should write it. Even if there isn't a bug that dual checking is hard to maintain.

tsteven4 commented 10 months ago

@robertlipe this is groundwork for my desire to move garmin_fs_xml_fprint into GpxFormat::gpx_write_common_extensions, and only write the garmin extensions when the gpx option garminextensions is enabled. This will simplify the decision tree in GpxFormat::gpx_waypt_pr. Today we either do passthrough, or print one subset of the garmin extensions(garmin_fs_xml_fprint), or print a different subset of the garmin extensions(gpx_write_common_extensions) based on the existence of gmsd data, and the value of the garminextensions and humminbirdextensions options. For the reasons in my preceding comment I think this PR is desirable by itself, but it is also blocking my desires explained in this comment. Are you ok with this PR? You can review my potential consolidation in an upcoming PR if this PR is accepted.

robertlipe commented 10 months ago

Agreed all around. This is better than what we have, but we're heading into an unscalable tangle. I agree, unsurprisingly, that your proposed path if moving that logic into the printer and let it decide when to spill a tag.

tsteven4 commented 10 months ago

I don't see a scalability problem with the number of wpts/rtepts/trkpts. As soon as the startOptionalElement is paired with a endElement that stack is destructed. In our usage all stack usage is contained is bounded by an extensions element. And I would argue the extensions element content is finite.

GPSBabelDeveloper commented 10 months ago

Sorry. I meant Scalability in the number of special cases inside the code as the number of tags grows, not anything with the count of things involved.

On Fri, Oct 27, 2023, 3:09 PM tsteven4 @.***> wrote:

Merged #1193 https://github.com/GPSBabel/gpsbabel/pull/1193 into master.

— Reply to this email directly, view it on GitHub https://github.com/GPSBabel/gpsbabel/pull/1193#event-10795426864, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC3VAD7QR5CAEA7HVXFZWRTYBQIARAVCNFSM6AAAAAA6MRJ726VHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJQG44TKNBSGY4DMNA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

tsteven4 commented 10 months ago

See #1200.

I think it is easier to understand when we write what for waypoints, it is now simply controlled by the gpx writer extension options.

Any unrecognized tags go through tt_unknown on read. This does mean we have to catch all tags from the gpx namespace so they don't go through tt_unknown. We added some missing ones in #1191. This set is finite, but if new elements are added to gpx 1.2 they could cause problems. We might be able to guard against this by checking the namespace. It would be easy to handle this in gpx 1.1 because all the foreign stuff is within gpx extension tags. But gpx 1.0 isn't so kind.

robertlipe commented 10 months ago

Yep, that's where I knēw we were going. I like it.

It basically makes us accept the reality that there are no unknown tags in GPX for us; we have to know about them all.

I have wondered if our trick of recognizing "gpxx:" as associating as the XMlns for an extension is just wrong. Someone that's basically an indirection point, isn't it, where we should booking up gpxx and confirming that it's bound to Garmin.com/... Blah.xsd., shouldn't we?

If that's the case,. It's interesting that we've had as little. Diversity in our input as. I'd expect.

On Fri, Oct 27, 2023, 4:28 PM tsteven4 @.***> wrote:

See #1200 https://github.com/GPSBabel/gpsbabel/pull/1200.

I think it is easier to understand when we write what for waypoints, it is now simply controlled by the gpx writer extension options.

Any unrecognized tags go through tt_unknown on read. This does mean we have to catch all tags from the gpx namespace so they don't go through tt_unknown. We added some missing ones in #1191 https://github.com/GPSBabel/gpsbabel/pull/1191. This set is finite, but if new elements are added to gpx 1.2 they could cause problems. We might be able to guard against this by checking the namespace. It would be easy to handle this in gpx 1.1 because all the foreign stuff is within gpx extension tags. But gpx 1.0 isn't so kind.

— Reply to this email directly, view it on GitHub https://github.com/GPSBabel/gpsbabel/pull/1193#issuecomment-1783531016, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD37DKKVWZU6EZFRUOADYBQRO7AVCNFSM6AAAAAA6MRJ726VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBTGUZTCMBRGY . You are receiving this because you were mentioned.Message ID: @.***>

tsteven4 commented 10 months ago

Your correct, we probably still take some liberties assuming prefixes are associated with the expected namespaces. On gpx read we do attempt to map some namespaces to the expected prefix used in the tags (GpxFormat::qualifiedName()). I also suspect if we had a gpx file where the gpx namespace used a prefix we would crash and burn.

I have been considering using more of the namespace support in QXmlStreamWriter.