TomTom Integration Incomplete

[DFortun81]

Hello! I'm the author of the AllTheThings addon.

Its come to my attention that your addon overrides the global variable for TomTom, but it doesn't do it completely. TomTom has the ability to add callbacks to their waypoints via the DefaultCallbacks function. This function creates a table that you can assign minimap and world map tooltip update and show calls to when assigning to a waypoint's callbacks field.

Your replacement of the TomTom global makes this function inaccessible. Additionally, it doesn't take these callbacks into consideration when rendering the waypoints. (You can use AllTheThings' Alt+Right Click to plot something in our addon and see how it's supposed to work and then do it again with your addon enabled. Prior to my fix in 3.5.6, this would crash ATT's plotting because the function TomTom.DefaultCallbacks was inaccessible to ATT.)

As a standard practice, if you are going to override a table value such as TomTom, you should at least pass through from your table override to the table being overridden. This will make it so that when an addon like ATT tries to use a function provided by TomTom that your addon doesn't also inject, it'll fall through to TomTom's functionality.

You can do this really easily like so:

local tom = setmetatable({}, { __index = _G.TomTom }); _G.TomTom = tom; tom.AddWaypoint = Nx.TTAddWaypoint; -- .... etc

While I don't necessarily agree with ever completely overriding another addon's global variable, this is how you'd do it without hiding functionality you didn't intend to completely override.

[IrcDirk]

@DFortun81 Thanks for the feedback.

But that's not quite what I'm doing... I'm Emulating TomTom when its not installed and then and only then I'm using _G.TomTom variable plus emulation of its functions that are needed.

There is variable in Carbonite Nx.RealTom globally accessible. If its false then I'm emulating TomTom and u can make an exemption in your code not to use TomTom functions in this case.

[DFortun81]

Oh, I guess I wasn't given all the details in the bug report that prompted me to look into this then. In any case, if your implementation could at the very least introduce the DefaultCallbacks method / functionality to your version of the TomTom emulation, that'd be great.

[IrcDirk]

Sorry but i will not implement them as this is a limited emulation only. Just use the Nx.RealTom variable to check if its a real TomTom or just emulation from Carbonite.

[DFortun81]

Your emulation of TomTom should be seemless. If you are going to have Carbonite impersonate TomTom, you need to implement all of the externally facing features provided by TomTom. I shouldn't need to check to see if your addon is present when interacting with TomTom's API.

[IrcDirk]

Its not a 100% Emulation because its not needed if someone wants to have 100% then just use TomTom. Its intended just to emulate only those functions that Carbonite needs to check routes and repopulate them on Carbonite Map.

I specifically added the Nx.RealTom variable to check if its the real TomTom or not. Ill just add a PR to your repo, its not that hard to check Nx and NX.RealTom variable :)

Of course u are also welcome to write PR to my repo to add those things u want :)

[DFortun81]

I see that we're both stubborn programmers.

I could probably do a PR, I don't like that the icons contain no ATT data when mouse over'd.

[IrcDirk]

The most important thing is to be able to discuss 👍 without hate.

As to PR let me know 😃 I can make this simple PR to your repo if u like 😄

[DFortun81]

https://github.com/IrcDirk/Carbonite-Classic/pull/338

[IrcDirk]

Can we close this? :D

[IrcDirk]

Just made fix for your PR ;) Nothing big but got some errors.