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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added functionality to open openHAB Preview on macOS based systems #14

Merged
merged 1 commit into from
Jun 21, 2017

Conversation

dennisausbremen
Copy link
Contributor

@dennisausbremen dennisausbremen commented Jun 20, 2017

Unfortunately I'm not able to test on Linux atm.
Since there seems to be no "configurability" in VSCode extensions a lot has to be hardcoded. 馃槥

Closes #15

@kubawolanin
Copy link
Collaborator

Hi @dennisausbremen! Thank you for your contribution.

Since there seems to be no "configurability" in VSCode extensions a lot has to be hardcoded

In fact, there is a way to provide configuration points for the extension. Please take a look:

https://code.visualstudio.com/docs/extensionAPI/extension-points#_contributesconfiguration

All we need to do is to add a "configuration" node within "contributes" in package.json:
https://github.com/openhab/openhab-vscode/blob/master/package.json#L27

    "configuration": {
        "type": "object",
        "title": "openHAB configuration",
        "properties": {
            "openhab.hostname": {
                "type": "string",
                "default": null, // or 'localhost' ?
                "description": "Hostname or IP address of openHAB instance."
            }
        }
    }

And then in the code you would just use this config like this:

let config = vscode.workspace.getConfiguration('openhab')
console.log(config.hostname) // localhost or openhabianpi etc.

A few remarks, though:

  • We'll need to parse the value in case user provides it with protocol and/or port, e.g. http://home or home:8080. - What we need is home only :-) We will reuse that in further features, like REST connection mentioned in Code completion for Items, Things etc.聽#7
  • README.md file would need to change as well as we're introducing a configuration point here.

Thanks again!

src/extension.ts Outdated
}

const getFileName = () => {
return (os === 'win32') ? absolutePath.split('\\').pop() : absolutePath.split('/').pop()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a thought. We could utilize Node.js path library so it does the detection for us :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it was late yesterday. 馃槃
Will do.
Thanks for the input! 馃帀

@dennisausbremen
Copy link
Contributor Author

dennisausbremen commented Jun 21, 2017

In fact, there is a way to provide configuration points for the extension. Please take a look

I think i just missed that. Great. 馃槅 馃憤

The main problem with the parsing of the correct path is, that it just works somewhat great on windows. 馃槥
Maybe we should abandon the auto-detection alltogether or make it optional or detect if the user is using "win32" to automatically turn it on, since it's of no real use on a mac or *NIX machine.

We'll need to parse the value in case user provides it with protocol and/or port
Using path() that should be no problem. 馃槈

README.md file would need to change as well as we're introducing a configuration point here.
I agree. 馃憤

@kubawolanin
Copy link
Collaborator

Maybe we should abandon the auto-detection alltogether

I'm perfectly fine with this. Let's just make sure that hostname provided in the user settings could be reusable.

Cheers! 馃槂

@dennisausbremen
Copy link
Contributor Author

Updated my code to accomodate your feedback.

Copy link
Collaborator

@kubawolanin kubawolanin left a comment

Choose a reason for hiding this comment

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

Awesome @dennisausbremen , thank you so much for your enhancements! 馃

* openhab.host (mandatory), default: openhabianpi
* openhab.port (optional), default: 8080

These settings should work fine on Windows machines and openHAB installations using the recommended [openHABian](http://docs.openhab.org/installation/openhabian.html) setup.
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 for openHABian recognition! :-)

Copy link
Member

Choose a reason for hiding this comment

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

+1 Indeed 馃槃

@kubawolanin
Copy link
Collaborator

@dennisausbremen your changes LGTM overall. There's one thing that's needed from you before I merge it.

Can you squash your commits and change commit message to include a Signed-off-by (to certify that you wrote it or otherwise have the right to pass it on as an open-source), see http://docs.openhab.org/developers/contributing/contributing.html#sign-your-work

For squashing / rebasing see:
http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

Also, if you edit your first comment with Closes #15 annotation, it'll automatically refer to and close the issue you've created. Thanks!

Added configuration for openhab.host and openhab.path
Updated README.md to accomodate new configuration options (including examples)

Signed-off-by: Dennis Gieseler <dennis.gieseler@me.com> (github: dennisausbremen)
@dennisausbremen
Copy link
Contributor Author

dennisausbremen commented Jun 21, 2017

@kubawolanin: Like this? 馃槃
Whatever that grey github thingie means.

@kubawolanin kubawolanin merged commit 70801c4 into openhab:master Jun 21, 2017
@kubawolanin
Copy link
Collaborator

perfecto! Thanks again mate!

@dennisausbremen
Copy link
Contributor Author

No problem! 馃嵏馃槑馃憤

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.

None yet

3 participants