[PATCH] Ticket #423 - Import GPX file from SD card

Venkatesh Shukla IIT BHU venkatesh.shukla.eee11 at iitbhu.ac.in
Tue Mar 11 11:26:15 PDT 2014


On Sun, Mar 9, 2014 at 11:51 PM, Aurélien PRALONG <
aurelien.pralong at gmail.com> wrote:

> Hello Venkatesh,
>
> > I am waiting for Aurélien to review this and my earlier patch
> > involving addition of mappicker.
> A bit late, but here it is.
>
> This looks promising. But I found some flaws.
>
> Code comments
> -----------------------
> - Do not use normal comments ("//") on top of methods. Prefer Javadoc
> style ("/** ... */")
> - Make attributes private in classes, especially if you don't use them in
> other classes. It helps auto-complete.
> - SimpleDateFormat are not thread-safe. Do not make them final.
> Instanciate them each time it is necessary
> - I guess it is due to your editor, but why do you make "return" for each
> setter in GpxFileInfo ? They are not necessary.
> - GpxParser should not be in org.subsurface package, as it is not an
> activity
> - Do not use [Exception].printStackTrace. Never. Use Log.X("Error", e)
> instead.
> - Do not use "magic" variables, define constants (found for
> bundle.get("...") in PickLocationMap and PickGpx)
> - Keep constants and attributes on the top of the class (cf. PickGpx)
>

Reading these points, I came face to face with my ignorance.
I accept my mistakes. I am here to learn. I assure you that I will not be
making such mistakes again.
I have taken care of all these errors in the patch attached.

For code style, I refer to normal java guidelines, which are quite readable
> :
> http://www.oracle.com/technetwork/java/javase/documentation/codeconvtoc-136057.html.
> Maybe should I write a checkstyle template for the formatting stuff.
>

I have gone through these guidelines. They are indeed useful.
A template is a great idea. Do tell me if you want any help with the
documentation.


> Usability comments
> ----------------------------
> The GPX importer works well (tested with a file on
> http://www.topografix.com/gpx_sample_files.asp). But from what I see,
> "sym" should be used in place of name when it exists, as it has a more
> explicit text. Other enhancement, could you add a "check / uncheck all"
> menu ? It is easier for big files.
>
>
Firstly, the enhancement is added. That was really necessary.
Second. I have gone through many GPX files and have the following
conclusions.
    a. name attribute is probably added by the user to identify the dive.
Hence it is more identifiable to the user as a dive location
    b. sym attribute is probably the symbol by which the location is
identified on the map.
I have never owned or used any such GPS device. So my knowledge in that
field is limited.
It might be a good idea to combine both and have something like "sym +
name" shown as the dive name.

The map picker works well too, but has a problem : you do not see where you
> touched the screen. A mark should be added on map (long) click, so before
> confirming, you are sure you chose this position.
>

Yes. I forgot to add the marker. I had it before but somehow managed to
erase it.!!
The marker has been replaced in the map picker.

Other problem : I did not understand what meant "Minutes past after dive",
> until I looked in the code. I think it would be better to use a date/time
> selector for the dive time instead.
>

I was having confusions regarding this. The confusion was this.
The location I would go for a dive could be a place with a different local
time. System time tends to change accordingly in such circumstances. And
the person picking the dive time could be looking at the android clock to
decide the time of his dive.
Hence, the question arises - A time was picked by the user on a time
picker. Now what about the timezone? Should I pick the timezone of the
present location or the default timezone. This is essential as all the dive
data is stored as UTC timestamp.

So, in order to simplify things and rise above the cloud of my doubts, I
chose time difference between now and the dive to be the input given by the
user.

I would like your suggestions on this issue.

This mail is long, but with minor problems, so do not worry : the result is
> still good. Please tell me if you have questions.
>
>
Aurélien
>
>
I am ecstatic to receive your review. Thanks a lot for giving me the time
and I apologize for the delay in the reply and patch.

The questions regarding the application and code have been asked above.
Besides that, I have the following questions in my mind.
1. As I have seen there are two projects that could be taken up in the
field of android application for Subsurface in Google Summer of Code.
    a. First is a full feature port of Subsurface to android supporting
all, or as much as possible , features of the present desktop version.
    b. Implementation of libdivecomputer on android giving your phone the
ability to be connected to the divecomputer and get data from it.
Which of these is more viable for GSOC, given the time period of two
months? Which would benefit the community more and has greater necessity?
Also, I am not a diver and would not have a divecomputer with me. So, is it
possible to develop libdivecomputer for android without having a
divecomputer?

I would be grateful for any feedback and comment.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.hohndel.org/pipermail/subsurface/attachments/20140311/ba2d2ae2/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Improved-code-as-suggested-by-Aur-lien.patch
Type: text/x-patch
Size: 50031 bytes
Desc: not available
URL: <http://lists.hohndel.org/pipermail/subsurface/attachments/20140311/ba2d2ae2/attachment-0001.bin>


More information about the subsurface mailing list