Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Add support for Image items in Sitemaps and Classic+Basic UI #1800

Closed
digitaldan opened this issue Jul 4, 2016 · 105 comments
Closed

Add support for Image items in Sitemaps and Classic+Basic UI #1800

digitaldan opened this issue Jul 4, 2016 · 105 comments

Comments

@digitaldan
Copy link
Contributor

Continuing the discussion from https://community.openhab.org/t/squeezebox-display-coverart/12072/4

The Squeezebox binding implemented image support (coverart) by retrieving the image from a URL and converting the raw image data to a RawType. Is this a supported use of a RawType? I remember reading in a comment that images were one of its intended uses, but is this still the case? Right now I do not think there is a way of displaying a RawType in any client. Also there may be some other issues with this, like how to know if the RawType is jpeg, png or gif for images.

@kaikreuzer
Copy link
Contributor

Yes, absolutely, there is even an ImageItem type, although I just noticed that this isn't listed in the documentation.

The Paper UI already supports it and we should imho also add support in the Classic UI and the Basic UI.

@kaikreuzer kaikreuzer changed the title Should the RawType be a use case for Images Add support for Image items in Classic/Basic UI Jul 4, 2016
@kgoderis
Copy link
Contributor

kgoderis commented Jul 9, 2016

@kaikreuzer We should/could add that to the Sonos binding as well

@kaikreuzer
Copy link
Contributor

@kgoderis Yes, this should probably be covered by #1617.

@lolodomo
Copy link
Contributor

lolodomo commented Aug 2, 2016

Is the ImageItem type alreaded supported by HABdroid ?

@kaikreuzer
Copy link
Contributor

No, feel free to enter issues for the mobile apps in their respective openHAB repos.

@lolodomo
Copy link
Contributor

lolodomo commented Oct 31, 2016

@kaikreuzer : could give an example of how item and sitemap should be defined ?
In OH 1.x wiki, we can find the syntax for Image and Video elements:
Image [item=<itemname>] [icon="<iconname>"] url="<url of image>" [label="<labelname>"] [refresh=xxxx]
Is it still valid in OH 2 ?
Unfortunately I don't understand what represents the item in this case as you have to define at the same time the URL of the image.

@watou
Copy link
Contributor

watou commented Oct 31, 2016

It would be great if image and Video sitemap elements could take the string version of the associated item's state in place of the explicitly specified url parameter, and make the url parameter optional. Whenever the associated item's state changes, the sitemap element would take it as the URL from which to fetch the image or video. There are existing bindings that could benefit from this enhancement with no code changes required in the bindings.

@lolodomo
Copy link
Contributor

Isn't it what is done in OH 1.x ? If not, what is done with the provided item ?

@watou
Copy link
Contributor

watou commented Oct 31, 2016

Isn't it what is done in OH 1.x ?

  • The url parameter is mandatory.
  • I don't find any evidence on the 1.8 branch of any use of the item= attribute of the Image widget (here for example).

@lolodomo
Copy link
Contributor

lolodomo commented Nov 1, 2016

So, we have to wait for Kai's explanation as he said that it works in Paper UI.

@kaikreuzer kaikreuzer changed the title Add support for Image items in Classic/Basic UI Add support for Image items in Sitemaps and Classic+Basic UI Nov 1, 2016
@kaikreuzer
Copy link
Contributor

Well, as there weren't any Image items in OH1, the (current) sitemaps do not support them. So adding support in Classic/Basic UI actually also means to add support in the sitemaps first (I adapted the issue title accordingly).

I think the sitemap change is pretty straight forward: Instead of having the url mandatory, we require either a url OR an item to be specified.

@watou
Copy link
Contributor

watou commented Nov 1, 2016

@kaikreuzer - In OH2, does the use case work where item=MyStringItem containing a URL from which the client retrieves the image? I would find this much easier to work with than having to set the state of an ImageItem to the raw image data.

@kaikreuzer
Copy link
Contributor

@watou Currently no, because the UIs simply do not support it.
I also think that IF we consider to support this, it will often only be an ugly workaround as a last resort. It will only work, if the url points to some publicly available server on the Internet, since the UI has to resolve it. And if you are using an app on your smartphone while you are on the road, it won't be able to get images e.g. from your local squeezebox or whatever. That's why piping it through the runtime is imho the best option. But yes, for cases where it is an album cover that can be taken from the Internet, we can also support String item types in the Image widget.

@watou
Copy link
Contributor

watou commented Nov 1, 2016

Thanks for resolving that question, @kaikreuzer. Could the StringItems' states, treated as URLs, be piped through the runtime just as the url= attribute is currently? There are currently bindings that report image URLs that change over time, and being able to specify Image item=MyImageURLStringItem in the sitemap, proxying the image through the runtime and refreshing the widget on state changes (and the optional refresh cycle) would address that nicely.

@lolodomo
Copy link
Contributor

lolodomo commented Nov 1, 2016

String item that eould be set to an URL + image element in sitemap. This will perfect for album cover.

I have mo idea how is remdered image by UIs in OH 1.x (using url parameter). Do we meed an optional size to resize the image to a particlar size ?

@kaikreuzer
Copy link
Contributor

Could the StringItems' states, treated as URLs, be piped through the runtime just as the url= attribute is currently?

I think this is difficult, since "piping through the runtime" would either mean to sent the content as a state event (that's what is done for ImageItems) or to provide a specific servlet, where the runtime would serve the content. Both is not really easy to do and probably rather results in something ugly, especially if this is only meant to be a temporary solution.

So I think there should only be the three possibilities:

  1. No item + Image widget with url param: The UIs resolve and display the image from the url. No updates required
  2. StringItem + Image widget with item param: The UIs dynamically receive urls that they themselves resolve
  3. ImageItem + Image widget with item param: The UIs dynamcally receives the image content through SSE as an event from the runtime.

Do we meed an optional size to resize the image to a particular size ?

No, I didn't introduce this in the sitemap and left it to the UIs and their layout/rendering decisions to scale images to an appropriate size.

@watou
Copy link
Contributor

watou commented Nov 3, 2016

especially if this is only meant to be a temporary solution

I don't recall any suggestion of using StringItems holding URLs as being a temporary solution; sorry if I missed that somewhere. As I said, there are a few bindings already that update items with image URLs, but there is no UI that can make use of them without item=StringItem support. Option 2 above would not be useful because, as you said, the client may not be able to reach the URL. The Classic UI already does all proxying server-side for the url= parameter, so I don't see why "piping through the runtime" (your catchy phrase 😄) would be any more difficult.

If someone submitted a PR that implemented your option 2 but with proxying as is done for the url= attribute, would you reject it?

@kaikreuzer
Copy link
Contributor

I don't recall any suggestion of using StringItems holding URLs as being a temporary solution

Sorry, I interpreted "There are currently bindings that report image URLs" as "soon there won't be such bindings anymore" ;-)

The Classic UI already does all proxying server-side for the url= parameter

Thanks for reminding me, this indeed completely escaped me. So this actually IS already the servlet option that I mentioned above.

If someone submitted a PR that implemented your option 2 but with proxying as is done for the url= attribute, would you reject it?

If this someone is you, I would never ever dare to reject it ;-) Go ahead!

@watou
Copy link
Contributor

watou commented Nov 3, 2016

Excellent! I'll give it a go.

@watou
Copy link
Contributor

watou commented Nov 5, 2016

@lolodomo
Copy link
Contributor

@watou @kaikreuzer : can we make a new step and adjust the sitemap syntax ? The Image element is already defined and as I understood we just have to make the url= parameter optional. @watou: is it already done or is it still something to be done ? Are you able to propose this change ?

@lolodomo
Copy link
Contributor

I mention here an interesting discussion in my PR for the Sonos cover art about the usage of RawType for image items and the lack of format indication that could help UI to render the picture.
The question is: is RawType without any content type indication sufficient for UI ?

@lolodomo
Copy link
Contributor

Just an idea: maybe we could add an optional "format" parameter to the Image sitemap element to help UI ?

@lolodomo
Copy link
Contributor

And another question: why is there a "web" and a "web-src" directories ? Shall I update only files in "web-src" ?

@vlad-ivanov-name
Copy link
Contributor

@lolodomo

  1. please refer to contributing.md in the BasicUI directory (mostly to the part where you need to run grunt to assemble client-side resources)
  2. client-side javascript can be debugged in browser's developer tools (F12)

@lolodomo
Copy link
Contributor

No chance to just edit the source file in Eclipse IDE and then run from inside Eclipse ? I need to run "grunt" outside Eclipse after each file update ? Isn(t it something integrated in Eclipse that runs automatically when I run OH from inside Eclipse ?
If I run a "mvn clean install" in the root directory of Basic UI, is it ok ?

@vlad-ivanov-name
Copy link
Contributor

No, there is not. You can create a run configuration (there is a "Grunt" configuration type), which will basically call the same command from Eclipse. It still will need to be triggered manually.

@lolodomo
Copy link
Contributor

Just to understand, when I run "mvn clean install", it does not do all the job ?

@vlad-ivanov-name
Copy link
Contributor

It does not. Client-side scripts and styles are assembled with grunt. It's not included in the maven build process (as far as I remember due to legal reasons — it requires getting clearance on lots of javascript libraries).

@lolodomo
Copy link
Contributor

Tu understand, where are in the tree the source files to edit and the files that are produced by grunt and then used at runtime ?

Is it easy to install grunt on a Windows machine ? It looks more something for Linux...

@vlad-ivanov-name
Copy link
Contributor

Sources are in /web-src, resulting files are in /web. You need to install node.js and npm first, Google can help with the rest.

@lolodomo
Copy link
Contributor

For testing purposes, if I copy smarthome.js from web-src to web, can it work ?

@vlad-ivanov-name
Copy link
Contributor

It will work, but the changes must be validated by eslint (run from grunt) before being commited.

@lolodomo
Copy link
Contributor

Thank you for your explanations, I am now able to see the impact of my changes (I have copied smarthome.js from web-src to web and edited it in web).

If only few lines need to be updated, maybe I can provide them to you so that you can propose a proper PR ?

@lolodomo
Copy link
Contributor

Now that I can edit and debug the smarthome.js file (cool, thank you @resetnow ), I can see that setValuePrivate is called as expected but with itemState argument having the value "raw type (unknown): 4387 bytes". It is the format output of RawType.toString(). As a remember, I only put the data URI in RawType.toFullString(). So I can't update the image as I would like in setValuePrivate as I don't have the new data URI.
So I think I have first to change the SSE event so that data.item.state contains the data URI when the item is an image item.
@resetnow : is it the good way to do ?

@vlad-ivanov-name
Copy link
Contributor

is it the good way to do ?

It's not really up to me right now. I mean it's technically correct and probably will work, you just need to confirm it with maintainers.

@lolodomo
Copy link
Contributor

@watou : a blank PNG would be OK. Will look for that in the existing code.

Regarding support of my changes in Safari, I will not be able to check that myself, sorry. But I will check at least with Firefox and Chrome.

@lolodomo
Copy link
Contributor

I succeeded making the update of image working in Basic UI (only 2 minor changes are required) but there is still one case that does not work, when state becomes UNDEF and you have suddenly to display the default URL while it was ignored when initially building the page.

@watou : I was asking myself if rather than bypassing the proxy, we could use the proxy even for images that are stored in image items ? The advantage is that there would be no change to do in Basic UI and Classic UI. But I have no idea if it makes sense and if it makesis doable for the proxy ?

@lolodomo
Copy link
Contributor

Sorry to share all my thoughts loudly lol
I see now that the proxy idea is a wrong idea.
But I think I have the solution: we need to store the default proxied URL in the HTML in case we need it at a certain time. So we just need to add a new entry in snippets/image.html to store it.

@lolodomo
Copy link
Contributor

Ok, the refresh of image in Basic UI is now working.

@lolodomo
Copy link
Contributor

In case no default URL is set, my idea would be that the proxy would return the none.png icon. What would be the URI to build that leads to this icon ? A relative path ?

@watou
Copy link
Contributor

watou commented Feb 27, 2017

the proxy would return the none.png icon

The ProxyServlet is not able to address an image that isn't referenced via url= or item= in an Image sitemap widget. The images/none.png image that is part of the Basic and Classic clients could be accessed without need of the ProxyServlet.

@vlad-ivanov-name
Copy link
Contributor

You can encode 1x1 transparent PNG in base64.

@watou
Copy link
Contributor

watou commented Feb 27, 2017

You can encode 1x1 transparent PNG in base64.

I had trouble making that work in Safari in #1106. See here.

...which might be an issue for this PR, if setting img.src to a data URI still doesn't work in Safari as I saw at the time of #1106. Hopefully it was a Safari bug that's since been fixed, otherwise this PR may not work on Safari.

@lolodomo
Copy link
Contributor

@watou : the current logic is mostly implemented by the ProxyServlet and hidden to the UIs. By the way, a minor update of the ProxyServlet will be required because if not it will lead to an exception (not valid URI).
We have now several combinations (StringItem, ImageItem, hardcoded url) and allowing to not set a default URL will even add more combinations. We could use images/none.png image that is part of the Basic and Classic clients but it will require to implement the full logic in UI.
This logic could be:
if itemState starts with "data:", use itemState (ImageItem)
else if itemState not NULL state, use the proxied URL (URL in StringItem)
else if url is not set in sitemap element, use images/none.png
else use the proxyied URL

@watou
Copy link
Contributor

watou commented Feb 28, 2017

Why would the ProxyServlet need to be involved in using ImageItems' state to show images? There are no http[s] URLs involved in those items. If the client has hit the proxy servlet in order show an image from an ImageItem, then it's already on the wrong code path. What am I missing here?

@lolodomo
Copy link
Contributor

No, I don't want to inject ImageItem in ProxyServlet, it is hanlded outside the ProxyServlet and it is ok. My concern is no more the ImageItem but the default URL and more precisely how to handle the case it will not be set in the sitemap image element (and so not provided by the ProxyServlet).
But that is ok, I will try to handle the full logic inside each UI. I have now already a good idea how to do it in Basic UI.

@lolodomo
Copy link
Contributor

lolodomo commented Feb 28, 2017

To be clear, I would like any UI to support all these cases for the sitemap image element:

  • url without item
  • ImageIten with url as default
  • ImageIten without url as default
  • StringIten with url as default
  • StringIten without url as default

And of course, there is still the refresh parameter to not forget.

@lolodomo
Copy link
Contributor

lolodomo commented Mar 1, 2017

I have now something working in my 5 use cases in Basic UI and Classic UI.

@resetnow : I installed node.js, grunt-cli and grunt. What is the command to run at the root of Basic UI folder ? I tried just "grunt" but it seems to do nothing, web/smarthome.js is not updated.

@vlad-ivanov-name
Copy link
Contributor

npm install and grunt

@lolodomo
Copy link
Contributor

lolodomo commented Mar 1, 2017

Works now, thanks.
I have warnings of this kind: Mixed spaces and tabs no-mixed-spaces-and-tabs
JavaScript file seems to not use the standard of the project (Eclipse IDE), that is tab for 4 spaces.
Not clear how I must update Eclipse IDE setting to update the JavaScript files.

@lolodomo
Copy link
Contributor

PR now merged.. issue to be closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants