Apply vscode Timeline Plugin API #7997

vinokurig · 2020-06-11T10:26:05Z

What it does

Add Timeline Plugin API that controls the Timeline view.

fixes #7577

How to test

Remove "@theia/git": "^1.2.0" from examples/browser/package.json file
Download vscode-git-1.44.2 extension which requires the Timeline API and copy it to the plugins folder
Compile and start theia with a project cloned from github.

After closing and reopening the widget, TimeLine widget exists as single object:

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

kittaakos · 2020-06-12T09:26:21Z

examples/browser/package.json

@@ -48,6 +48,7 @@
    "@theia/search-in-workspace": "^1.2.0",
    "@theia/task": "^1.2.0",
    "@theia/terminal": "^1.2.0",
+    "@theia/timeline": "^1.2.0",


Let's keep the browser app in sync with the electron one.

packages/plugin-ext/src/plugin/timeline.ts

akosyakov · 2020-06-13T10:02:16Z

Is it somehow based on the current git history implementation or there is no sense in reusing it?

akosyakov

I did the first round of code review.

packages/timeline/src/browser/timeline-widget.tsx

akosyakov · 2020-06-13T10:21:50Z

packages/timeline/src/browser/timeline-widget.tsx

+        this.containerLayout.addWidget(this.timelineEmptyWidget);
+
+        this.refresh();
+        this.timelineService.onDidChangeTimeline(event => {


always collect all listeners into this.toDispose

Please profile and check that when you close the widget that it does not leak. Please add it to How to test section for reviewers.

Done, updated the description. It is interesting that TimelineTreeWidget still duplicates:

I don't have any ideas why it happens because scmTreeWidget binded in the same way but it doesn't have this problem. Do you have any ideas?

packages/timeline/src/browser/timeline-service.ts

packages/timeline/src/browser/timeline-tree-widget.tsx

packages/timeline/src/browser/timeline-empty-widget.tsx

akosyakov · 2020-06-13T10:57:40Z

packages/timeline/src/browser/timeline-contribution.ts

+export class TimelineContribution implements FrontendApplicationContribution {
+
+    @inject(FileNavigatorContribution)
+    protected readonly explorer: FileNavigatorContribution;


It is used to add the timeline widget to explorer:

theia/packages/timeline/src/browser/timeline-contribution.ts

Line 61 in 5a0eec1

explorer.addWidget(timeline, { initiallyCollapsed: true });

I could not find any usages, please check again. Please do it by using find references, not find in text.

removed the leftovers

akosyakov · 2020-06-13T10:59:40Z

packages/timeline/src/browser/timeline-contribution.ts

+    protected readonly timelineService: TimelineService;
+
+    async onDidInitializeLayout?(app: FrontendApplication): Promise<void> {
+        const explorer = await this.widgetManager.getWidget(EXPLORER_VIEW_CONTAINER_ID);


We should instead listen when widgetManager fires onWillCreate event and if a new widget is explorer then we shoudl participate and add the timeline, otherwise we eagerly create a widget even if a user does not see it.

See PluginViewRegistry how we add views on demand.

Now, the timeline widget is created only when timeline provider is registered:

theia/packages/timeline/src/browser/timeline-contribution.ts

Lines 57 to 61 in 5a0eec1

this.timelineService.onDidChangeProviders( async event => {

if (explorer instanceof ViewContainer) {

if (event.added && event.added.length > 0 && explorer.getTrackableWidgets().indexOf(timeline) === -1) {

timeline = await this.widgetManager.getOrCreateWidget(TimelineWidget.ID);

explorer.addWidget(timeline, { initiallyCollapsed: true });

getWidget creates a new widget eagerly, so timeline is only added once in onDidInitializeLayout.

We should instead participate in creation of the explorer view container and add the timline widget each time via widgetManager.onWillCreate event. Try to completely close and open the explorer tab.

vinokurig · 2020-06-15T07:27:12Z

@akosyakov @kittaakos Thank you for your review. I will be able to answer all your comments after 30-th june, because I am on PTO right now.

akosyakov · 2020-06-15T13:16:45Z

@vinokurig enjoy your time off, there is no hurry!

akosyakov · 2020-06-29T07:47:26Z

@westbury I would like to have your look at it. I think you had ideas how to incorporate git history view in the timeline according to #8084. Do you see why it should not be necessary?

azatsarynnyy · 2020-06-30T14:39:38Z

I've done all the steps as described in How to test section but I don't see Timeline panel

git extension is present

I've just removed @theia/git and added the provided vsix. Are there some additional steps required?

vinokurig · 2020-07-01T11:12:21Z

@azatsarynnyy

I've done all the steps as described in How to test section but I don't see Timeline panel

I've fixed the plugin link, please download the the patched plugin and replace it.

azatsarynnyy · 2020-07-01T13:55:24Z

I rebuilt the sources, re-downloaded the git vsix. But Timeline panel is still not present 🤷

westbury · 2020-07-01T22:42:44Z

Thanks for doing the initial contribution of the timeline. I have a couple of questions.

I would expect there to be a ViewContribution so that the view appears in the Open View etc.
It appears you have used ScmWidget as the template for TimelineWidget, so you have ended up with TimelineTreeWidget. However the timeline is inherently flat, not a tree. I would have used ScmHistoryWidget as the template. The history is the principal use for the timeline, perhaps for a while the only use. I am interested to know your reasons for using ScmWidget as the template.

vinokurig · 2020-07-02T08:23:27Z

@azatsarynnyy

I rebuilt the sources, re-downloaded the git vsix. But Timeline panel is still not present

Sorry, updated the link again, now it should work

westbury · 2020-07-02T08:56:12Z

I think you had ideas how to incorporate git history view in the timeline according to #8084. Do you see why it should not be necessary?

I had assumed that the git history view would be updated to use the timeline view. However I have now realized that the timeline view is designed to show items for a single file only, so perhaps using the same view is not a good idea (though our git package will need to contribute to the timeline). Also we have more flexibility if git history is a different view. For example I experimented with adding a graph of branches to the history view (though I had not got it into a state good enough for a PR).

akosyakov · 2020-07-02T09:06:13Z

Timeline will need to support folders eventually as well: microsoft/vscode#95332 Not sure why we need to support double code doing the same and giving different UX.

westbury · 2020-07-02T09:22:00Z

Not sure why we need to support double code doing the same

We would use the same base code

and giving different UX

When I am looking at file or folder history, I am typically looking into understanding the details of the code in that file or folder. When I am looking at repository history I am more likely to be managing commits (rebasing, merging etc). Different work, different UX. Maybe it can all be shoehorned into the timeline.

azatsarynnyy · 2020-07-02T12:37:31Z

@azatsarynnyy

I rebuilt the sources, re-downloaded the git vsix. But Timeline panel is still not present

Sorry, updated the link again, now it should work

Thanks @vinokurig! It works now )

When I'm calling Open Changes command, it says:

Is it expected?

westbury · 2020-07-02T12:45:04Z

When I'm calling Open Changes command, it says:

Is it expected?

That's a known issue, not related to this PR. I thought there was an issue for it but I could not find it so I will create one.

azatsarynnyy · 2020-07-06T11:21:57Z

packages/timeline/package.json

+    "@theia/core": "^1.2.0",
+    "@theia/editor": "^1.2.0",
+    "@theia/navigator": "^1.2.0"


should be ^1.3.0

vinokurig · 2020-07-14T13:18:51Z

@akosyakov @westbury Looks like I have answered all your comments, could you please review the PR again?

packages/timeline/src/browser/timeline-contribution.ts

packages/plugin-ext/src/plugin/timeline.ts

packages/plugin-ext/src/common/plugin-api-rpc.ts

akosyakov · 2020-08-28T07:10:22Z

Could someone test and approve it if it is fine? Rather than that it is fine with me.

westbury

I have tested this and I have noticed the following:

The 'load-more' does not really work very well. I have to repeatedly press it to get another 20 items. I have a portrait oriented screen that shows 150 lines so this takes a few presses. Then when my timeline section is full, I scroll down. However as soon as I try to scroll it resets and shows only 20 items again. So I can't see the bottom items.
The view is missing a few things which are shown is vs-code. That is ok as we don't need to implement everything in the first commit. However two things that would be good to get in fairly soon, preferably before the 1.6 release, are a) put the file name in the title and b) show the timestamp for each item.
I still think this should not be based on TreeWidget. A timeline is not a tree and never can be (unless one lives in Winden). @akosyakov mentioned Timeline support for folders microsoft/vscode#95332 but, even if Microsoft were to accept that request, the timeline will still be a chronological list of commits that affect a folder, not a tree. Furthermore a timeline may have branches. I know that neither the timeline API nor Theia's history view currently support branches but I don't want to preclude supporting branches. Also there are over 1400 lines in TreeWidget that support trees yet all we use is selection and cursor up and down. The tree widget causes problems, for example cursor-left does nothing while cursor-right moves down, not a major bug and easily fixed but indicative of problems that can occur.

Overall though this is good and we need to get it merged. The only significant problem is the load-more action mentioned above. Ultimately I think this would work better if we used react-virtualized, so more items are fetched as the user scrolls, as is done in Theia's history view. I therefore don't think we need to spend too much effort perfecting this now. Most of the load-more problems are not visible to the user if PAGE_SIZE is increased, 200 should be plenty enough for most users' screen sizes. So I am happy to approve this with PAGE_SIZE at 200 and, of course, rebased and squashed.

packages/timeline/src/browser/timeline-widget.tsx

packages/timeline/src/browser/timeline-tree-model.ts

vinokurig · 2020-09-01T11:27:56Z

@westbury

The 'load-more' does not really work very well. I have to repeatedly press it to get another 20 items. I have a portrait oriented screen that shows 150 lines so this takes a few presses. Then when my timeline section is full, I scroll down. However as soon as I try to scroll it resets and shows only 20 items again. So I can't see the bottom items.

I've fixed it and other issues in the latest commit.

The view is missing a few things which are shown is vs-code. That is ok as we don't need to implement everything in the first commit. However two things that would be good to get in fairly soon, preferably before the 1.6 release, are a) put the file name in the title and b) show the timestamp for each item.

I'll file an issue with this improvements when it is merged.

I still think this should not be based on TreeWidget. A timeline is not a tree and never can be (unless one lives in Winden). @akosyakov mentioned Timeline support for folders microsoft/vscode#95332 but, even if Microsoft were to accept that request, the timeline will still be a chronological list of commits that affect a folder, not a tree. Furthermore a timeline may have branches. I know that neither the timeline API nor Theia's history view currently support branches but I don't want to preclude supporting branches. Also there are over 1400 lines in TreeWidget that support trees yet all we use is selection and cursor up and down. The tree widget causes problems, for example cursor-left does nothing while cursor-right moves down, not a major bug and easily fixed but indicative of problems that can occur.

I prefer to keep it as a tree because in this case we just extend the basic tree widget, so the implementation is simpler. Otherwise it would require a new widget from scratch or moving the git-history widget to some common place and extending it which can produce even more bugs. The problems that you described are fixed in the latest commit.

vinokurig · 2020-09-04T09:09:44Z

@westbury any updates on this?

westbury · 2020-09-04T22:39:26Z

I've tested it again and I have found the following issues:

I cannot get the 'Load-More' action to work. For me nothing happens when I click on it. Actually, I have now figured out how to get it to work: It works only if one clicks on it before anything else in the timeline. If one clicks on an item then clicks on 'Load-More' then it does not work.
When I select a file in explorer, the timeline is not updated. I have to then click in the editor. It was not obvious to me that I needed to click in the editor to update the timeline. The timeline in the explorer should match the file selection in the explorer, like it does in vscode.
If I cursor down to 'Load-More' and press the Enter key, nothing happens.
If, immediately after the timeline is updated to another file, I select an item then I see both the item and the 'Load-More' rows selected. This may be related to 1. above because the 'Load-More' action does not work once in this state.

vinokurig · 2020-09-07T11:01:06Z

@westbury I've fixed it again, so your cases are fixed now except the last one. The load more node is selected by default:

theia/packages/timeline/src/browser/timeline-tree-model.ts

Line 57 in e30056a

selected: true

so I think it is a bug in the tree widget. I can file an issue about the default selection is not lost after selecting other nodes after merging it to be able to provide a use-case in the issue. WDYT?

westbury · 2020-09-07T11:50:40Z

Thanks for fixing those issues.

I think it is a bug in the tree widget

I think the tree widget is working as designed. The problem is that the Load-More item has the selected flag set but is not registered in the selection service. If you look at the transiteTo function in tree-selection-impl.ts then you will see where the old selection is deselected. The Load-More item should appear in oldNodes on line 79, at which point it would be deselected.

westbury · 2020-09-07T11:57:04Z

packages/timeline/src/browser/timeline-service.ts

+        }
+
+        this.providers.delete(id);
+        this.providerSubscriptions.delete(id);


Surely you should be disposing the listener here? You make the effort to keep the listener disposables in this.providerSubscriptions but you don't do anything with them.

I've fixed that by disposing the provider directly instead of storing disposables in a map.

vinokurig · 2020-09-07T14:34:40Z

@westbury
Thank you for the advice with the selection-service. I've registered the load-more node in the selection service and it doesn't keep the selection any more if another node is selected. Could you please review it again?

westbury

Great. It tests well and I did not see any memory leaks. Thanks for fixing that last issue. Just remove the unnecessary inject and I will be happy for this to be merged.

westbury · 2020-09-07T21:53:03Z

packages/timeline/src/browser/timeline-tree-model.ts

@@ -30,6 +30,8 @@ export interface TimelineNode extends SelectableTreeNode {
 @injectable()
 export class TimelineTreeModel extends TreeModelImpl {

+    @inject(TreeSelectionService) protected readonly selectionService: TreeSelectionService;


Not necessary. This is injected into TreeModelImpl.

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

See theia's [changelog](https://github.com/eclipse-theia/theia/blob/v1.16.0/CHANGELOG.md#v1160---7292021), we update from 1.13.0 to 1.16.0. Changes specific to slapos port: * We set `warnOnPotentiallyInsecureHostPattern` because the new warning is very intrusive ... but it might actually be very insecure. For now we don't have solution. We are tracking this in https://erp5js.nexedi.net/#/bug_module/20210819-20614EE * New libsecret component that is now required by theia * Switch to using vscode git instead of theia git, to use timeline ( see eclipse-theia/theia#7997 ) * globally version up slapos.core to https://pypi.org/project/slapos.core/1.6.18/ See merge request nexedi/slapos!1039

vinokurig added plug-in system issues related to the plug-in system vscode issues related to VSCode compatibility labels Jun 11, 2020

vinokurig requested review from akosyakov, azatsarynnyy and paul-marechal and removed request for akosyakov and azatsarynnyy June 11, 2020 10:26

kittaakos reviewed Jun 12, 2020

View reviewed changes

akosyakov reviewed Jun 13, 2020

View reviewed changes

packages/plugin-ext/src/plugin/timeline.ts Show resolved Hide resolved

akosyakov mentioned this pull request Jun 13, 2020

scm tree is not always correct #8004

Closed

akosyakov reviewed Jun 13, 2020

View reviewed changes

akosyakov requested a review from westbury June 29, 2020 07:46

azatsarynnyy reviewed Jul 6, 2020

View reviewed changes

akosyakov reviewed Aug 21, 2020

View reviewed changes

packages/timeline/src/browser/timeline-contribution.ts Outdated Show resolved Hide resolved

akosyakov reviewed Aug 21, 2020

View reviewed changes

packages/plugin-ext/src/plugin/timeline.ts Outdated Show resolved Hide resolved

akosyakov reviewed Aug 21, 2020

View reviewed changes

packages/plugin-ext/src/common/plugin-api-rpc.ts Outdated Show resolved Hide resolved

vinokurig force-pushed the theia-7577 branch 6 times, most recently from d38a9a3 to 3d594e2 Compare August 26, 2020 09:33

westbury reviewed Aug 30, 2020

View reviewed changes

packages/timeline/src/browser/timeline-widget.tsx Show resolved Hide resolved

packages/timeline/src/browser/timeline-tree-model.ts Show resolved Hide resolved

vinokurig force-pushed the theia-7577 branch 3 times, most recently from 3987218 to e30056a Compare September 1, 2020 10:09

vinokurig force-pushed the theia-7577 branch from 6ecc67d to bc2e375 Compare September 7, 2020 10:41

westbury reviewed Sep 7, 2020

View reviewed changes

vinokurig force-pushed the theia-7577 branch from 15827e2 to ad7a249 Compare September 7, 2020 15:12

westbury approved these changes Sep 7, 2020

View reviewed changes

Apply Timeline plugin API

af95c30

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

vinokurig force-pushed the theia-7577 branch from 66ef68d to af95c30 Compare September 8, 2020 08:20

vinokurig merged commit 35d4780 into master Sep 8, 2020

vinokurig deleted the theia-7577 branch September 8, 2020 09:18

vinokurig mentioned this pull request Sep 8, 2020

Timeline panel improvements #8480

Open

3 tasks

	this.timelineService.onDidChangeProviders( async event => {
	if (explorer instanceof ViewContainer) {
	if (event.added && event.added.length > 0 && explorer.getTrackableWidgets().indexOf(timeline) === -1) {
	timeline = await this.widgetManager.getOrCreateWidget(TimelineWidget.ID);
	explorer.addWidget(timeline, { initiallyCollapsed: true });

Apply vscode Timeline Plugin API #7997

Apply vscode Timeline Plugin API #7997

Conversation

vinokurig commented Jun 11, 2020 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov commented Jun 13, 2020

akosyakov left a comment

Choose a reason for hiding this comment

akosyakov Jun 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov Aug 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinokurig commented Jun 15, 2020

akosyakov commented Jun 15, 2020

akosyakov commented Jun 29, 2020 • edited Loading

azatsarynnyy commented Jun 30, 2020

vinokurig commented Jul 1, 2020

azatsarynnyy commented Jul 1, 2020

westbury commented Jul 1, 2020 • edited Loading

vinokurig commented Jul 2, 2020

westbury commented Jul 2, 2020 • edited Loading

akosyakov commented Jul 2, 2020 • edited Loading

westbury commented Jul 2, 2020

azatsarynnyy commented Jul 2, 2020

westbury commented Jul 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinokurig commented Jul 14, 2020

akosyakov commented Aug 28, 2020

westbury left a comment

Choose a reason for hiding this comment

vinokurig commented Sep 1, 2020

vinokurig commented Sep 4, 2020

westbury commented Sep 4, 2020

vinokurig commented Sep 7, 2020

westbury commented Sep 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinokurig commented Sep 7, 2020

westbury left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinokurig commented Jun 11, 2020 •

edited

Loading

akosyakov Jun 13, 2020 •

edited

Loading

akosyakov Aug 21, 2020 •

edited

Loading

akosyakov commented Jun 29, 2020 •

edited

Loading

westbury commented Jul 1, 2020 •

edited

Loading

westbury commented Jul 2, 2020 •

edited

Loading

akosyakov commented Jul 2, 2020 •

edited

Loading

westbury left a comment •

edited

Loading