Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSSTUDIO-2076 Bugfix: add a leading forward slash and replace backslashes in file-paths with forward slashes in DisplayInfo.forModel() #2866

Merged
merged 4 commits into from
Nov 17, 2023

Conversation

abrahamwolk
Copy link
Collaborator

This PR adds a leading forward slash and replaces backslashes in file-paths with forward slashes in DisplayInfo.forModel() so that filepaths are compared correctly in DisplayRuntimeInstance.trackCurrentModel().

A consequence of this is that running instances of OPIs are located when running "Execute Display" under Windows. (At the moment, they are not located, and new instances are created when running "Execute Display".)

…aths with forward slashes in DisplayInfo.forModel() so that filepaths are compared correctly in DisplayRuntimeInstance.trackCurrentModel().
@kasemir
Copy link
Collaborator

kasemir commented Nov 13, 2023

adds a leading forward slash

We access displays in two different ways. In the control room, the top_resources or home_display point to something like /controls/displays/main.bob. From outside the controls network, there's no direct file access, but display files can be read via something like http://controls.server/files/display/main.bob.
In the first case, when I later right-click on a display tab to open "Info", it will show Input: file://controls/displays/main.bob, and in the latter case, it will show Input: http://controls.server/files/display/main.bob.
Without digging back into the source code, I'm not sure where exactly the "file:" prefix is added.

Will this PR always add an initial '/'? Will it turn http://controls.server/files/display/main.bob into an invalid /http://controls.server/files/display/main.bob and file://controls/displays/main.bob into /file://controls/displays/main.bob??

@abrahamwolk
Copy link
Collaborator Author

@kasemir: I will try to test this.

@abrahamwolk
Copy link
Collaborator Author

abrahamwolk commented Nov 14, 2023

@kasemir: Indeed, it doesn't work with OPIs loaded over HTTP.

There seems to be an incongruity in the representation: on line 115 of DisplayInfo.java

return new DisplayInfo(model.getUserData(DisplayModel.USER_DATA_INPUT_FILE),

the expression model.getUserData(DisplayModel.USER_DATA_INPUT_FILE) returns a file-path (i.e., not an URI) if an OPI is located in the filesystem, while it returns an URI if the OPI is retrieved over HTTP.

Furthermore, the function Widget.getUserData() (invoked by model.getUserData() in the expression above) is essentially untyped: it retrieves the requested value based on a key from the field Widget.user_data which has the type Map<String, Object> (it is, however, instantiated by ConcurrentHashMap<String, Object>).

  1. Should we make the representation uniform for all kinds of input? I.e., always store an URI that can be retrieved by model.getUserData(DisplayModel.USER_DATA_INPUT_FILE)?
  2. Should the file-path/URI be a field instead of an entry in the map Widget.user_data?
  3. Should the type of Widget.user_data be strengthened to ConcurrentHashMap<String, Object>? Since Widget.user_data is instantiated by ConcurrentHashMap<String, Object>(), I presume that concurrency is an issue and it is probably good to enforce the type that is needed for correctness.

@kasemir
Copy link
Collaborator

kasemir commented Nov 14, 2023

I had approached this with a mindset based on display links in all the previous display tools. In the editor, you can enter a display name like "xxx". At runtime, the tool will then look for "xxx.edl", maybe along a search path. While editing the file, there is really no way to check/resolve the display name, have to simply accept whatever the user enters, because the file may be edited in a place where the linked display doesn't exist.
We still have some of that uncertainty because for a display "xxx.opi" we check if there's actually an "xxx.bob" available and then open that instead of the older "xxx.opi".
Still, in the runtime we have already resolved the linked display into an absolute path. While the link for the related display or embedded screen might have been "../sub/xxx.opi", the "Info" context menu on the runtime tab will show "file://some/path/to/sub/xxx.bob" or "http://my.site.org/displays/sub/xxx.bob". Again I don't remember off the top of my head where the "file:/.." is added, but I'm OK with putting that resolved URI into the DisplayModel.USER_DATA_INPUT_FILE. As for changing from the widget user data to a widget field, it shouldn't be a Widget field but a Display field, but then I'm not sure if that's all, or if it also needs to be an EmbeddedDisplayWidget field and NavigationTabsWidget field

@kasemir
Copy link
Collaborator

kasemir commented Nov 15, 2023

Indeed, it doesn't work with OPIs loaded over HTTP.

That's a show stopper. Will break half our CSS setups.

The ultimate solution of having true URLs everywhere might take longer.

Still, the basic replace('\\', '/') looks good, and if you add a check for "http" at the start before enforcing a leading /, that's fine as well, and could turn this into a smaller step towards the larger goal.

@georgweiss
Copy link
Collaborator

Don't forget https.

@kasemir
Copy link
Collaborator

kasemir commented Nov 15, 2023

Right, I meant literally check for starting with "http", which covers "http://" as well as "https://". OK, also a file named "http_is_part_of_my_name"...

@abrahamwolk abrahamwolk changed the title Bugfix: add a leading forward slash and replace backslashes in file-paths with forward slashes in DisplayInfo.forModel() CSSTUDIO-2076 Bugfix: add a leading forward slash and replace backslashes in file-paths with forward slashes in DisplayInfo.forModel() Nov 16, 2023
…ding '/' before replacing '\' with '/' and adding a leading '/'.
@abrahamwolk
Copy link
Collaborator Author

I have added checks for examples:, file:, http: and https: now.

I am not sure about adding file:, but I think that it may future-proof the code a little bit.

Do we support any other schemas?

@kasemir
Copy link
Collaborator

kasemir commented Nov 16, 2023

"examples:" is a good one!
I think "ftp://" would in principle also work, but don't remember using that in this millennium. The javadoc for URL mentions "jar". I doubt anybody will bundle their local displays into a jar and pack them into their site-specific product.

Protocol handlers for the following protocols are guaranteed to exist on the search path:
    http, https, file, and jar

@abrahamwolk
Copy link
Collaborator Author

It's probably better to include too many than too few schemas, so I added "ftp:" and "jar:" also.

Copy link
Collaborator

@georgweiss georgweiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please not ftp. It is history.

@abrahamwolk
Copy link
Collaborator Author

Please not ftp. It is history.

If it in principle can occur, why not add it?

@georgweiss
Copy link
Collaborator

To discourage bad habits.

@abrahamwolk
Copy link
Collaborator Author

But it is not my role to choose the protocol people use to host their OPIs.

@abrahamwolk abrahamwolk merged commit 1d3a946 into master Nov 17, 2023
2 checks passed
@abrahamwolk abrahamwolk deleted the Bugfix-FilepathsOnWindows branch November 17, 2023 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants