redhat-developer / vscode-server-connector

📦 Connects Visual Studio Code to your server adapters and run, deploy apps !!
Eclipse Public License 2.0
57 stars 26 forks source link

Added editor to allow user change server properties (#352) #355

Closed lstocchi closed 5 years ago

lstocchi commented 5 years ago

it fixes #352

Solution still incomplete, need to fix some minor things but I'm creating this PR so you can start checking it due to the limited time we have left. Hopefully you will have some idea to improve it.

Currently i'm using temp files because i wasn't able to store the json in-memory. Virtual documents seem that are not a valid solution for our purpose. Already opened a issue so we can investigate more later #353

codecov[bot] commented 5 years ago

Codecov Report

Merging #355 into master will decrease coverage by 3.28%. The diff coverage is 22.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #355      +/-   ##
==========================================
- Coverage   56.47%   53.18%   -3.29%     
==========================================
  Files          10       11       +1     
  Lines         765      848      +83     
  Branches      167      186      +19     
==========================================
+ Hits          432      451      +19     
- Misses        333      397      +64
Impacted Files Coverage Δ
src/utils.ts 20% <20%> (ø)
src/serverEditorAdapter.ts 29.31% <29.31%> (ø)
src/extensionApi.ts 42.3% <4.76%> (-1.87%) :arrow_down:
src/extension.ts 52.8% <50%> (+0.36%) :arrow_up:
src/serverExplorer.ts 64.25% <7.14%> (-4.83%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 54c3c7b...dcc1acb. Read the comment docs.

adietish commented 5 years ago

@lstocchi code-wise things look good, thx for the updates.

When testing it I stumbled upon the following things:

Usecase 1

  1. EXEC: pick "Edit Server" in context menu of your server

Result: Json editor pops up. The editor title wont help me what server I am editing image

Expected result: The editor title should somehow allow me to identify the server that I am editing.

Usecase 2

  1. EXEC: pick "Edit Server" in context menu of your server
  2. ASSERT: editor pops up with the following json:
    {
    "server.home.dir": "/Users/adietish/Downloads/runtimes/wildfly-16.0.0.Final",
    "id-set": "false",
    "org.jboss.tools.rsp.server.typeId": "org.jboss.ide.eclipse.as.wildfly.160",
    "id": "wfl16"
    }
  3. EXEC: add a space behind the "false",
  4. EXEC: save the editor

Result: Editor is saved, an info dialog tells me so, but it's missing to tell me what server (I have 4 servers). image

Expected result: The info dialog should tell me what server was saved.

Usecase 3

  1. ASSERT: editor with the server json is opened.
  2. EXEC: change "id-set" to true
  3. EXEC: save the editor

Result: image Error dialog pops up complaining about the "org.jboss.tools.rsp.server.typeId" field. I didnt change that value, I changed the id-set field. From now on, whatever I edit, the error message consistently complains about "org.jboss.tools.rsp.server.typeId"

lstocchi commented 5 years ago

Good points, working on 1 and 2. I already found point 3 on thursday and told Rob who fixed it immediately, maybe you're using an old version of rsp-server (commit https://github.com/redhat-developer/rsp-server/commit/153cf2a9f084d793b6f5698e74edde23ffc46ac7 )

adietish commented 5 years ago

@robstryker I think that validation should ignore elements in a json document that are unchanged. Usecase 3 (see above) seems to imply they are not. Are they?

adietish commented 5 years ago

@lstocchi I preferred the previous context menu item where it was called "Edit" bcs it was a more common/obvious labelling image I think that usecase 1 & 2 can be fixed in vscode while for 3 we might have to fix it in the server. Correct?

adietish commented 5 years ago

"Customize Server" is now back to "Edit Server", I like it much more bcs it's more common in terms of naming, thx! image

adietish commented 5 years ago

@lstocchi Usecase 1: is now better, the editor title now contains the id of the server. Maybe having the id all in front of the name would be even better? image

adietish commented 5 years ago

@lstocchi Usecase 2: when saving you now report the server that was saved, noticeable, valuable info, thx! image

adietish commented 5 years ago

Usecase 3: now the validation correctly reports the first field that errors: image

adietish commented 5 years ago

I filed an issue to add tests for this in #358