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

Small fixes and updates #117

Merged
merged 11 commits into from
Dec 9, 2018
Merged

Small fixes and updates #117

merged 11 commits into from
Dec 9, 2018

Conversation

Confectrician
Copy link
Collaborator

@Confectrician Confectrician commented Dec 5, 2018

This will contain everything for a new small release in the nearer future.
Mainly this is thought to adress some of the currently urgent problems.

Closes #115, #108, #109

  • Fix description for restCompletions
  • Add (at least minimal) WebView replacement for the outdated previewHtml function
  • Remove searchDocs option for now
  • Show in paperUI should always open an external browser for now

Signed-off-by: Jerome Luckenbach github@luckenba.ch

vsix of this PR:
openhab-0.4.1-beta.vsix.zip

Signed-off-by: Jerome Luckenbach <github@luckenba.ch>
Signed-off-by: Jerome Luckenbach <github@luckenba.ch>
Signed-off-by: Jerome Luckenbach <github@luckenba.ch>
Signed-off-by: Jerome Luckenbach <github@luckenba.ch>
Signed-off-by: Jerome Luckenbach <github@luckenba.ch>
@Confectrician
Copy link
Collaborator Author

The WebView works basically already:
grafik

I have used an approach with an own exported module now, which lets us add new view types fast and easy. (hopefully)
Also the class should be easily extendable with a refresh method and could handle different content types in the future.

Maybe we can simplify the extension.ts code this way on long term too.

The class approach is also already prepared for injecting new content through a javascript within the webview html document.

Hopefully i have enough time to finish this part tommorow to get basicUI/iframe running fine.

@MHerbst, @SamuelBrucksch

Maybe both of you could have a look at the code.
I would generate new .vsix anyway and promote it in the community for testers, when the main work is finished here.

Copy link
Contributor

@SamuelBrucksch SamuelBrucksch left a comment

Choose a reason for hiding this comment

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

Hi,

i did not use the webview yet, so i'm not sure about what it does so i cannot review the logic itself, however i tried to give some feedback on the code in general.

BR
Sam

package.json Outdated Show resolved Hide resolved
src/WebView/PreviewPanel.ts Outdated Show resolved Hide resolved
src/WebView/PreviewPanel.ts Outdated Show resolved Hide resolved
src/WebView/PreviewPanel.ts Show resolved Hide resolved
@Confectrician
Copy link
Collaborator Author

i did not use the webview yet, so i'm not sure about what it does so i cannot review the logic itself, however i tried to give some feedback on the code in general.

No problem @SamuelBrucksch i also didn't use it before, so i am sure we will have to adapt things when the extension has been released and some more users get to test it.
Thanks for your comments so far. 👍

…eview.

Signed-off-by: Jerome Luckenbach <github@luckenba.ch>
@Confectrician
Copy link
Collaborator Author

Don't worry about the many insertions and deletions.
I have changed the minimal supported vscode version to get all currently available apis for packaging process.
This did some magic with the package-lock.json file.

@MHerbst
Copy link

MHerbst commented Dec 7, 2018

I have scrolled through the code and it looks good to me. Will fetch it a bit later and try it in my test environment.
With the new implementation, I think there is quite a lot now unnecessary code now in ContentProvider/openHAB.ts that we can remove.

@Confectrician
Copy link
Collaborator Author

With the new implementation, I think there is quite a lot now unnecessary code now in ContentProvider/openHAB.ts that we can remove.

Recognized that too, but that's something we can take care of later.
Maybe we can remove the whole ContentProvider area and move still needed parts into the existing code, when it makes sense.

@Confectrician
Copy link
Collaborator Author

I am not satisfied completely with the class approach that i more or less copied over from the Microsoft extension api examples.
But it works for now and we can adapt and refactor it later too.

@MHerbst
Copy link

MHerbst commented Dec 7, 2018

Maybe we can remove the whole ContentProvider area and move still needed parts into the existing code, when it makes sense.

That was my impression, too. It seems that only the Queryinterface is still in use.

Signed-off-by: Jerome Luckenbach <github@luckenba.ch>
@Confectrician
Copy link
Collaborator Author

OK i am finished with the WIP part.

I think the query is only used in one place in the webview part from me, so refactoring and then removing the content provider should be no big deal.

@Confectrician Confectrician changed the title [WIP] Small fixes and updates Small fixes and updates Dec 7, 2018
@Confectrician Confectrician reopened this Dec 7, 2018
Signed-off-by: Jerome Luckenbach <github@luckenba.ch>
@Confectrician
Copy link
Collaborator Author

ContentProvider folder was removed too now.
Debugging looks good to me for now.

We could share a vsix in the community for a bigger test audience.
I can package one.

Wdyt?

@MHerbst
Copy link

MHerbst commented Dec 7, 2018

Sounds good

@Confectrician
Copy link
Collaborator Author

Ok I'll prepare one.

Signed-off-by: Jerome Luckenbach <github@luckenba.ch>
@MHerbst
Copy link

MHerbst commented Dec 8, 2018

Tested it on Windows and Ubuntu. Looking good on both systems.

Signed-off-by: Jerome Luckenbach <github@luckenba.ch>
Signed-off-by: Jerome Luckenbach <github@luckenba.ch>
@Confectrician
Copy link
Collaborator Author

Confectrician commented Dec 9, 2018

:shipit:

Approved with PullApprove

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

Successfully merging this pull request may close these issues.

3 participants