#2492 Declined
Repository
peci1
Branch
add-uri-resolvers-default
Repository
osrf
Branch
default
Author
  1. Martin Pecka
Reviewers
Description
  • Added the possibility to specify additional URI resolvers based on URI scheme.

Retargeting #2479 to default. Compilation, code check and tests ok on my machine.

I also made use of the newly introduced URI class. For that, I needed to add ability to recognize absolute URIs to this class (I did it in ABI compatible way, no API changes, no test cases broken, two more added).

Comments (17)

    1. Martin Pecka author

      It's possible, but for which Gazebo version is it used? I can't find any docs on its use in Gazebo...

      And do you mean cancelling this PR, or having it here AND in ign-common?

    1. Martin Pecka author

      Hi Shane, I'd say: let's see, who's faster. This week I finally got a machine I can use for testing with various OSes, so I might be able to do it. If you start working on it, just leave here a comment, so that we don't duplicate the work...

      1. Martin Pecka author

        The original idea was to allow plugins to handle various URL protocols. The current implementation with SetFindFileURICallback seems to only support one callback, and if a plugin would want to add something, it would have to chain the callbacks. That is however impossible because there's no GetFindFileURICallback .

        So I still think the proposed way with multiple handlers has some benefits over the current version. It doesn't make sense to have both mechanisms, so I'll try to change the API and let's see how it looks. If nobody's using the callback yet, it should be okay. Am I right?

        1. Louise Poubel

          On ign-common, those functions have already been released, so removing them would take a deprecation cycle even if we don't think anyone is using them.

          I'm currently exploring the idea of supporting callbacks on gazebo::common too, I'm planning to use it with Ignition Fuel. Would something like AddFindFileCallback, which appends a callback to a vector of callbacks, work for you?

          1. Martin Pecka author

            Sounds good 😉

            SetFindFileURICallback would be the first in the chain that gets called (if it is set), and then the rest of the callbacks added through AddFindFileURICallback . This should retain the old behavior even if there would be a new plugin using the additional handlers and some older code would depend on the current behavior.

            One more design question: there is a pair of functions: *FileCallback and *FileURICallback . I first only thought about handling the URI case. Does it make some sense to also do the same for the *FileCallback?

            1. Louise Poubel

              I just saw the rest of your reply (was it there before?).

              I just went ahead and implemented only AddFindFileCallback, without Set or URI.

              I didn't add Set because it wasn't needed, but I agree that once ign-common starts being used the migration may get confusing.

              As for URI vs File, looking at the code the intended usage of each function is a bit confusing; they call each other under the hood, so in the end does it matter which one is used? I think they could eventually be consolidated into a single function.

      1. Louise Poubel

        Gazebo 9 already depends on ign-common1, but I don't think the system paths logic will be migrated to use ign-common for Gazebo 10. Gazebo 11 will use ign-common exclusively, so it will also use SystemPaths and URI from there; so unless you need this in Gazebo 9 or 10, you should be fine without this PR. Feel free to decline it if that's the case.