-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Exposed preserveFocus
on OutputChannel#show
.
#8243
Conversation
We usually use nvm it seems to be before like that too |
import { Emitter, Event, Disposable, DisposableCollection } from '@theia/core'; | ||
import { QuickPickItem } from '@theia/core/lib/common/quick-pick-service'; | ||
import { MonacoEditorModel } from '@theia/monaco/lib/browser/monaco-editor-model'; | ||
import { MonacoTextModelService, IReference } from '@theia/monaco/lib/browser/monaco-text-model-service'; | ||
import { OutputUri } from './output-uri'; | ||
import { OutputCommands } from '../browser/output-contribution'; | ||
import { OutputCommands, OutputContribution } from '../browser/output-contribution'; |
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.
dependency to browser code from common
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.
Thanks for keeping an eye on everything ;) It's a known issue: #4306
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.
Do you want me to move the modules from common
to browser
? I do not mind moving them now, but it's a breaking change. This PR most likely won't make it to the next release anyways.
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.
As you want.
@inject(QuickPickService) | ||
protected readonly quickPickService: QuickPickService; | ||
|
||
@inject(MonacoTextModelService) | ||
protected readonly textModelService: MonacoTextModelService; | ||
|
||
@inject(OutputContribution) | ||
protected readonly outputContribution: OutputContribution; |
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 don't think that channel should know anything about contribution managing it
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.
Could you give some pointers besides saying you do not like this approach? Thanks!
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.
sorry, I commented here: #8243 (comment)
this.channelWasShownEmitter.fire({ name }); | ||
const reveal = true; | ||
const activate = !preserveFocus; | ||
await this.outputContribution.openView({ reveal, activate }); |
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 think the original idea was that channel represents the state which the contribution should listen to and react. The channel should NOT depend on concrete views.
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.
OK, this makes sense. 👍
if (model && model.uri.toString() === selectedChannel.uri.toString()) { | ||
if (!preserveFocus) { | ||
MessageLoop.sendMessage(editorWidget, Widget.Msg.ActivateRequest); |
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.
usually activation is done via ApplicationShell.activate
. It makes sure to activate all widgets in parent-child order.
It is strange that the widget itself reacts and try to update visibility/focus without knowing visibility/activation state of its parents.
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.
activate all widgets in parent-child order.
Hmm, that's interesting. We use the MessageLoop.sendMessage(myWidget, Widget.Msg.ActivateRequest);
construct overalll the application:
- view container,
- terminal,
- scm,
- siw,
- editor preview.
What's wrong with this one?
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 see; you're right. Let me check why it was needed. Thanks for the heads-up.
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.
Are you sure? I think it is the only place where we use it.
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.
Yes, you're right.
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.
Widget should implement how it is activated in onActivateRequest
, but activation is done via ApplicationShell.activate
. It will analyze what are parents (areas, tabs, composite widgets) of widgets and reveal/activate them first after that it will ask the widget to activate.
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.
It does not seem to be caused by your changes though, so probably out of the scope to refactor it. I guess it works because there is another path running in parallel making sure that the output view is visible and activated.
f26e217
to
b2bf7b1
Compare
Thanks for the feedback, @akosyakov. I updated the PR. |
@@ -16,26 +16,83 @@ | |||
import { FrontendApplicationContribution } from '@theia/core/lib/browser'; | |||
import { inject, injectable, interfaces } from 'inversify'; | |||
import { OutputChannelManager, OutputChannelSeverity } from '@theia/output/lib/common/output-channel'; | |||
import { CommandContribution, CommandRegistry } from '@theia/core/lib/common/command'; |
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.
the name of this file should be changed now since it does not only test the severities, right?
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.
Good point, but the corresponding commit will be dropped from the PR.
@@ -93,13 +94,16 @@ export class OutputWidget extends BaseWidget implements StatefulWidget { | |||
this.onStateChangedEmitter.fire(this._state); | |||
} | |||
|
|||
protected async refreshEditorWidget(): Promise<void> { | |||
protected async refreshEditorWidget({ preserveFocus }: { preserveFocus: boolean } = { preserveFocus: false }): Promise<void> { |
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.
why not have boolean argument instead of object with single boolean property?
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.
We can easily extend it without breaking the API in the future; it's pretty common that we use an object instead of a single property. I can make the change, though. I am OK with both.
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 ok with both if we think it might expand in the future. consider having a named interface so it is more readable.
Tested new functionality and it work. Recent severity changes also seems to work. |
Thanks a lot for your help, @amiramw 🙏 |
CHANGELOG.md
Outdated
@@ -20,6 +20,7 @@ Breaking Changes: | |||
<a name="1_4_0_deprecate_languages"></a> | |||
- [[languages]](#1_4_0_deprecate_languages) `@theia/languages` extension is deprecated, use VS Code extensions to provide language smartness: | |||
https://code.visualstudio.com/api/language-extensions/language-server-extension-guide [#8112](https://github.com/eclipse-theia/theia/pull/8112) | |||
- [output] `OutputWidget#setInput` has been removed. The _Output_ view automatically shows the channel when calling `OutputChannel#show`. Moved the `OutputCommands` namespace from the `output-contribution` to its dedicated `output-commands` module to overcome a DI cycle. [#8243](https://github.com/eclipse-theia/theia/pull/8243) |
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.
we could simple reference a command by string? commands' ids are apis and cannot be changed easily anyway
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.
code changes look to me
This PR is on hold until the next release; 30.7. Edit: I am going to drop the API-sample commit after the release: #8243 (comment) |
We should not forget to move changes to 1.5.0 in CHANGELOG for such PRs. |
Sure. It will be a conflict after the release. |
From now on, calling `show` with `preserveFocus` `true` will ensure that the Output widget is revealed but not activated. The default behavior remains `preserveFocus` `false` which will reveal and activate the widget. Removed `OutputWidget#setInput`; it is automatically wired into `show`. Signed-off-by: Akos Kitta <kittaakos@typefox.io>
b2bf7b1
to
003113f
Compare
What it does
preserveFocus
onOutputChannel#show
. From now on, callingshow
withpreserveFocus
true
will ensure that the Output widget is revealed but not activated. The default behavior remainspreserveFocus
false
which will reveal and activate the widget.OutputWidget#setInput
; it is automatically wired intoshow
.How to test
Reset the workbench layout.
Run
Show channel (command, explicit, preserve-focus)
-> Output view is revealed but not active.Reset the workbench layout.
Run
Show channel (API, explicit, preserve-focus)
-> Output view is revealed but not active.Run
Hide Output Channel...
, select theAPI Sample: my test channel
-> The Output is active but the input is not theAPI Sample: my test channel
.Run
Show Output Channel...
, select theAPI Sample: my test channel
-> The Output is active and the input is theAPI Sample: my test channel
.Reset the workbench layout.
Run
Show channel (command, implicit, no preserve-focus)
-> Output is active.Reset the workbench layout.
Run
Show channel (command, explicit, no preserve focus)
-> Output is active.Reset the workbench layout.
Run
Show channel (API, implicit, no preserve-focus)
-> Output is active.Reset the workbench layout.
Run
Show channel (API, explicit, no preserve-focus)
-> Output is active.Review checklist
Reminder for reviewers