-
Notifications
You must be signed in to change notification settings - Fork 30
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
Various minor improvements #401
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change looks good to me overall but I wonder whether the focus tracking needs to be correctly initialized. Also the PR has merge conflicts. Other than that, I like the cleanup and various improvements, thank you Tobias!
@@ -29,7 +30,7 @@ export interface FocusChange { | |||
* Allows querying of the current focus state and the focused root diagram element and the currently focused element within the diagram. | |||
*/ | |||
@injectable() | |||
export class FocusTracker implements IActionHandler { | |||
export class FocusTracker implements IActionHandler, IDiagramStartup { | |||
protected inActiveCssClass = 'inactive'; | |||
protected _hasFocus = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if that should really be true now that we actually track the focus change in the beginning?
postModelInitialization(): MaybePromise<void> { | ||
this.handleFocusChange(); | ||
} | ||
|
||
handle(action: Action): void | Action | ICommand { | ||
if (!FocusStateChangedAction.is(action)) { | ||
return; | ||
} | ||
|
||
this._hasFocus = action.hasFocus; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be initialized correctly somehow.
- Merge *-default.ts implementation files of `FeedbackActionDispatcher` and `HelperLineManager` with the the interface definition files. The split is no longer necessary since we resolved potential circular dependency issues - Ensure that we consistently use the `LazyInjector` over dedicated async providers and deprecate existing provider injection - Move MaybeActions utility type from feedback-action-dispatcher into the protocol package to enable reuse + add testcases
9b9fc1b
to
483a189
Compare
Thanks for the review Martin. I decided to revert the focus tracker fix for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with handling the focus tracking in a follow-up issue. Other than that, everything looks good to me, thank you Tobias!
+Fix eclipse-glsp#401 Node creation (eclipse-glsp#402) + Resolve eclipse-glsp#398 Refactor TypeHints API (eclipse-glsp#399) + Resolve eclipse-glsp#396 RouterKind for FeedbackEdge (eclipse-glsp#397) + Reuse Sprotty's request/response system and remove custom solution(eclipse-glsp#404) + Add support for context menu and harmonize with command palette (eclipse-glsp#405)
+Fix eclipse-glsp#401 Node creation (eclipse-glsp#402) + Resolve eclipse-glsp#398 Refactor TypeHints API (eclipse-glsp#399) + Resolve eclipse-glsp#396 RouterKind for FeedbackEdge (eclipse-glsp#397) + Reuse Sprotty's request/response system and remove custom solution(eclipse-glsp#404) + Add support for context menu and harmonize with command palette (eclipse-glsp#405)
What it does
FeedbackActionDispatcher
andHelperLineManager
with the the interface definition files. The split is no longer necessary since we resolved potential circular dependency issuesLazyInjector
over dedicated async providers and deprecate existing provider injectionHow to test
Follow-ups
Changelog