some changes to XBMC binding #4622
Conversation
Committing change for xbmc binding
| public void activate() { | ||
| logger.debug(getName() + " activate()"); | ||
| setProperlyConfigured(true); | ||
| //setProperlyConfigured(true); // <-Removed as it has been moved to the "updated" call |
There was a problem hiding this comment.
Just remove the commented code - the history will be retained in github.
I added several hundred commands to xbmc binding. 1) can send all 192 input commands supported by Input.ExecuteActions JSON call directly from the items file using Input.ExecuteAction.<<command>> List of commands are in list 1 below. Source: http://kodi.wiki/view/JSON-RPC_API/v6#Input.Action Usage: {xbmc=">[#xbmcinstance|Input.ExecuteAction.<<commandname>>"} Examples: Switch sXBMC_SHOWOSD "XBMCosd" {xbmc=">[#kodisalotto|Input.ExecuteAction.osd]"} Switch sXBMC_UP "XBMCup" {xbmc=">[#kodisalotto|Input.ExecuteAction.up]"} Switch sXBMC_ASPECTRATIO "XBMCar" {xbmc=">[#kodisalotto|Input.ExecuteAction.aspectratio]"} Switch sXBMC_TOGGLESCREEN "XBMCtf" {xbmc=">[#kodisalotto|Input.ExecuteAction.togglefullscreen]"} 2) can read all 23 Player.Property.Name values: List of commands in list 2 below. Source: http://kodi.wiki/view/JSON-RPC_API/v6#Player.Property.Name Usage: {xbmc="<[#xbmcinstance|Property.<<propertyName>>"} Examples: String sXBMC2 "XBMC2 [%s]" {xbmc="<[#kodisalotto|Property.Type]"} String sXBMC3 "XBMC3 [%s %%]" {xbmc="<[#kodisalotto|Property.Percentage]"} String sXBMC4 "XBMC4 [%s]" {xbmc="<[#kodisalotto|Property.Time]"} String sXBMC5 "XBMC5 [%s]" {xbmc="<[#kodisalotto|Property.TotalTime]"} String sXBMC6 "XBMC6 [%s]" {xbmc="<[#kodisalotto|Property.CurrentSubtitle]"} 3) can Retrieve info labels about Kodi and the system using XBMC.GetInfoLabels List of possible commands in: http://kodi.wiki/index.php?title=InfoLabels (there are hundreds) Usage: {xbmc="<[#xbmcinstance|Label.<<infolabelname>>"} Examples: String sXBMC7 "XBMC7 [%s]" {xbmc="<[#kodisalotto|Label.VideoPlayer.Title]"} String sXBMC8 "XBMC8 [%s]" {xbmc="<[#kodisalotto|Label.Player.Time]"} String sXBMC9 "XBMC9 [%s]" {xbmc="<[#kodisalotto|Label.VideoPlayer.VideoResolution]"} String sXBMC10 "XBMC10 [%s]" {xbmc="<[#kodisalotto|Label.VideoPlayer.VideoCodec]"} String sXBMC11 "XBMC11 [%s]" {xbmc="<[#kodisalotto|Label.System.Uptime]"} list1: left right up down pageup pagedown select highlight parentdir parentfolder back previousmenu info pause stop skipnext skipprevious fullscreen aspectratio stepforward stepback bigstepforward bigstepback osd showsubtitles nextsubtitle codecinfo nextpicture previouspicture zoomout zoomin playlist queue zoomnormal zoomlevel1 zoomlevel2 zoomlevel3 zoomlevel4 zoomlevel5 zoomlevel6 zoomlevel7 zoomlevel8 zoomlevel9 nextcalibration resetcalibration analogmove rotate rotateccw close subtitledelayminus subtitledelay subtitledelayplus audiodelayminus audiodelay audiodelayplus subtitleshiftup subtitleshiftdown subtitlealign audionextlanguage verticalshiftup verticalshiftdown nextresolution audiotoggledigital number0 number1 number2 number3 number4 number5 number6 number7 number8 number9 osdleft osdright osdup osddown osdselect osdvalueplus osdvalueminus smallstepback fastforward rewind play playpause delete copy move mplayerosd hidesubmenu screenshot rename togglewatched scanitem reloadkeymaps volumeup volumedown mute backspace scrollup scrolldown analogfastforward analogrewind moveitemup moveitemdown contextmenu shift symbols cursorleft cursorright showtime analogseekforward analogseekback showpreset presetlist nextpreset previouspreset lockpreset randompreset increasevisrating decreasevisrating showvideomenu enter increaserating decreaserating togglefullscreen nextscene previousscene nextletter prevletter jumpsms2 jumpsms3 jumpsms4 jumpsms5 jumpsms6 jumpsms7 jumpsms8 jumpsms9 filter filterclear filtersms2 filtersms3 filtersms4 filtersms5 filtersms6 filtersms7 filtersms8 filtersms9 firstpage lastpage guiprofile red green yellow blue increasepar decreasepar volampup volampdown channelup channeldown previouschannelgroup nextchannelgroup leftclick rightclick middleclick doubleclick wheelup wheeldown mousedrag mousemove noop list 2: canrepeat canmove canshuffle speed percentage playlistid audiostreams position repeat currentsubtitle canrotate canzoom canchangespeed type partymode subtitles canseek time totaltime shuffled currentaudiostream live subtitleenabled
|
Looks ok to me, if you have tested it and are happy it works then I am happy to approve it. The only other thing to do would be to update the WIKI page for this binding with instructions on how to use all the new actions. Thanks very much! |
watou
left a comment
There was a problem hiding this comment.
Just a few issues with comments and logging.
| * @author tlan, Ben Jones | ||
| * @since 1.5.0 | ||
| * @author tlan, Ben Jones, Plebs | ||
| * @since 1.9.0 |
There was a problem hiding this comment.
Please just add your name to the existing @author tag for files you've changed, and leave the @since tag as is. So in the case of this file, no changes are warranted.
| * @author tlan, Ben Jones | ||
| * @since 1.5.0 | ||
| * @author tlan, Ben Jones, Plebs | ||
| * @since 1.9.0 |
There was a problem hiding this comment.
Please just add your name to the existing @author tag for files you've changed, and leave the @since tag as is.
| * @author tlan, Ben Jones | ||
| * @since 1.5.0 | ||
| * @author tlan, Ben Jones, Plebs | ||
| * @since 1.9.0 |
There was a problem hiding this comment.
Please just add your name to the existing @author tag for files you've changed, and leave the @since tag as is. So in the case of this file, no changes are warranted.
| * @author tlan, Ben Jones | ||
| * @since 1.5.0 | ||
| * @author tlan, Ben Jones, Plebs | ||
| * @since 1.9.0 |
There was a problem hiding this comment.
Please just add your name to the existing @author tag for files you've changed, and leave the @since tag as is. So in the case of this file, no changes are warranted.
| * @author tlan, Ben Jones | ||
| * @since 1.5.0 | ||
| * @author tlan, Ben Jones, Plebs | ||
| * @since 1.9.0 |
There was a problem hiding this comment.
Please just add your name to the existing @author tag for files you've changed, and leave the @since tag as is.
| * @since 1.5.0 | ||
| * @author tlan, Ben Jones, Plebs | ||
| * @since 1.9.0 | ||
| */ |
There was a problem hiding this comment.
Since you added this file, the comment should just be
* @author Plebs
* @since 1.9.0| } | ||
|
|
||
| Map<String, Object> params = new HashMap<String, Object>(); | ||
| // params.put("playerid", playerId); |
There was a problem hiding this comment.
Remove commented code lines.
| @Override | ||
| protected void processResponse(Map<String, Object> response) { | ||
| Map<String, Object> result = getMap(response, "result"); | ||
| // item = getMap(result, "label"); |
There was a problem hiding this comment.
Remove commented code lines.
| * @author Ben Jones | ||
| * @since 1.5.0 | ||
| * @author Plebs | ||
| * @since 1.9.0 |
There was a problem hiding this comment.
Comment tags should just be
* @author Ben Jones, Plebs
* @since 1.5.0| @Override | ||
| protected void processResponse(Map<String, Object> response) { | ||
| Map<String, Object> result = getMap(response, "result"); | ||
| // item = getMap(result, "item"); |
There was a problem hiding this comment.
Remove commented code lines.
Also added a new command: "Refresh" to asynch refresh the labels coming from XBMC
|
Hi, I have also added a new command in the meantime... On 20 September 2016 at 12:09, John Cocula notifications@github.com wrote:
|
|
Hi all, On 20 September 2016 at 20:54, STEFANO FRANCO franco.stefano@gmail.com
|
watou
left a comment
There was a problem hiding this comment.
I've added a few more minor comments and a question on thread safety in calling execute() from different threads. If the above is addressed and some other XBMC/Kodi binding user can help verify proper function, I'd say it should be mergeable. Thanks for this PR.
| // process any response as necessary (can be null) | ||
| ListenableFuture<Response> future = client.preparePost(uri).setBody(writeJson(request)) | ||
| String resultWrite = writeJson(request); | ||
| logger.debug("Write JSON: {}", resultWrite ); // Stefano: logging the JSON request |
There was a problem hiding this comment.
Please put the code through the Eclipse IDE formatter so future changes don't reformat lines with incorrect formatting.
| .execute(new AsyncCompletionHandler<Response>() { | ||
| @Override | ||
| public Response onCompleted(Response response) throws Exception { | ||
| logger.debug("Read JSON: {}", response.getResponseBody()); // Stefano: logging the JSON response |
There was a problem hiding this comment.
Please put the code through the Eclipse IDE formatter so future changes don't reformat lines with incorrect formatting.
| * | ||
| */ | ||
| public void addItem(String itemName, String property) { | ||
| logger.debug("Mapping: itemname = {} & property = {}", itemName, property); |
There was a problem hiding this comment.
Please put the code through the Eclipse IDE formatter so future changes don't reformat lines with incorrect formatting.
| } else if (property.equals("PVR.OpenRadio")) { | ||
| connector.playerOpenPVR(command.toString(), 1); | ||
| } else if (property.equals("Refresh")) { | ||
| execute(); |
There was a problem hiding this comment.
Are you certain that it is threadsafe to do this? The execute() method is called at some refreshInterval in another thread, so it may be possible that this method could be run from each thread overlapping. You might consider using a technique like in the HTTP or Ecobee binding, where the execute() method is called based on a short granularity (five seconds, for example), but checks a future timestamp to see if it should do anything. The "Refresh" action would then just expire the future timestamp, so the next scheduled execute happens on average granulary/2 into the future. The execute() method would just set the future timestamp to the current timestamp + refreshInterval. (There are certainly other sound approaches to avoiding thread safety issues.)
| * Player.GetProperties RPC | ||
| * | ||
| * @author Ben Jones | ||
| * @author Ben jones, Plebs |
|
Hi all, Do I have to reformat the single line in the 3 files and resend to you? Or Regarding the refresh() method, I didn't want to change the original What are the next steps? thanks On 28 September 2016 at 15:39, John Cocula notifications@github.com wrote:
|
My comment was just to verify that this PR wasn't introducing a new thread safety issue. If other parts of the binding invoke So I will merge this now, and make a new PR myself to only correct the minor formatting issues. |
|
@giig1967g Please update the wiki to document your new enhancements. Thanks! |
|
Hi watou, So, I think that the options are three:
What is the best practice? thanks |
|
Hi Stefano, it's OK to update the wiki now, but please indicate which features will be available in version 1.9 and later of the binding, which will be the next official release that contains your enhancements. So the wiki will still be helpful to 1.8 users, but they won't be confused about which features they don't have if they are marked something like "available in 1.9." |
|
ok. thanks for the tip. stefano On 9 October 2016 at 12:24, John Cocula notifications@github.com wrote:
|
* master: (30 commits) [satel] Fixed refresh after control command (openhab#4674) [IPX800] Add binding to OH2 distro (openhab#4684) Database update (openhab#4692) ZWave database update (openhab#4679) [xbmc] Minor formatting (openhab#4683) some changes to XBMC binding (openhab#4622) [telegram] sendTelegramPhoto caption should be UTF-8 (openhab#4650) New api url for forecastio provider (openhab#4677) Review 1: Patch for: Error on startup with SQLite openhab#4652 (openhab#4670) [Plugwise] Fix Circles not always calibrated (openhab#4669) Database update (openhab#4664) [garadget] Remove misleading text on exception logs (openhab#4663) Fixes openhab#4635 (openhab#4649) [Plugwise] NullPointerException may occur when concurrent threads get PlugwiseDevice from Stick (openhab#4648) insteonplm binding: added mini remote 2342-222 (openhab#4641) [modbus] Connection timeout parameter for TCP slaves. (openhab#4627) [Plugwise] Add openHAB 2 feature addon (openhab#4644) fixes build problems [Plugwise] Add support for Scan, Sense, Stealth and Switch openhab#4565 (openhab#4586) [satel] Added rule action to set user code (openhab#4634) ...
|
Hi, thanks |
|
Thanks @giig1967g! Comments on your wiki changes:
I am concerned that this change to the binding will break existing users' configurations. Every PR should not break any existing configurations unless critically necessary. If this is a breaking change, it and the wiki should be reverted. What are your thoughts?
I suggest that you only label features that are new in 1.9 as such, and remove any references to 1.8 that you added if nothing about the feature has changed from 1.8 to 1.9.
When you say
I think the examples would be more helpful if they showed a complete Item definition, as one would have in a .items file. That way, the user knows better what item type is being used, a label string could in some cases add more context, etc. |
|
Hi John, You can check in github.com. You are right about the wiki regarding the expected returned text for the Thanks for your tips. Il 10/Ott/2016 18:59, "John Cocula" notifications@github.com ha scritto:
|
That's the best possible answer. Thanks for fixing the documentation.
Perfect. Thank you! |
I hope this time is fine