Skip to content

CSSTUDIO-3811 Don't call Action.resolve() on the URL in the "Open Web Page Action" action - #3986

Open
abrahamwolk wants to merge 4 commits into
masterfrom
CSSTUDIO-3811
Open

abrahamwolk wants to merge 4 commits into
masterfrom
CSSTUDIO-3811

Conversation

@abrahamwolk

Copy link
Copy Markdown
Collaborator

This pull request changes the implementation of the "Open Web Page" action in Phoebus to not call ActionUtil.resolve() on the URL that it is given when the action is run, and instead to expand macros, escape space characters, and open the resulting URL.

The motivation for this change is counterintuitive behavior observed when a URL is opened that Phoebus fails to open. I think that the core reason for the counterintuitive behavior is that the concept in Phoebus of "resolving a resource" is a Phoebus-internal concept: a resource (e.g., of the form examples:/...) may resolve internally in Phoebus, but not resolve outside of the Phoebus process. The converse can also be true: an external application (such as a web browser) may be able to resolve a resource that Phoebus cannot resolve.

As a consequence of the change implemented in this pull request, Phoebus no longer checks whether it can open a URL before opening the URL in the web browser. (I believe that this is correct, based on the reasoning in the previous paragraph.)

…Page Action Handler. Instead, expand macros, escape space characters, and open the resulting URL.
logger.log(Level.WARNING, "Open Web Page Action: URL has leading and/or trailing whitespace: \"{0}\"", url);
return;
}
else if (!(url.startsWith("http://") || url.startsWith("https://") || url.startsWith("file:"))) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe this check should be case insensitive, e.g. url.toLowerCase().startsWith...?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed: ed363d9

@sonarqubecloud

Copy link
Copy Markdown

This branch has not been deployed

No deployments
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.

2 participants