Skip to content
This repository was archived by the owner on Mar 23, 2026. It is now read-only.

Fix Windows default browser open issue#2

Merged
ctrueden merged 1 commit intoscijava:masterfrom
panovr:fix-win10-browser-open-issue
Dec 20, 2016
Merged

Fix Windows default browser open issue#2
ctrueden merged 1 commit intoscijava:masterfrom
panovr:fix-win10-browser-open-issue

Conversation

@panovr
Copy link
Copy Markdown
Contributor

@panovr panovr commented Dec 16, 2016

See issue 34 discussion.

Copy link
Copy Markdown
Member

@ctrueden ctrueden left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I requested a couple of small changes. Please squash them into the existing commit and force push. Then we can merge it!

public void open(final URL url) throws IOException {
final String cmd;
final String args;
// FIXME: the cmd and args separate is necessary for Windows OS
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please change FIXME to NB.
The term FIXME means "the code here is not correct, and should be fixed as soon as possible." The term "NB" means "nota bene" = "note well" and indicates an important comment explaining that the code is the way it is for a very important reason, even if it might appear incorrect at first glance.

For details, see http://imagej.net/Coding_style#Javadoc_and_comments

@Override
public void open(final URL url) throws IOException {
final String cmd;
final String args;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suggest calling this arg instead of args—if there were more than one argument, it would be a String[] instead, right?

@panovr panovr force-pushed the fix-win10-browser-open-issue branch from a3a71c4 to fe3b580 Compare December 20, 2016 21:25
@panovr
Copy link
Copy Markdown
Contributor Author

panovr commented Dec 20, 2016

@ctrueden Done.

@ctrueden ctrueden merged commit 30df47b into scijava:master Dec 20, 2016
@ctrueden
Copy link
Copy Markdown
Member

Thank you! 👍

@panovr panovr deleted the fix-win10-browser-open-issue branch December 20, 2016 21:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants