Hi Adrian,

your patch looks spot on! Could you just comment on the following points:

• Would it be worth having some testing instructions where the latitude is a positive, and where the longitude is negative?
• Did you notice the white space on line 125?
• I was curious about the use of sprintf in export_text_value(), could you comment here and/or in the code to explain why we don't need to format the float here?
• I'd like to see a comment in update_content which explains why you unformat the data.

Other than that, it looks perfect!

Cheers,
Fred

Thanks for the peer review and spotting those mistakes.
Submitting for integration.

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

Thanks!

Rebased and ready to go.

Uhm, I'm a bit worried about this change (that looks perfect) and how things will work after it (more changes needed).

If I'm not wrong, after current change people will get heterogeneous results here and there (on export, when searching, linking...). While old contents follow the stable (and imperfect, agree) rule of "being shown the way they were introduced", after the change people will start to get mixed results.

And the only solution I can imagine is to try to fix all existing latlong contents in DB. So I think that:

1) Some conversion - un-format - (via upgrade step) of stored latlongs is required to normalize them to use the dot decsep.

2) On search we should be also un-formatting, in order to get all the entries matching (because of 1)

3) On export, IMO it's better to apply formatting, but I'm not strong about that. Feel free to decide.

4) Any import code (restore included) must ensure proper (un-formatted) insertion.

5) On linking to external service... it depends of which decsep the service supports.

So I'm reopening this because it seems to need some more work. For sure, only for 2.5dev.

Ciao

Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

Hello Eloy,

Thanks for raising these concerns. It made me think a bit more about my solution.

Excuse me if I have this wrong, but your main concern is that old entries in the database will not be displayed correctly due to them being entered in a region specific way?

With some testing I have found that currently any entry that does not conform to the period delimiter will not enter into the database correctly i.e. -23,8764 will be stored in the database as -23. This data is already corrupted and hopefully spotted by the user who entered it in. Either way there is no way or need to 'fix' this information.

So with this in mind -

1. A conversion using an upgrade script is unnecessary.
2. I had forgotten about the search. I have changed this so that the select box shows region formatted coordinates.
3. I personally would prefer to leave this the way it is. If the file was exported with the data formatted and then restored in a moodle instance that was not set to the same sort of region then there would be import problems.
4. As all the data is already in the correct format, there is no need to change anything here.
5. All links to external services still function exactly as they did before.

I think the main point that I am trying to make here is that only figures shown to the user are formatted for the region. All functioning figures remain in the same format as before.

Thanks.

Aha, so database has always "forced" decimal point, no matter the lang used. Super, that makes things really easier. Looking...

Oh, but import entries can break that, grrr. I just exported a database, changed some "points" by "commas" and imported them back. Result = commas in the DB. See, for example, the generated, googlemaps link:

http://maps.google.com/maps?q=42,2800,+-2.2700&iwloc=A&hl=en (and it should be my city).

Anyway, I think that can be moved to another issue... and sort it out there so ignoring here...

Uhm, also, note how negatives are handled. I introduced 42,2800 -2.2700 (thinking that the negative (West) would be converted into proper positive (East). But it ended being stored in DB as negative... the activity shows it without the sign, but exports contain the sign (see link above).

Again, unrelated to this, but needing fix in another issue.

Ciao

Note this is BEFORE your patch. But means that we can have both negatives and commas in the DB.

Ah, forget my last comment, I did not realize that it was, correctly, changing the -2.2700 E to 2.2700 W (on display). So it seems that the handling of negatives is being done ok.

Oki, so:

1) We need to ensure, on import, that conversion to decimal points happen always.
2) Perhaps that should lead to an upgrade, to normalize existing decimal commas.
3) Restore may find also decimal commas, it also needs handling.

Would you create a new issue to fix all those remaining point/comma problems? I'm going to integrate this right now, so the 1-2-3 can be done on top of this. => Done, see below.

Ciao

Edited: I've created MDL-38217 to get the remaining point/comma problems under control.

Integrated (master only), thanks!

It works as expected when following the provided testing instructions.

However, if we add an entry using English with the values "49,242175 -123,11383" they are rendered as "49.0000°N, 123.0000°W", instead of "49.2422°N, 123.1138°W" when adding the entry in French. Is this expected? Shouldn't we handle both cases? If it is, please pass this test and ignore my comment.

I personally think that it works fine this way, but I'm open to suggestions about the desired functionality. Perhaps we could get a few opinions about the idea.

I personally think it's safer to use the current user locale, instead of trying to get possible decimal delimiters from "who knows where" and creating unlimited possible parsing options.

Its hard to make a judgement on this because i'm a crazy englishman who only ever uses .

1. Format_float is doing exactly what it should, being strict. This is used for grading by teachers all the time. It should not allow both, thats the whole point.
2. Is there something about coordinates which makes them special and have interchangeable seperators? If there is then we shouldn't have added it this change, since surely users would know to swap it?

But i'm willing to be corrected by someone who actually uses a different separator.

Based on fred comment, sending back to testing.

As discussed above, passing this.

Because

A
MARVELOUS
A       U
Z  YOU  P
I  ARE  E
N  PPL  R
G       B
  TNKS! 

Closing, ciao