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

rework scm, recover git extension #5416

Merged
merged 8 commits into from
Jun 13, 2019
Merged

rework scm, recover git extension #5416

merged 8 commits into from
Jun 13, 2019

Conversation

akosyakov
Copy link
Member

@akosyakov akosyakov commented Jun 11, 2019

Generally for this PR

  • functionality of Theia git extension should work as before (in 0.6.0 release). It should be verified by reviewers.
  • and corresponding functionality in VS Code git extension should work as well. It is nice to verify but not necessary since it never was working on master.

Things to test:

  • keyboard navigation between resources and resource changes
  • opening a resource with keyboard
  • Git status bar integration
  • multiple repos support (new repos are picked up, old are deleted)
  • toolbar actions (inlined and more)
  • commit with keyboard
  • resource and resource group actions from context and inline menus
  • preserving state on reload (input message, selected repository)
  • input validation (empty box, no staged resources)
  • check logs for errors especially for bogus rewatching of git repos
  • issues fixed below

Fixed issues

UI changes for Theia git extension to align with VS Code

  • there is no Commit button anymore but a check in the toolbar:

Screen Shot 2019-06-11 at 14 43 13

- git diff editors have shorter titles instead of using `lalala (abc123) <-> lalala (HEAD)`

Screen Shot 2019-06-11 at 14 43 10

Let me know if you prefer to put Commit button back (we will need to make it generally then for VS Code git extension as well) and have more verbose diff editor titles.

Theia Git extension UI

Screen Shot 2019-06-11 at 09 43 55

VS Code Git extension UI

Screen Shot 2019-06-11 at 09 49 47

@@ -98,6 +98,13 @@
"@phosphor/signaling" "^1.2.2"
"@phosphor/virtualdom" "^1.1.2"

"@primer/octicons-react@^9.0.0":
Copy link
Member Author

Choose a reason for hiding this comment

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

@marcdumais-work Could you please run your tool on new deps to check whether everything is alright? 🙏

We hope to test and merge today/tomorrow and then release 0.7.2.

Copy link
Contributor

Choose a reason for hiding this comment

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

@akosyakov I did the "deep" check, recursively covering all production dependencies for the whole repo/PR, and everything looks good.

Copy link
Member Author

Choose a reason for hiding this comment

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

@marcdumais-work thank you

@akosyakov akosyakov changed the title WIP rework scm, recover git extension rework scm, recover git extension Jun 11, 2019
@akosyakov
Copy link
Member Author

akosyakov commented Jun 11, 2019

Theia git extension is ready for review. I will add some change for VS Code extension to fix triggering commit from the keyboard and improve decorations, but they should not have significant impact.

@akosyakov
Copy link
Member Author

looking into failing tests...

@svenefftinge
Copy link
Contributor

Looking into it now.
First thing I noticed is that the focussed commit message field is not highlighted blue.

@akosyakov
Copy link
Member Author

First thing I noticed is that the focussed commit message field is not highlighted blue.

will do, but it was not highlighted blue before 😛 you can check in Gitpod prod

@kittaakos
Copy link
Contributor

kittaakos commented Jun 11, 2019

First off, it works reliably, I cannot see the repository watcher starting for each file I open in the editor 👍 I collected a few things, none of them are critical. The decorator behavior is odd, however.

commit message field

Screen Shot 2019-06-11 at 15 47 14
Screen Shot 2019-06-11 at 15 47 36


  • [minor] We do not need the separator below the message field. VS Code does not have it either.

Screen Shot 2019-06-11 at 15 48 47


  • [minor] The initial input size is larger than necessary.

Screen Shot 2019-06-11 at 15 50 22


  • [bug] It seems the Git decorations work based on the editor and not the selected repository. See the attached screen-cast:

screencast 2019-06-11 15-53-46


  • [minor] Use noselect.

Screen Shot 2019-06-11 at 16 02 34


  • [bug] Navigation to left is broken. The last item is ignored. It works when navigating right.

screencast 2019-06-11 16-52-00


Screen Shot 2019-06-11 at 16 59 28

@kittaakos
Copy link
Contributor

Steps to reproduce:

  • Empty workspace.
  • Open SCM view. You can see source control is not available at this time.
  • Clone a repository into the workspace.
scm-amend-component.tsx:15 Uncaught (in promise) Error: Request 'log' failed
    at Proxy.<anonymous> (proxy-factory.ts:232)
    at GitAmendSupport.<anonymous> (git-scm-provider.ts:414)
    at step (git-scm-provider.ts:15)
    at Object.next (git-scm-provider.ts:15)
    at git-scm-provider.ts:15
    at new Promise (<anonymous>)
    at push.../../packages/git/lib/browser/git-scm-provider.js.__awaiter (git-scm-provider.ts:15)
    at GitAmendSupport.push.../../packages/git/lib/browser/git-scm-provider.js.GitAmendSupport.getLastCommit (git-scm-provider.ts:413)
    at ScmAmendComponent.<anonymous> (scm-amend-component.tsx:285)
    at step (scm-amend-component.tsx:15)
ResponseError @ messages.js:46
handleResponse @ main.js:430
processMessageQueue @ main.js:258
(anonymous) @ main.js:242
run @ setImmediate.js:40
runIfPresent @ setImmediate.js:69
onGlobalMessage @ setImmediate.js:109
Promise.then (async)
step @ scm-amend-component.tsx:15
(anonymous) @ scm-amend-component.tsx:15
push.../../packages/scm/lib/browser/scm-amend-component.js.__awaiter @ scm-amend-component.tsx:15
push.../../packages/scm/lib/browser/scm-amend-component.js.ScmAmendComponent.componentDidMount @ scm-amend-component.tsx:87
commitLifeCycles @ react-dom.development.js:14362
commitAllLifeCycles @ react-dom.development.js:15463
callCallback @ react-dom.development.js:100
invokeGuardedCallbackDev @ react-dom.development.js:138
invokeGuardedCallback @ react-dom.development.js:187
commitRoot @ react-dom.development.js:15604
completeRoot @ react-dom.development.js:16619
performWorkOnRoot @ react-dom.development.js:16564
performWork @ react-dom.development.js:16483
performSyncWork @ react-dom.development.js:16455
requestWork @ react-dom.development.js:16355
scheduleWork$1 @ react-dom.development.js:16219
scheduleRootUpdate @ react-dom.development.js:16786
updateContainerAtExpirationTime @ react-dom.development.js:16813
updateContainer @ react-dom.development.js:16840
../../node_modules/react-dom/cjs/react-dom.development.js.ReactRoot.render @ react-dom.development.js:17123
legacyRenderSubtreeIntoContainer @ react-dom.development.js:17278
render @ react-dom.development.js:17318
../../packages/core/lib/browser/widgets/react-widget.js.ReactWidget.onUpdateRequest @ react-widget.tsx:41
push.../../packages/scm/lib/browser/scm-widget.js.ScmWidget.onUpdateRequest @ scm-widget.tsx:146
../../node_modules/@phosphor/widgets/lib/widget.js.Widget.processMessage @ widget.js:489
invokeHandler @ index.js:433
sendMessage @ index.js:169
runMessageLoop @ index.js:483
requestAnimationFrame (async)
enqueueMessage @ index.js:452
postMessage @ index.js:220
../../node_modules/@phosphor/widgets/lib/widget.js.Widget.update @ widget.js:351
../../node_modules/@phosphor/widgets/lib/tabbar.js.TabBar._onTitleChanged @ tabbar.js:881
invokeSlot @ index.js:475
emit @ index.js:433
../../node_modules/@phosphor/signaling/lib/index.js.Signal.emit @ index.js:106
set @ title.js:89
push.../../packages/scm/lib/browser/scm-widget.js.ScmWidget.refresh @ scm-widget.tsx:104
(anonymous) @ scm-widget.tsx:94
(anonymous) @ event.ts:119
../../packages/core/lib/common/event.js.CallbackList.invoke @ event.ts:127
../../packages/core/lib/common/event.js.Emitter.fire @ event.ts:220
set @ scm-service.ts:78
push.../../packages/scm/lib/browser/scm-service.js.ScmService.registerScmProvider @ scm-service.ts:100
push.../../packages/git/lib/browser/git-repository-provider.js.GitRepositoryProvider.registerScmProvider @ git-repository-provider.ts:190
push.../../packages/git/lib/browser/git-repository-provider.js.GitRepositoryProvider.updateRepositories @ git-repository-provider.ts:179
(anonymous) @ git-repository-provider.ts:159
step @ git-repository-provider.ts:15
(anonymous) @ git-repository-provider.ts:15
fulfilled @ git-repository-provider.ts:15
Promise.then (async)
step @ git-repository-provider.ts:15
fulfilled @ git-repository-provider.ts:15
Promise.then (async)
step @ git-repository-provider.ts:15
(anonymous) @ git-repository-provider.ts:15
push.../../packages/git/lib/browser/git-repository-provider.js.__awaiter @ git-repository-provider.ts:15
push.../../packages/git/lib/browser/git-repository-provider.js.GitRepositoryProvider.refresh @ git-repository-provider.ts:149
(anonymous) @ git-repository-provider.ts:78
invokeFunc @ index.js:160
trailingEdge @ index.js:207
timerExpired @ index.js:195
setTimeout (async)
leadingEdge @ index.js:168
debounced @ index.js:235
(anonymous) @ git-repository-provider.ts:75
(anonymous) @ event.ts:119
../../packages/core/lib/common/event.js.CallbackList.invoke @ event.ts:127
../../packages/core/lib/common/event.js.Emitter.fire @ event.ts:220
push.../../packages/filesystem/lib/browser/filesystem-watcher.js.FileSystemWatcher.onDidFilesChanged @ filesystem-watcher.ts:148
onDidFilesChanged @ filesystem-watcher.ts:120
../../packages/core/lib/common/messaging/proxy-factory.js.JsonRpcProxyFactory.onNotification @ proxy-factory.ts:180
(anonymous) @ proxy-factory.ts:137
handleNotification @ main.js:483
processMessageQueue @ main.js:255
(anonymous) @ main.js:242
run @ setImmediate.js:40
runIfPresent @ setImmediate.js:69
onGlobalMessage @ setImmediate.js:109
postMessage (async)
registerImmediate @ setImmediate.js:120
setImmediate @ setImmediate.js:27
triggerMessageQueue @ main.js:240
callback @ main.js:289
../../node_modules/vscode-ws-jsonrpc/lib/socket/reader.js.WebSocketMessageReader.readMessage @ reader.ts:57
(anonymous) @ reader.ts:18
../../packages/core/lib/common/messaging/web-socket-channel.js.WebSocketChannel.handleMessage @ web-socket-channel.ts:49
WebSocketConnectionProvider.socket.onmessage @ ws-connection-provider.ts:74

@kittaakos
Copy link
Contributor

We have to document it. Please add one single sentence about the change to the doc:

https://github.com/theia-ide/theia/blob/29fb37626f1a3e20e7c81e95a411926e24561990/packages/core/src/browser/status-bar/status-bar.tsx#L37

Copy link
Contributor

@svenefftinge svenefftinge left a comment

Choose a reason for hiding this comment

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

I have tested this and it works very well. Only read through 50% of the code so far but that was very good, too. thank you!

@kittaakos
Copy link
Contributor

Commit message input sizing (re-sizing) issues:
screencast 2019-06-11 17-21-12

packages/core/src/browser/shell/tab-bar-toolbar.tsx Outdated Show resolved Hide resolved
@@ -14,6 +14,8 @@
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
********************************************************************************/

// tslint:disable:no-any
Copy link
Contributor

Choose a reason for hiding this comment

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

[minor]: I would prefer having three distinct no-any instead of disabling this rule for the entire file.

@paul-marechal
Copy link
Member

Just shimming in as I see the comments and approvals already, but CI is failing because of missing bindings in the tests.

@AlexTugarev
Copy link
Contributor

Works pretty well @akosyakov!

Small issue with * and + in the checkout statusbar item? It looks like the git status used to decide on the indicator is off by one.
2019-06-12 08 14 43

@akosyakov
Copy link
Member Author

[minor] We should use ⌘ instead of Cmd on macOS.

@kittaakos Do you know hot to achieve it? I am using kebinding service to get an accelerator. It seems to be the generic issue.

@kittaakos
Copy link
Contributor

I am using kebinding service to get an accelerator. It seems to be the generic issue.

I have opened a follow-up: #5427.

@akosyakov
Copy link
Member Author

It seems the Git decorations work based on the editor and not the selected repository. See the attached screen-cast.

@AlexTugarev Can you comment it? Was it always the case? git-diff module was not touched at all by scm changes.

@kittaakos
Copy link
Contributor

Can you comment it? Was it always the case?

It works differently with the "master" state of Theia in Gitpod.

If I have two repositories, A and B. If I set A as the selected repository and I modify a file which belongs to B; neither the editor diff nor the Git change decorators are visible for my modified file. After selecting B, the opened editor will show the diff, and the navigator will have the Git decorators.

@AlexTugarev
Copy link
Contributor

ScmDecorationsService doubles the logic of a manager for dirty diffs. It should either stay in the git extension, or be moved properly to scm.

@AlexTugarev
Copy link
Contributor

2019-06-12 15 00 52

status indicator in statusbar works fine now!

@akosyakov akosyakov force-pushed the ak/scm_rework branch 2 times, most recently from 8fc6f8d to 8379164 Compare June 12, 2019 13:48
@akosyakov
Copy link
Member Author

Input field should be highlighted as focused now when selected and should not flicker anymore. cc @svenefftinge @kittaakos

@akosyakov
Copy link
Member Author

@kittaakos Would it be fine to tackle #5416 (comment) separately? Could you file an issue please? please point which repo you have used, i've tried with monaco-languageclient and could not reproduce

Copy link
Contributor

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this Anton!

@lmcbout
Copy link
Contributor

lmcbout commented Jun 12, 2019

@akosyakov I tested with the "commit 8379164"
for the issue #5338 with this PR. It looks like the multiple "start/stop watching the git repository" looks OK . Only one start at the beginning, I also test performance when starting electron; since there is only one watch signature, starting electron/browser is in fact faster, no more wait for the git repo to be ready.it is ready right away.
+1 Great job

@@ -102,6 +102,10 @@
padding: 4px 4px 4px 4px;
}

.theia-scm-input-message-container textarea[placeholder] {
text-overflow: ellipsis;
Copy link
Contributor

@kittaakos kittaakos Jun 12, 2019

Choose a reason for hiding this comment

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

This does not seem to work in Chrome but FF :/ I will rework it.

It works now. I have verified the behavior with the default layout in Safari, FF, and Chrome.

akosyakov and others added 8 commits June 12, 2019 19:55
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
+ align view options handling with VS Code
+ fix preserving focus for documents opened by plugins

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
… to align with VS Code

e.g. diff URIs should be unwrapped

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Aligned the `p-debounce` version between `@theia/debug` and `@theia/scm`
Aligned styles (`padding`) with VS Code.
Got rid of the `messageBoxHeight` property.

Signed-off-by: Akos Kitta <kittaakos@typefox.io>
Otherwise, the placeholder won't fit into the default side-panel width
and multi line `textarea` will be rendered instead.

This CSS style adjusts the `textarea` behavior to VS Code.

Signed-off-by: Akos Kitta <kittaakos@typefox.io>
@kittaakos
Copy link
Contributor

@akosyakov, I am going to force push to this branch. Sorry :(

@akosyakov
Copy link
Member Author

@kittaakos it is completely fine! Pease merge if you are fine with the PR!

@kittaakos
Copy link
Contributor

I did another cycle; It worked nicely ✨ Thank you for the fix!

@akosyakov
Copy link
Member Author

Thanks for all testing!

@akosyakov akosyakov merged commit db8c807 into master Jun 13, 2019
@akosyakov akosyakov deleted the ak/scm_rework branch June 13, 2019 04:47
vince-fugnitto added a commit that referenced this pull request Jun 25, 2019
Now that `octicons` are supported in the statusbar thanks to
#5416, the icon for
`running tasks...` is now updated to match that of its vscode counterpart.

Signed-off-by: Vincent Fugnitto <vincent.fugnitto@ericsson.com>
vince-fugnitto added a commit that referenced this pull request Jun 25, 2019
Now that `octicons` are supported in the statusbar thanks to
#5416, the icon for
`running tasks...` is now updated to match that of its vscode counterpart.

Signed-off-by: Vincent Fugnitto <vincent.fugnitto@ericsson.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants