Bookmarks refactor: simplying API for adding a new portal

[nexushoratio]

Also adds jsdoc for the API being replaced.

[nexushoratio]

While, net code wise, there are now more lines, I think this new API would be easier for other plugins to use, rather than remembering how to properly build the latlng string and extract the portal name.

[github-actions]

🤖 Pull request artifacts

file	commit
IITC_Mobile-test.apk	fad6e62
test-0.38.1.20240418.080839.zip	fad6e62

See build on website

[nexushoratio]

Hmmm... apparently rebasing to pick up the recent build fixes was not the right thing to do history wise? It looks like it mixed in @modos189 's changes with mine, which was not my intent.

I've not done a lot of work with git in a collaborative setting like this, so it is not surprising if I messed something up.

Regardless of the final disposition of this PR, guidance on git protocol in this case would be appreciated. :->

[xscreach]

Hmmm... apparently rebasing to pick up the recent build fixes was not the right thing to do history wise? It looks like it mixed in @modos189 's changes with mine, which was not my intent.

I've not done a lot of work with git in a collaborative setting like this, so it is not surprising if I messed something up.

Regardless of the final disposition of this PR, guidance on git protocol in this case would be appreciated. :->

You need to do rebase with "onto" to select the starting point...

Should be smth like this: git rebase 141e2d1b2 --onto master

[modos189]

So it is possible that the portal will not be found and adding a bookmark will be quietly ignored?
In the old function the addition happens anyway. But whether it is good or bad is a question

[nexushoratio]

In the previous paths, the portal was looked prior to building the details.

In my edit, that would be lines 370 and 1180 in the original code. The first is the same lookup, unchecked. And the second is the format callback for the Portals list dialog, in which case, the marker is actually available in that context (you can tell it is a marker because it has the getLatLng() method). It could actually make sense to replace that with a call to addPortalBookmarkByMarker(portal, true);

By definition, for something to be displayed on the map, there must be an existing L.circleMarker() in window.portals. And both the sidebar and Portals list use that as a source to populate themselves. So lack of checking in existing code is not an issue.

But, for any future uses, who knows where the guid may come from? Say, someone wrote something to turn all saved key counts into bookmarks. It is likely some of those would not be currently loaded because they're not in view. So, yes, in that case, the request would be silently dropped.

The only other piece of code I found that does anything interesting when a portal cannot be found by guid is

ingress-intel-total-conversion/plugins/debug-raw-portal-data.js

Line 29 in 2b47ad4

console.warn ('Error: failed to find portal details for guid '+guid+' - failed to show debug data');

where all it does is a console.warn.

Best I can tell, nothing else uses this information to update user data.

Would something like:

else {
  raise new Error(`Could not find portal information for guid "${guid}"`);
}

be appropriate/sufficient?

[modos189]

Yes, let's do that:

1. Replace what you mentioned with addPortalBookmarkByMarker
2. Add raise new Error