Add support for context menus (#139) #144

planger · 2019-11-05T19:40:49Z

Fixes #139

gitpod-io · 2019-11-05T19:40:53Z

planger · 2019-11-05T19:45:47Z

Please note that the implementation for Theia is provided in eclipse-sprotty/sprotty-theia#47

Unfortunately, these changes are a bit tedious to test as this change spans across sprotty and sprotty-theia, and then needs a Theia-based app that uses these changes. I've used https://github.com/eclipsesource/graphical-lsp/tree/context-menu-support for testing, but again it requires GLSP.

Is there a up-to-date Theia app into which I could integrate this so you can test it more easily (via yarn link)?

JanKoehnlein

Nice.

As there are no tests, could you add a sample context menu to one of the examples?

src/base/types.ts

src/features/context-menu/menu-providers.ts

Signed-off-by: Philip Langer <[email protected]>

planger · 2019-11-22T14:47:03Z

As there are no tests, could you add a sample context menu to one of the examples?

Currently we only have an implementation of the IContextMenuService for Theia. So the example would have to be Theia-based at the moment. Is there a public example that I could use for demonstrating this change together with eclipse-sprotty/sprotty-theia#47? I'm happy to integrate it in such an example.

Alternatively, I could implement the context menu service for plain HTML pages, but that'd require more effort and I'm not sure how useful this would be for real applications, as you'd typically would want to use the same context menu implementation that is also used in the surrounding app (e.g. react-based, angular-based, or whatever).

Currently, this is already integrated in the GLSP workflow example, which is fairly simple. Ignoring server-provided menu items, this would look as follows:

The Theia context menu service must be handed to the sprotty container:
eclipsesource/graphical-lsp@8aaf5ff#diff-6327e0a9b5c1f4e0e6fc0f16300391aa
Context menu item providers have to be created:
eclipsesource/graphical-lsp@8aaf5ff#diff-6f3ee8e876e151ba5cee7ce458b47132R65
Context menu item providers have to be bound:
eclipsesource/graphical-lsp@8aaf5ff#diff-c9f7fdba61179497d2459d5b7848d871R89

Signed-off-by: Philip Langer <[email protected]>

spoenemann

I'm fine with merging this to ease development in downstream projects and examples. We can still change (or even revert) things later.

One public example that would benefit from this feature is the npm dependencies viewer:

https://github.com/TypeFox/npm-dependency-graph

Could you add a context menu to the Theia extension in that example? Be careful not to break the standalone-app, which uses parts of the Sprotty code, but without Theia.

src/base/actions/action.ts

src/features/context-menu/context-menu-service.ts

src/features/context-menu/di.config.ts

Also fixes types of `MenuItem` and adds docs.

planger

One public example that would benefit from this feature is the npm dependencies viewer:

https://github.com/TypeFox/npm-dependency-graph

Could you add a context menu to the Theia extension in that example? Be careful not to break the standalone-app, which uses parts of the Sprotty code, but without Theia.

Ok, sure, I'll clone it and see if I manage to add a context menu there. I'll merge this and wait until it is available on npmjs to avoid headaches with yarn link and Theia (since both sprotty-theia and the npm-dependency-graph point to the next version). I'll open a PR once done.

Thanks! And happy new year! :-)

planger · 2019-12-31T15:32:19Z

@spoenemann I've tried to integrate a context menu to npm-dependency-graph, but once I upgrade to the latest next versions of sprotty, sprotty-theia (and sprotty-elk), I get a couple of errors:

lerna ERR! execute callback with error
lerna ERR! Error: Command failed: yarn run prepare
lerna ERR! error Command failed with exit code 2.
lerna ERR! error Command failed with exit code 2.
lerna ERR! 
lerna ERR! yarn run v1.21.1
lerna ERR! $ yarn run clean && yarn run build
lerna ERR! $ rimraf lib
lerna ERR! $ tsc
lerna ERR! src/browser/graph/graph-layout.ts(18,15): error TS2416: Property 'graphOptions' in type 'DepGraphLayoutConfigurator' is not assignable to the same property in base type 'DefaultLayoutConfigurator'.
lerna ERR!   Type '(sgraph: SGraphSchema, index: SModelIndex<SModelElementSchema>) => LayoutOptions' is not assignable to type '(sgraph: SGraphSchema, index: SModelIndex<SModelElementSchema>) => LayoutOptions | undefined'.
lerna ERR!     Types of parameters 'index' and 'index' are incompatible.
lerna ERR!       Type 'import("/home/philip/Git/OpenSource/npm-dependency-graph/node_modules/sprotty/lib/base/model/smodel").SModelIndex<import("/home/philip/Git/OpenSource/npm-dependency-graph/node_modules/sprotty/lib/base/model/smodel").SModelElementSchema>' is not assignable to type 'import("/home/philip/Git/OpenSource/npm-dependency-graph/depgraph-navigator/node_modules/sprotty/lib/base/model/smodel").SModelIndex<import("/home/philip/Git/OpenSource/npm-dependency-graph/depgraph-navigator/node_modules/sprotty/lib/base/model/smodel").SModelElementSchema>'.
lerna ERR!         Types have separate declarations of a private property 'id2element'.
lerna ERR! src/browser/widget/diagram-commands.ts(49,37): error TS2352: Conversion of type 'ModelSource' to type 'DepGraphModelSource' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
lerna ERR!   Type 'ModelSource' is missing the following properties from type 'DepGraphModelSource': loadIndicator, graphGenerator, graphFilter, postConstruct, and 26 more.
lerna ERR! src/browser/widget/diagram-config.ts(35,38): error TS2345: Argument of type 'typeof TheiaKeyTool' is not assignable to parameter of type 'new (...args: any[]) => KeyTool'.
lerna ERR!   Type 'TheiaKeyTool' is not assignable to type 'KeyTool'.
lerna ERR!     Property 'keyListeners' is protected but type 'TheiaKeyTool' is not a class derived from 'KeyTool'.
lerna ERR! src/browser/widget/diagram-manager.ts(36,31): error TS2352: Conversion of type 'DiagramWidget' to type 'DepGraphWidget' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
lerna ERR!   Type 'DiagramWidget' is missing the following properties from type 'DepGraphWidget': searchBox, attached, diagramType
lerna ERR! src/browser/widget/diagram-manager.ts(38,9): error TS2322: Type 'DepGraphWidget' is not assignable to type 'DiagramWidget'.
lerna ERR!   Types of property 'modelSource' are incompatible.
lerna ERR!     Type 'DepGraphModelSource' is not assignable to type 'ModelSource'.
lerna ERR! src/browser/widget/diagram-widget.ts(25,9): error TS2416: Property 'modelSource' in type 'DepGraphWidget' is not assignable to the same property in base type 'DiagramWidget'.
lerna ERR!   Type 'DepGraphModelSource' is not assignable to type 'ModelSource'.
lerna ERR!     Property 'viewerOptions' is protected but type 'ModelSource' is not a class derived from 'ModelSource'.
lerna ERR! info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
lerna ERR! info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
lerna ERR! 
lerna ERR!     at Promise.all.then.arr (/home/philip/Git/OpenSource/npm-dependency-graph/node_modules/execa/index.js:236:11)
lerna ERR!     at <anonymous>
{ Error: Command failed: yarn run prepare
error Command failed with exit code 2.
error Command failed with exit code 2.

I can look through those errors and try to fix them, but I probably won't be able to do that until the week of the 7th of Jan. Are you planning to upgrade npm-dependency-graph anyway? If yes, it'd be probably more efficient if I wait (since I don't really know the npm-dependency-graph).

Thanks!

BjBe82 · 2020-05-10T12:08:58Z

I was interested in a context menu too so i spend some time to create a simple one using Sprotty UIExtension as well as the IContextMenuItemProvider and IContextMenuService api.

If you are interested i adapted the implementation for the mindmap example here: BjBe82@c87eac0

planger · 2020-05-10T17:48:54Z

I was interested in a context menu too so i spend some time to create a simple one using Sprotty UIExtension as well as the IContextMenuItemProvider and IContextMenuService api.

If you are interested i adapted the implementation for the mindmap example here: BjBe82@c87eac0

Very interesting, thanks!

Imho it would be great to have a generic, Theia-independent implementation like yours in Sprotty for standalone deployments of Sprotty.

@spoenemann What do you think?

@BjBe82 Would you be interested in opening a PR where you put your context menu implementation (UI extension and ContextMenuService implementation) into an optional module in Sprotty and just use it in the mind-map example? The context menu provider should remain in the mindmap example.

Thanks for letting us know!

spoenemann · 2020-05-11T06:20:32Z

I'm not sure how useful it is in a stand-alone scenario. IMO a context menu is something for complex applications such as IDEs. "Normal" websites do not have custom context menus.

JanKoehnlein · 2020-05-11T07:06:00Z

Into the bargain, the surrounding framework often provides context menus and we should try to use that one to not look to alien, e.g. VS Code and Theia Desktop use the native context menu.

BjBe82 · 2020-05-11T12:19:32Z

My intention was not to provide an extension of the sprotty api in that case because i agree with @spoenemann and @JanKoehnlein that advanced frameworks might provide a context menu implementation.

But for a small prototype i was interested in the sprotty context menu api so i created my own quite simple rendering.

Here i just tried to provide a very lightweight example which could be added to the sprotty default examples if you are interested. Not saying that the rendering part is good or bad .... just to show that there is sprotty api around context menus and the fact that you discussed a “simple” example in this topic.

spoenemann · 2020-05-11T12:24:02Z

I'm fine with adding it to the mindmap example.

planger mentioned this pull request Nov 5, 2019

Add Theia implementation for Sprotty context menu eclipse-sprotty/sprotty-theia#47

Merged

JanKoehnlein requested changes Nov 22, 2019

View reviewed changes

src/base/types.ts Outdated Show resolved Hide resolved

src/features/context-menu/menu-providers.ts Outdated Show resolved Hide resolved

planger added 2 commits November 22, 2019 15:12

Add support for context menus (eclipse-sprotty#139)

63fac01

Signed-off-by: Philip Langer <[email protected]>

Fix symbol name and sort symbol list

3624568

Signed-off-by: Philip Langer <[email protected]>

planger force-pushed the issues/139 branch from 477c98e to 3624568 Compare November 22, 2019 14:24

planger requested a review from JanKoehnlein November 22, 2019 14:59

Add test for flattening menu items

b8d29bc

Signed-off-by: Philip Langer <[email protected]>

spoenemann approved these changes Dec 18, 2019

View reviewed changes

src/base/actions/action.ts Show resolved Hide resolved

src/features/context-menu/context-menu-service.ts Outdated Show resolved Hide resolved

src/features/context-menu/di.config.ts Show resolved Hide resolved

planger added 2 commits December 31, 2019 14:39

Merge remote-tracking branch 'origin' into issues/139

9bd2989

Add context menu to default modules

1ec561d

Also fixes types of `MenuItem` and adds docs.

planger commented Dec 31, 2019

View reviewed changes

planger merged commit 2629dae into eclipse-sprotty:master Dec 31, 2019

planger deleted the issues/139 branch December 31, 2019 14:24

This was referenced Jan 1, 2020

Use context menu from Sprotty eclipse-glsp/glsp-client#24

Merged

Use context menu from Sprotty eclipse-glsp/glsp-theia-integration#12

Merged

Use context menu from Sprotty eclipse-glsp/glsp-examples#22

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for context menus (#139) #144

Add support for context menus (#139) #144

planger commented Nov 5, 2019

gitpod-io bot commented Nov 5, 2019 •

edited

Loading

planger commented Nov 5, 2019

JanKoehnlein left a comment

planger commented Nov 22, 2019

spoenemann left a comment

planger left a comment •

edited

Loading

planger commented Dec 31, 2019

BjBe82 commented May 10, 2020 •

edited

Loading

planger commented May 10, 2020

spoenemann commented May 11, 2020

JanKoehnlein commented May 11, 2020

BjBe82 commented May 11, 2020

spoenemann commented May 11, 2020

Add support for context menus (#139) #144

Add support for context menus (#139) #144

Conversation

planger commented Nov 5, 2019

gitpod-io bot commented Nov 5, 2019 • edited Loading

planger commented Nov 5, 2019

JanKoehnlein left a comment

Choose a reason for hiding this comment

planger commented Nov 22, 2019

spoenemann left a comment

Choose a reason for hiding this comment

planger left a comment • edited Loading

Choose a reason for hiding this comment

planger commented Dec 31, 2019

BjBe82 commented May 10, 2020 • edited Loading

planger commented May 10, 2020

spoenemann commented May 11, 2020

JanKoehnlein commented May 11, 2020

BjBe82 commented May 11, 2020

spoenemann commented May 11, 2020

gitpod-io bot commented Nov 5, 2019 •

edited

Loading

planger left a comment •

edited

Loading

BjBe82 commented May 10, 2020 •

edited

Loading