Skip to content
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

My changes #35

Closed
wants to merge 6 commits into from
Closed

My changes #35

wants to merge 6 commits into from

Conversation

danfos
Copy link
Contributor

@danfos danfos commented Oct 1, 2017

This branch holds 6 commits that:

  • Fix two problems I had ("negative easting and northing values" and "Clip improvement"),
  • Adds a Ctrl-F shortcut and Goto Lat/Lon Free-Format functionality
    • The Goto Lat/Lon Free-Format functionality is currently limited to one format I encountered, will update the code when I encounter different formats.
  • Fix one issue reported on github
  • Improve the code of vik_viewport_compute_bearing

I did run with the changes for quite some time, no (new) problems found.

Please consider merging them.

@rnorris
Copy link
Collaborator

rnorris commented Oct 1, 2017

The Lat/Lon Free Format looks useful.
I'd like most of the printf() removed and probably use g_debug() for the 'did NOT match' message and the 'converted to' message.
Look to use a_dialog_error_msg_extra() for the "Conversion of "%s" failed."
I can do these tweaks if you want.

Update: Just remembered in clipboard.c there is a function clip_parse_latlon() which tries to do this sort of thing. IIRC this is currently only used in creating a waypoint.
Probably more useful to make this a project reusable function e.g. put something vik_utils.c file.
Figure out what is most powerful - probably your new regex method - but maybe keep the old logic if that gets results not picked out by the regex.

Regarding Issue #25, although superficially works but it breaks any existing translations.
I plan to sort this - I think only the German and/or French ones had translations anyway - so they're languages easy to keep aligned.

The other changes look good and I will include them.
Thanks for your efforts 👍

@danfos
Copy link
Contributor Author

danfos commented Oct 1, 2017

Yes, looking at the Lat/Lon Free Format commit I did indeed forget to remove the debug messages; also a Error box for the "Conversion of "%s" failed." makes sense.

Did not know clip_parse_latlon but that indeed seems to have the same intention, so a common implementation makes sense.

It makes sense not to commit f7ccdd and update the code. I can have a look, if you want to do the tweaks also fine.

Thanks for catching the issue with the translations, did not know that.

@rnorris
Copy link
Collaborator

rnorris commented Oct 1, 2017

Regarding translations, perhaps it's doesn't break after all.
If/when the translation files get 'auto' updated, it seems it should do a fuzzy match and thus at least keep the old translation.
I was never sure how this update really worked, so previously I was more adverse to changing the core text.
There's also some more languages with translations of this than I previously thought - which is not so straightforward to guess an updated translation.
Hence I'll apply your edit.

@danfos
Copy link
Contributor Author

danfos commented Oct 9, 2017

About the Lat/Lon Free Format: Had a look at clip_parse_latlon and yes, indeed good to merge with that code.

I can put it in vik_utils.c file, but now it is in degrees_converters.c and that looks also like a logical place to me.

I am looking at wikipedia.org to see what kind of coordinate systems there are.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants