annex4-inc / vscode-control4-ext

VSCode extension for building Control4 drivers.
GNU General Public License v3.0
25 stars 6 forks source link

A few small issues #9

Open pmcgriff opened 1 year ago

pmcgriff commented 1 year ago

First of all, this is great work. I am enjoying learning how to utilize your extension. There are a few things I want to bring to your attention.

1.) The versioning creates a SemVer XML tag with the correctly formatted version, but Control4 does not use this tag. It uses the version tag and that is not formatted correctly in the build. I think if you just put the version formatting created for the semver tag that it would work nicely.

2.) The only driver control methods allowed by the extension in the packages.json are "ip" and "serial". Driverworks also contains "ir", "zigbee", "relay", and "other". Can these be added? When I tried adding them to the source of the extension, the project would not build.

3.) There seems to be a bug and an omission on the DEVICE_SELECTOR property. If you create a new property of this type from the extension's GUI, it only has a default value that can be set. The XML tags that are needed are , , and . Each item will be the name of a C4i or C4Z file. Here is example:

  <property>
    <name>AssociatedDevice</name>
    <type>DEVICE_SELECTOR</type>
    <items>
      <item>control4_network_mediastorage.c4i</item>
    </items>
    <multiselect>false</multiselect>
  </property> 

I noticed that if a DEVICE_SELECTOR property is imported into a project when importing an existing c4z file, the the XML created does have an tag with the correct value, but it still does not build correctly.

Thanks again for your hard work. Hopefully you can make these fixes.

Peter

annex4 commented 1 year ago

The version tag is set here: https://github.com/annex4-inc/vscode-control4-ext/blob/944cc62fe26b8fdf4418bc28cddc510608f746f2/client/src/control4/driver.ts#L156

  1. It's padding the minor & patch portions of the semantic version in order to remain consistent in increasing integer values for new versions. The reason I designed it this way was in the event your patch version for example hit 2 characters a version '1.0.11' becomes version '1011' if I don't pad it. Now if you bumped the major version and minor/patch are back to 0 then you're set to '200' in the version tag if I don't pad it. Alternative would be to use yyyy-mm-dd-hh-mm (without the '-') that way the value is always increasing.

  2. I made the change for the control methods already, just hasn't been pushed to the released version: https://github.com/annex4-inc/vscode-control4-ext/blob/944cc62fe26b8fdf4418bc28cddc510608f746f2/client/src/resources/schemas/package.json#L147

  3. Definitely seems like a bug, should be easily resolved. When I have a minute I'll get that fixed & push a new release.

pmcgriff commented 1 year ago

Thanks for that and it looks like you went down a path with the experience icons that I was looking at. I am not a great programmer but do have a background in CS. Not well versed on Git, but I did take this repo private on Bitbucket to familiarize myself and work on it some. I think I have made a number of UI fixes and minor bug fixes as well as have made progress on implementing a control panel for the Navigator Display Options. As I said however I am not the best coder so I have refactored/ in the process of refactoring the code again as I become more familiar with how it all works together. I will be happy to share what I have done as I imagine it will not take much time for you to get up to speed and likely fix/improve anything you see wrong if you think it is worth it. I did change out the Annex4 VSCode Icon for a Control4 one because I liked keeping to the theme. but I don't know if that could legally be used publicly. Let me know if you are interested and the best way to give you access. I have been using Git but as I mentioned I don't really know how to use it properly.

pmcgriff commented 1 year ago

Also, the way I fixed the form "Enter" issue instead of changing back to a Div like it appears you did was with this:

<form class="page" on:submit|preventDefault={submit}>

The issue with this was that "Enter" was then selecting the first remove button. This was fixed by adding:

type="button"

To each of the remove buttons and any other button but submit because the default behavior is to assume the next button without this explicit type is submit.

Then you can actually update with the Enter and still tab through the fields properly.

I have also added information mousovers for the Webview form fields.

There was also a but where the dispose event was not firing correctly for the Webview panels and so if you closed a tab. You could not reopen. It seems like it was caused by the ComponentPanel parent class having and OnDidDispose defined and was preventing the inherited/child classes event from firing.

annex4 commented 1 year ago

Best way to contribute would be to fork the repository on GitHub (looks like you already have) make changes and then submit a pull request & then I can merge. I've started writing some more tests, particularly around the capability & stages section of the extension. Once they're complete I'll set up a GitHub action to build & deploy the extension automatically.

pmcgriff commented 1 year ago

I copied the repo to Bitbucket and have been working on it there. Is there an easy way to remerge it with the original fork from the Github repo? Love to contribute but I strongly advise you check my work. There is a good chance that you can clean up code and implement something a bit more elegant and efficient while leveraging what I have done. LMK

annex4 commented 1 year ago

I'm not sure how familiar you are with the command line for git but you can setup multiple remote repositories (remotes), one being for GitHub, one for Bitbucket this way you can sync the repository up to GitHub or Bitbucket. Bitbucket is likely set as 'origin', you can confirm with git remote -v

Add the remote for github (this is your forked project): git remote add github https://github.com/pmcgriff/vscode-control4-ext.git

Push to the remote (your fork): git push github master

You may want to create a new branch instead of 'master', this way we can work on any changes to merge in your new features if you think we should review it.

pmcgriff commented 1 year ago

Thanks for the reply and sorry for my late reply I have been tied up lately. Yes, Bitbucket is confirmed as origin. I will follow your instructions above to also push to my forked version. To confirm, after I do that I should create a new branch in which I will issue the pull requests to that they do not go to the original master branch and we can review if necessary?

pmcgriff commented 1 year ago

Hey, so I spent some time getting all of the above handled and then adding your repo as an upstream option so that I can do merges to keep my repo up to date with yours. I did a merge today but had to make some choices where there we changes to same file. LMK if you want me to send a pull request or how you want to handle. Right now I am on master branch but can create another if that helps. Some of my code is likely a bit cumbersome but it does work. I'm sure you could help make a number of improvements to it.

annex4 commented 1 year ago

You can issue a pull request to master, that's fine. We can resolve any issues that pop up.

annex4 commented 1 year ago

I think NavigatorDisplayOptions technically supports multiple proxies, not just one. I think that's really the only issue I see so far.

pmcgriff commented 1 year ago

Thats good. The examples I have seen all seem to show one and it is an attribute in the xml tag, but could there be more than one navigator_display_options tag with their own proxies and icons? This came from the Driverworks SDK. Again I am new and just learning about C4 driver programming, so I'll defer to you.

Capabilities Non-AV Devices ››UI Button ›› Capbilities navigator_display_options / display_icons / state This capability adds the ability to display device state icons for the button. As of this release, icon resolution for non-Media Server Proxy based drivers is: 300x300px 90x90px 70x70px. For example:

controller://driver/shortcutTest/icons/msp_ico_settings/msp_ico_s ettings_300.png controller://driver/shortcutTest/icons/msp_ico_settings_a/ msp_ico_settings_a_300.png controller://driver/shortcutTest/icons/msp_ico_settin gs/msp_ico_settings_a_300.png

DriverWorks SDK OS 3.1.3 Proxy and Protocol Guide 444 / 451 Note, this example does not include all of the icon sizes for brevity.

pmcgriff commented 1 year ago

Also, I had to make a fix to the state and default icon imports because it was creating one even when it did not exist. Was just a simple bug. Do you happen to have any information on the UI functionality? I looked at the xml for the roku driver but have no idea whether or not that is standard formatting or whether there can be various formats. I at least fixed it up to where the import of the driver succeeds but stopped after that because I am unsure what really needs to happen next.

annex4 commented 1 year ago

There can be multiple 'navigator_display_option' elements AFAIK. You bind them to their associated proxy via the 'proxybindingid' attribute. I'll confirm here in a minute to make sure.

pmcgriff commented 1 year ago

Didn't really think about that possibility because I was focused on experience icons. This will necessitate rethinking the implementation as the interface only handles one NavOption as implemented. What resources are you referencing? Is there something better than the SDK?

annex4 commented 1 year ago

The navigator_display_option part I had used before. I did just confirm you can support multiple proxies and target them with the capability separately using proxybindingid attribute.

As for the UI section, there's an XSD file with the schema defined.

annex4 commented 1 year ago

Because the navigator_display_option is bound to the proxy specified by ID, i wonder if it would make more sense to include it (optionally) in the "proxies.c4c" file. Something like this:

  {
    "id": 5001,
    "image_source": "c4z",
    "large_image": "icons/device_lg.png",
    "small_image": "icons/device_sm.png",
    "name": "LG TV",
    "primary": true,
    "proxy": "tv",
    "navigator_display_option": {
      "display_icons" {
        "defaults": []
        "states": {
          "on": []
          "off": []
        }
      }
    }
  }

Where the empty array values contain the icon information (path, height, width, sizes)

pmcgriff commented 1 year ago

Just now getting some time to. Look. I need to digest your thoughts.under "states" you show an empty array for On and off. Are those just examples because the states can be about anything particularly when doing experience when using icons.

Also, are you saying there can be multiple navigator_display_options tags each with one proxybindingid attribute or that one tag can have multiple proxybindingid tags?

pmcgriff commented 1 year ago

And sorry for typos. On phone and Apple is not being very helpful. Lol

annex4 commented 1 year ago

In the previous example the states are just empty arrays, I didn't want to clutter it too much. As for the navigator display option capability, you can do this:

  <proxies qty="2">
    <proxy proxybindingid="5001" name="Media 1" small_image="sm.png" large_image="lg.png" image_source="c4z">media_player</proxy>
    <proxy proxybindingid="5002" name="Media 2" small_image="sm.png" large_image="lg.png" image_source="c4z">media_player</proxy>
  </proxies>
    <navigator_display_option proxybindingid="5001">
      <display_icons>
        <Icon width="70" height="70">controller://driver/serial_shim/images/device_sm.png</Icon>
        <Icon width="90" height="90">controller://driver/serial_shim/images/device_sm.png</Icon>
        <Icon width="300" height="300">controller://driver/serial_shim/images/device_sm.png</Icon>
        <Icon width="512" height="512">controller://driver/serial_shim/images/device_lg.png</Icon>
        <Icon width="1024" height="1024">controller://driver/serial_shim/images/device_lg.png</Icon>
      </display_icons>
      <translation_url>controller://driver/serial_shim/translations</translation_url>
    </navigator_display_option>
    <navigator_display_option proxybindingid="5002">
      <display_icons>
        <Icon width="70" height="70">controller://driver/serial_shim/images/pacman512.png</Icon>
        <Icon width="90" height="90">controller://driver/serial_shim/images/pacman512.png</Icon>
        <Icon width="300" height="300">controller://driver/serial_shim/images/pacman512.png</Icon>
        <Icon width="512" height="512">controller://driver/serial_shim/images/pacman512.png</Icon>
        <Icon width="1024" height="1024">controller://driver/serial_shim/images/pacman512.png</Icon>
      </display_icons>
      <translation_url>controller://driver/serial_shim/translations</translation_url>
    </navigator_display_option>

Which will cause the navigator to display the appropriate icon for each proxy.

pmcgriff commented 1 year ago

Ok that is what i was picturing. thanks for clarifying. I wonder if we don't do it similar to the connections and we have state icons and default icons as sub items for each nav display? I'll think on it. Hoping to have some time in the next few days to work on it a bit. Also, where are the XSD schema files you referred to located? That would definitely help. If you are referring to the json schema files then where did those originate?