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

Added support for dashboard links to external services. #185

Merged
merged 8 commits into from Aug 18, 2017

Conversation

Projects
None yet
5 participants
@paulianttila
Copy link
Contributor

commented Aug 12, 2017

Signed-off-by: Pauli Anttila pauli.anttila@gmail.com

Added dashboard support for links to external services.
Signed-off-by: Pauli Anttila <pauli.anttila@gmail.com>
@martinvw
Copy link
Member

left a comment

Looks great! @kaikreuzer can you also take a look?

@kaikreuzer
Copy link
Member

left a comment

Thanks, a nice feature!
Just some suggestions on how to further improve it :-)

@@ -0,0 +1,34 @@
# Dashboard tiles

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 12, 2017

Member

-> Just "Dashboard"

@@ -0,0 +1,34 @@
# Dashboard tiles

OpenHAB dashboard is landing page for the user where all openHAB UI's can be found. Dashboard support also links to external services. Links can be added to dashboard by ```conf/services/dashboard.cfg``` configuration file.

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 12, 2017

Member

-> openHAB

This comment has been minimized.

Copy link
@kaikreuzer

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 12, 2017

Member

-> to the dashboard by editing the...

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 12, 2017

Member

Please put every sentence in a new line to make diffing and reviewing easier.

|-----------------|---------|-----------------------------------------------------------------------------------------|
| link-nameX | String | Name which is shown in the openHAB dashboard. |
| link-urlX | String | URL to external service. |
| link-overlayX | String | Image overlay icon. Supported values are empty (no icon), "html5", "android" or "apple" |

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 12, 2017

Member

Actually many more. If I remember right, all icons from https://icomoon.io/#preview-free should work as an overlay.

This comment has been minimized.

Copy link
@paulianttila

paulianttila Aug 13, 2017

Author Contributor

Dashboard embedded web/fonts/icomoon.ttf seems to contain only html5, android and apple icons.

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 16, 2017

Member

I wonder if in that case this feature at all makes sense - the overlay was used in the past, but as everything ended up using the HTML5 overlay, we removed it. So it is rather in there as some legacy, which we might not want to promote again through a new feature. Wdyt?

This comment has been minimized.

Copy link
@paulianttila

paulianttila Aug 17, 2017

Author Contributor

It's totally fine for me to remove this feature. If more icons would be supported, this feature make more sense as well.

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 17, 2017

Member

Ok, so may I ask you to remove the overlay support here for the external tiles as a last change before merging? Thanks :-)

| link-overlayX | String | Image overlay icon. Supported values are empty (no icon), "html5", "android" or "apple" |
| link-imageurlX | String | URL to image. |

Where X is link unique number (see examples). All configuration parameters need to start with ```org.openhab.ui.dashboard:``` prefix.

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 12, 2017

Member

We should change the service pid from org.openhab.ui.dashboard to org.openhab.dashboard. With that change, the pid is derived from the filename and you can leave out the prefix in the file:

link-name1=openHAB Log Viewer
link-url1=http://localhost:9001
link-overlay1=
link-imageurl1=http://localhost:8080/static/image.png

Would be much nicer!

This comment has been minimized.

Copy link
@paulianttila

paulianttila Aug 13, 2017

Author Contributor

That was the missing piece. I was wondering why some of the configuration files works without prefix.


## Image URL

Image URL support several URL formats. URL support direct http links to local or Internet servers, but also the "data" URL scheme (RFC2397) is supported. See e.g. [https://www.base64-image.de](https://www.base64-image.de) to convert images to base64 coded data.

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 12, 2017

Member

Who needs access to the links? The server or the browser? Should be clarified.

## Example configuration file
```
org.openhab.ui.dashboard:link-name1=openHAB Log Viewer
org.openhab.ui.dashboard:link-url1=http://localhost:9001

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 12, 2017

Member

If the answer to my question above is "the browser", your examples are pretty misleading - "localhost" won't really get you far...

org.openhab.ui.dashboard:link-name1=openHAB Log Viewer
org.openhab.ui.dashboard:link-url1=http://localhost:9001
org.openhab.ui.dashboard:link-overlay1=
org.openhab.ui.dashboard:link-imageurl1=http://localhost:8080/static/image.png

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 12, 2017

Member

To avoid localhost, a relative url like ../static/image.png is probably better.

@@ -153,4 +162,30 @@ protected HttpServlet createServlet() {
return new DashboardServlet(configurationAdmin, indexTemplate, entryTemplate, warnTemplate, setupTemplate,
tiles);
}

private void addTilesToExternalServices(Map<String, Object> properties) {

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 12, 2017

Member

I'd prefer addTilesForExternalServices

*
* @author Pauli Anttila - Initial contribution
*/
public class DashboardTileImp implements DashboardTile {

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 12, 2017

Member

Could we fina a better class name? How about ExternalServiceTile?

Review comments changes and improved configuration file syntax.
Signed-off-by: Pauli Anttila <pauli.anttila@gmail.com>
@paulianttila

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2017

Thanks @martinvw and @kaikreuzer for review. Hopefully all review comments are now fixed. I also improved configuration file syntax.

Removed useless NumberFormatException handling as suffix is text now
Signed-off-by: Pauli Anttila <pauli.anttila@gmail.com>
@kaikreuzer
Copy link
Member

left a comment

Sorry @paulianttila, another PR just got merged which now causes merge conflicts :-(
As you have to touch it again anyhow, let me add some further minor comments. Thanks!


Browser fetch image from image URL. URL can be direct http link or data URIs according to [RFC2397](https://tools.ietf.org/html/rfc2397). If data URIs are used, browser should support them as well. All five major browsers (Chrome, Firefox, IE, Opera and Safari) support data URIs . See e.g. [https://www.base64-image.de](https://www.base64-image.de) to convert images to base64 coded data.

## Example configuration file

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 15, 2017

Member

please add an empty line after the header

@@ -0,0 +1,34 @@
# Dashboard

openHAB dashboard is landing page for the user where all openHAB UIs can be found. Dashboard support also links to external services. Links can be added to the dashboard by editing the ```conf/services/dashboard.cfg``` configuration file.

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 15, 2017

Member

Could you put every sentence in a new line?

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 15, 2017

Member

Maybe change to "The openHAB dashboar is a landing page..."

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 15, 2017

Member

You can use single backticks when highlighting code inline (no need to have three backticks).

paulianttila added some commits Aug 15, 2017

More review changes
Signed-off-by: Pauli Anttila <pauli.anttila@gmail.com>
Fixed merge conflicts
Signed-off-by: Pauli Anttila <pauli.anttila@gmail.com>
Fixed merge problem
Signed-off-by: Pauli Anttila <pauli.anttila@gmail.com>
@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Aug 18, 2017

Thanks for the update, but you missed the removal of overlays from the documentation...

Removed overlay support
Signed-off-by: Pauli Anttila <pauli.anttila@gmail.com>

@paulianttila paulianttila force-pushed the paulianttila:dashboard-links branch from 5e311de to efbb419 Aug 18, 2017

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Aug 18, 2017

Ah, I think we now have it 👍

@kaikreuzer kaikreuzer merged commit ef8d0df into openhab:master Aug 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -0,0 +1,34 @@
# Dashboard

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 18, 2017

Member

@ThomDietrich Any suggestion where this page might best fit in on docs.openhab.org and whether we should automatically pick the page up from here, or simply copy the content over to openhab-docs?

This comment has been minimized.

Copy link
@ThomDietrich

ThomDietrich Aug 19, 2017

Member

I'll open an issue for this.

@openhab-bot

This comment has been minimized.

Copy link
Collaborator

commented Nov 30, 2017

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/how-to-add-additional-links-apps-to-the-openhab-8080-start-index-page/36770/9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.