magma1447 / greasemonkey-geocaching-projectgc

Adds links and data to Geocaching.com to make it collaborate with Project-GC.com
MIT License
18 stars 14 forks source link

Elevation - meters or feet is not automatic #113

Closed rteitelman closed 1 year ago

rteitelman commented 1 year ago

The elevation can be displayed in metric or imperial based on the value of variable "imperial" on line 181. Could this be done automatically without making the user change this value in the script? "Distance from home" is displayed correctly on the cache page based on the user's gc.com settings. Can the script simply look at this string on the web page and if the distance is displayed in miles, then display the elevation in feet. and if the distance is displayed in kilometers, then display the elevation in meters. ???

magma1447 commented 1 year ago

In my opinion it would be weird because of two reasons:

  1. That variable potentially can be used on pages that don't have it visible for parsing as well. It would also be one more DOM path to maintain upon web changes on Geocaching dot com.
  2. If the user wants imperial units, the user probably wants to switch on Project-GC as well. Which is possible, that's why the script receives the setting from Project-GC.

This is just my opinion. But honestly I don't care too much. I live in the world of metric, and I have metric set on both sites. So I won't get affected myself. :)

rteitelman commented 1 year ago

OK never mind on this. I just learned there is a settings dropdown to select various settings for the script. Didn't know it existed. But just using that gives the desired result.

magma1447 commented 1 year ago

I actually thought the script used the setting from Project-GC, and not a local setting.

It feels a bit stupid that the script has its own setting and isn't just using whatever setting the user has at Project-GC. I think it should be fixed into doing that. But sounds like I will have to add a flag for that into the user-object that the script receives in it's api call to Project-GC then.

I'll keep this open and try to look at the server side API tomorrow. Right now it's midnight here (almost).

PS! I am probably personally responsible for the stupid part. :)

magma1447 commented 1 year ago

A patch to the GM API has just been released. The imperialUnits boolean flag is now returned with the GetMyUsername method. The script should use this instead of having its own setting.

rteitelman commented 1 year ago

I just used that API change. Right after this line in the code: pgcUsername = result.data.username; (around line 347)

I added: imperialFlag = result.data.imperialUnits;

then using that new variable (in two places, around lines 277 and 682)) instead of the old 'imperial' setting, it seems to work fine.

We would still need to eliminate 'imperial' as a script user option in the menu.

magma1447 commented 1 year ago

An option would be to leave the setting in that menu, but with the attribute disabled set, and a mouse-over saying that the setting is imported from Project-GC. I don't recall how the code looks, if that's easy enough to do. If I recall correctly the whole menu is built from a multi-dimensional array.

Just an idea. Just removing the setting works for me as well.

rteitelman commented 1 year ago

I now have a new version 2.3.16 which fixes this issue. The fix was added to the version 2.3.15 which released yesteray. The elevation displayed on the cache page (ft. or m.) is determined by the 'use imperial units' setting in Project-GC, not by a setting in this script. That script setting is now removed.

How do I make this new file available for review?

rteitelman commented 1 year ago

jewettg gave me a short tutorial on github desktop and showed me how to proceed. I made the proposed changes, committed them and made my file available for pull. Does magma1447 see it now?

magma1447 commented 1 year ago

@rteitelman Now it looks better. You can see the diff(erence of files) that I was looking for at https://github.com/magma1447/greasemonkey-geocaching-projectgc/pull/129/files

The push is all fine if @jewettg approves it. I do have three suggestions for the future though. I will leave them as a comment on the Pull request.