search

osrf / rvizweb

RVizWeb: RViz on the browser
285 stars 59 forks source link

Default config file is too restrictive #17

Closed chapulina closed 5 years ago

chapulina commented 5 years ago

https://github.com/osrf/rvizweb/pull/14 added a default configuration file which has some hardcoded URLs. However, when not running from localhost, like if you're inside docker, those URLs are wrong.

In fact, ros-rviz by default fills the URLs with the current location on the browser, so by itself it works, and then the config file is actually overriding the correct URLs.

I'm not sure what's the best solution here. We could just remove the default file and don't try to load it if it's an empty string. I like having the file as an example though, but I can't think of a convenient way of keeping it.

Thoughts, @jubeira ?

jubeira commented 5 years ago

Interesting observation. If the location is solved during runtime, we don't have too many options. If we want to keep the file, we could keep it with no useful content, maybe just a comment like # Insert your configuration here and catch that when loading. Then, the documentation in the README file should do the rest to show how things work. WDYT?

chapulina commented 5 years ago

How about keeping the file, but without any of the URL fields? Currently that clears the URLs though, so we'll need to add some logic not to set the URL if it's empty.

jubeira commented 5 years ago

I think it can be done; right now the operation is straightforward because it's just converting the JSON to an object. If we go that way I'd temporarily store all the fields that we would like to preserve in case the new ones are empty before overwriting them, and in case they are erased I'd restore them. Does that make sense?

Edit: the only thing I don't like about this approach is that it adds something else to maintain (i.e. "fields that we don't want overwritten in case they are empty"), but I still think it's acceptable.

chapulina commented 5 years ago

Yeah I think that could work

fields that we don't want overwritten in case they are empty

Would it make implementation simpler if that was the case for every field? That's probably the best thing to do.

jubeira commented 5 years ago

Sounds good; let's try that!