search

BirdyF / rw_tcx

Dart package to read or write TCX files
Other
7 stars 3 forks source link

Is this a working package? #1

Open gkrawiec opened 3 years ago

gkrawiec commented 3 years ago

Hi, I am learning to program using dart/flutter. I need to write tcx files in the project I chose so I've installed this package. It might be that I am doing things wrong, but I havent found where. The TCX file that the package generates hardcodes activity type cycling and a bunch of results (totaldistance and a bunch of others) and also, only 1 single data point is written, even though I add a bunch of them to the trackpoints list. Can you please let me know if the package is supposed to be used at all or if you simply stopped working on it at early beta? thank you.

gkrawiec commented 3 years ago

ok, I've figured out the single data point issue, but the hardcoded data is still there.

MrCsabaToth commented 3 years ago

ok, I've figured out the single data point issue, but the hardcoded data is still there.

I wanted to use the write portion of this package but I'm a little shocked while looking at wTCX.dart. It's full of hard-coded arbitrary values. This is not even simply the so called "magic numbers" bad pattern, but there are just unnecessarily hard-coded values...

MrCsabaToth commented 3 years ago

https://github.com/BirdyF/rw_tcx/issues/2

MrCsabaToth commented 3 years ago

ok, I've figured out the single data point issue, but the hardcoded data is still there.

I see that you forked the repo and have some bugfix. If that's a legitimate bug you can open up a Pull Request (PR) so the package owner can merge it. Before the PR open a related issue and reference it in your commit and PR by stating "Fixes #number'

gkrawiec commented 3 years ago

@MrCsabaToth Sorry, I am just learning, I don't even know how to do a PR. If you have the math done to calculate the totals somewhere else, just replace the hardcoded stuff with the variables that already exist in the model. The problem is you need code to do that math, which is completely miissing. sorry I am of no help.

MrCsabaToth commented 3 years ago

@MrCsabaToth Sorry, I am just learning, I don't even know how to do a PR.

Pull Request is a method the package owner will be able to accept any contributions (unless you are part of the project yourself). You made the first step for a PR and forked his repo. To create the PR it's super simple, just a few button clicks. You can see a "Pull Request" button when you navigate to https://github.com/gkrawiec/rw_tcx

Here are more steps, some of which you can ignore: https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/creating-a-pull-request-from-a-fork

Probably you don't need to adjust the comparison, but you'd need to describe the contribution to the package owner. However before you open any PR it has to be clear what problem does it solve. The way to document this is to open up an issue ticket in the package maintainer's repo (the one you forked, not yours), and described the problem and the proposed solution the best you can. Then you can reference that issue in your PR. I see that you have iOS/Apple related changes, if you are fixing something related to that that's welcoming, since I don't have iPhone or any Mac. If you'd fix anything related to hard-coded values, I already opened issues #2 and #3 and my PR fixes those.

If you have the math done to calculate the totals somewhere else, just replace the hard coded stuff with the variables that already exist in the model. The problem is you need code to do that math, which is completely miissing.

You are correct that since the write function get the List of the TrackPoint objects, it could calculate several statistical functions, like max speed, averages. I'll open a ticket for that and possibly include it in my PR. The only variable which maybe tricky is the total time, since during exercise there can be periods of pause, which doesn't count into the workout elapsed time, but counts into the total time (or vice-versa).

sorry I am of no help.

You can help. If the fix what you performed in your fork is about some well defined iOS issue, open an issue ticket and you can submit a PR, simple click-click-next.

MrCsabaToth commented 3 years ago

OK, added https://github.com/BirdyF/rw_tcx/issues/5

MrCsabaToth commented 3 years ago

The only thing I missing is any feedback from the package maintainer. You opened this ticket 15 days ago...

gkrawiec commented 3 years ago

@MrCsabaToth Maybe there was a misunderstanding, I didn't fix anything on this package. I just looked at it. Then I did something else to solve what I needed. Eventually I want to use the XML package to generate the TCX, but even for that I first need to understand XSD and XML which I don't right now.

As far as this package goes, from what I saw, another change could be that the string concatenations could be mostly replaced by a StringBuffer.

MrCsabaToth commented 3 years ago

I wouldn't worry about the StringBuffer right now. I found this old blog post from the days when dart was invented for Angular, dates back to 2012 https://shailen.github.io/blog/2012/11/13/string-concatenation-in-dart/ However I'd wonder how Dart fares now, because it was developed further a lot for Flutter. What is alarming is that we still haven't heard of the package maintainer. I added a #5 fix to my PR.

MrCsabaToth commented 3 years ago

Filed #6