-
Notifications
You must be signed in to change notification settings - Fork 326
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
Respond to ThemeChanged
event from IDEs
#8336
Conversation
410edd3
to
bfdc175
Compare
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.
done with first pass
e, | ||
errorType: 'Dart Tooling Daemon connection failed.', | ||
stack: st, | ||
); | ||
}, | ||
); | ||
|
||
if (dtdManager.connection.value != null) { | ||
ThemeManager(dtdManager.connection.value!).listenForThemeChanges(); |
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.
instead of calling a method on a new class to initialize the event handler, perhaps we should add an optional parameter void Function(DtdEvent) onEvent
to dtdManager.connect
where we can pass a callback to initialize an event handler.
This stream subscription will need to be stored on the DTDManager class so that it can be cancelled on disconnect
.
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.
Does this mean editorClient.event
needs to be a var on DTD manager and set to null on disconnect? I'm not sure how to structure this because DtdEditorClient
is within devtools_app
while DTDManager
is in devtools_app_shared
. Does this mean DtdEditorClient
would need to be moved to the shared package as well?
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.
Ah okay I see what you are saying. The only thing that is unfortunate about having this as a separate class is that disposing the stream subscription has to take place separately instead of that being tied to the lifecycle of dtdManger. But this can be done separately.
To do this, mixin add the following to the ThemeManager class signature:
extends DisposableController with AutoDisposeControllerMixin
Then add the stream subscription with addAutoDisposeStreamSubscription
.
And then just make sure you call ThemeManager.dispose
in the FrameworkCore.dispose
method before disposing dtdManager
.
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! Should ThemeManager
be stored in globals.dart
or can it be just in this file if it's not referenced elsewhere?
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.
Just in this file should be fine since it is not referenced elsewhere. Globals are generally used for Objects we want accessible from all of DevTools, and this object is just needed for the framework initialization.
Color? _tryParseColor(String? input) { | ||
if (input == null) return null; | ||
|
||
try { | ||
return parseCssHexColor(input); | ||
} catch (e, st) { | ||
// The user can manipulate the query string so if the value is invalid | ||
// print the value but otherwise continue. | ||
_log.warning( | ||
'Failed to parse "$input" as a color from the querystring, ignoring: $e', | ||
e, | ||
st, | ||
); | ||
return null; | ||
} | ||
} |
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 code is duplicated in ide_theme.dart
. Can we find a way to re-use?
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 moved this to a utility class; hopefully the logger param isn't too awkward
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.
Neat, I'll take a look at adding the postMessage
part for this and wiring up VS Code :-)
packages/devtools_app/lib/src/standalone_ui/vs_code/flutter_panel.dart
Outdated
Show resolved
Hide resolved
I haven't figured it out yet, but when I use this version of DevTools, the sidebar no longer loads in VS Code (it shows a spinner indefinitely - though there are no errors in the console). If I revert to the last commit this was based on, everything works fine. I'll keep debugging, but if anyone has any ideas what could impact this (nothing in the code looks related to me!), please let me know! |
I see the issue... we're creating a second To fix this specific issue, we could ensure the theme manager uses the same |
Some ideas:
|
@DanTup thanks for the debugging and thoughts here! I'm inclined for this PR to do:
It seems odd to create multiple DTD connections for one service (DevTools), and it sounds like options 3 and 4 warrant more consideration outside the scope of this change. What do you think? Also @kenzieschmoll |
@helin24 I started trying to share one, but then found that So as a simple fix to avoid holding this PR up, I suggest we just use DTD directly here (don't use the editor client). It's a tiny change and I can look at refactoring Here's the diff that uses DTD directly:
|
@helin24 @kenzieschmoll btw, just as an FYI in case it comes up... while testing this in VS Code I found a weird bug where query string params (for colors + font size) were being lost. It turns out to be that something in Flutter is decoding encoded I've filed flutter/flutter#155992 about it, but as a workaround, it turns out we will parse hex colours from the querystring with or without the hashes, so I'm updating VS Code to instead of doing (
Just doing:
This fixes the issue I saw (which is that the background color for dark mode was always black, even when the theme had a slightly different dark shade). If IntelliJ also passes background/foreground colors, it may be worth ensuring it doesn't include the |
@helin24 we'll need to also add the (I think you should be able to easily apply this locally with With this PR + that change, the sidebar is working fine in VS Code. (I also fixed that we were using |
@DanTup thanks for the patch! I applied it with copy and paste because I got this error from git: |
This reverts commit ddb45af.
Addresses flutter/flutter-intellij#7626