#2492 Open
Repository
peci1
Branch
add-uri-resolvers-default
Repository
osrf
Branch
default

Bitbucket cannot automatically merge this request.

The commits that make up this pull request have been removed.

Bitbucket cannot automatically merge this request due to conflicts.

Review the conflicts on the Overview tab. You can then either decline the request or merge it manually on your local system using the following commands:

hg update default
hg pull -r add-uri-resolvers-default https://bitbucket.org/peci1/gazebo
hg merge add-uri-resolvers-default
hg commit -m 'Merged in peci1/gazebo/add-uri-resolvers-default (pull request #2492)'
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 (14)

    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.