Hot UI implementation #4160
Hot UI implementation #4160
Conversation
| @@ -0,0 +1,398 @@ | |||
| /* | |||
There was a problem hiding this comment.
This class isn't executed in the checked in code for now as showing previews directly in the code editor is disabled to avoid risking memory leaks or rendering performance issues on slow machines.
| @@ -0,0 +1,37 @@ | |||
| /* | |||
There was a problem hiding this comment.
This file is also disabled behind a non-user configurable flag in the checked in code.
| private final int offset; | ||
| private final int endOffset; | ||
| @Nullable | ||
| class TextRangeTracker { |
There was a problem hiding this comment.
This helper class was extracted out as we now want to track two text ranges per outline location. One to render the ui guides and one that tracks the entire body of the outline node that is used for the inline widget previews.
| @@ -0,0 +1,152 @@ | |||
| /* | |||
There was a problem hiding this comment.
This class isn't enabled in the actual running application as the inline widget previews are disabled without an experimental flag to enable them as described earlier.
| * Copyright 2019 The Chromium Authors. All rights reserved. | ||
| * Use of this source code is governed by a BSD-style license that can be | ||
| * found in the LICENSE file. | ||
| */ |
There was a problem hiding this comment.
This is a refactor of existing code that was directly in the OutlineView class.
| @@ -84,35 +81,30 @@ | |||
| @NotNull | |||
| private final FlutterDartAnalysisServer flutterAnalysisServer; | |||
|
|
|||
| final QuickAssistAction actionCenter; | |||
There was a problem hiding this comment.
All this code for assists is still around it just got moved into a separate toolbar class for reuse across multiple locations.
The reuse across multiple locations isn't actually used in the enabled code so I could go through and pull out that refactor from this CL but the refactor seems reasonable and should be useful in the future showing widget refactors directly in the code editor when the outline view is not enabled.
|
|
||
| private boolean isSettingSplitterProportion = false; | ||
| private Splitter splitter; |
There was a problem hiding this comment.
This CL was based on re-applying the existing PreviewArea code from the OutlineView which was reverted earlier in the year. For example, PreviewArea should sound familiar.
|
|
||
| private final Set<FlutterOutline> outlinesWithWidgets = Sets.newHashSet(); | ||
| private final Map<FlutterOutline, DefaultMutableTreeNode> outlineToNodeMap = Maps.newHashMap(); | ||
|
|
||
| private VirtualFile currentFile; | ||
| private final EventStream<VirtualFile> currentFile; |
There was a problem hiding this comment.
state that needs to be shared with subviews is moved into EventStream classes for easy listening.
|
|
||
| private final WidgetEditToolbar widgetEditToolbar; |
There was a problem hiding this comment.
this is where all the toolbar logic went.
| final ActionManager actionManager = ActionManager.getInstance(); | ||
| final ActionPopupMenu popupMenu = actionManager.createActionPopupMenu(ActionPlaces.UNKNOWN, group); | ||
| popupMenu.getComponent().show(comp, x, y); | ||
| widgetEditToolbar.createPopupMenu(comp, x, y); |
There was a problem hiding this comment.
The popup menu is displayed identically as before, this code has just moved to the toolbar class.
| } | ||
| finally { | ||
| currentEditor.getCaretModel().addCaretListener(caretListener); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private void updateActionsForOutlines(List<FlutterOutline> outlines) { |
There was a problem hiding this comment.
moved to toolbar class.
| @@ -548,45 +501,13 @@ private FlutterOutline getOutlineOfPath(@Nullable TreePath path) { | |||
| return getOutlineOfNode(node); | |||
| } | |||
|
|
|||
| private int getConvertedFileOffset(int offset) { | |||
There was a problem hiding this comment.
these outline location helpers were moved to the outline coverter helper class as they were needed a bunch of places.
| @@ -8,6 +8,7 @@ | |||
| import com.intellij.util.EventDispatcher; | |||
| import com.intellij.util.xmlb.annotations.Attribute; | |||
There was a problem hiding this comment.
All the changes to this file are from re-applying the reverted PreviewArea CL.
| */ | ||
| package io.flutter.preview; | ||
|
|
||
| import com.google.common.util.concurrent.Uninterruptibles; |
There was a problem hiding this comment.
This is a simple refactor of the toolbar code from the PreviewView into its own class.
No functionality was added or changed. The one difference is EventStream objects are used to ensure this class is reliably notified on changes to the selected outline and selected file.
| @@ -249,6 +249,15 @@ | |||
| <toolTipText value="Provides advanced editing capabilities for Java and Kotlin code. Uses Gradle to find Android libraries then links them into the Flutter project."/> | |||
| </properties> | |||
| </component> | |||
| <component id="33088" class="javax.swing.JCheckBox" binding="myEnableHotUiCheckBox"> | |||
There was a problem hiding this comment.
we could also add a checkbox to enable previews in the code editor but that is more risky as described elsewhere.
jacob314
force-pushed
the
hot_ui_implementation
branch
from
5c95d76
to
c5dc61f
Compare
There was a problem hiding this comment.
@stevemessick do you have any tips on where I need to put code that works in IntelliJ but not Android Studio? I need to have separate AndroidStudio and IntelliJ implementations of the color picker. For now I just skip on IntelliJ as I can't figure out the spot in the code base to put classes that will compile under IntelliJ but not android studio.
Here's my android studio specific version.
https://github.com/flutter/flutter-intellij/pull/4160/files#diff-be782206a290e2b3713047e010c87ff8
I'd like to provide an IntelliJ version here but it seems like Android Studio has stripped out the IntelliJ ColorPicker classes replacing them with very similar and slightly improved versions so I get compile errors on "bin/plugin build" when building the android studio version.
https://github.com/flutter/flutter-intellij/pull/4160/files#diff-3974bfa36da17f0302b990dff03077fc
|
|
| // component but for some reason it throws exceptions even though there | ||
| // are examples of it being used identically without throwing exceptions | ||
| // elsewhere in the IntelliJ code base. | ||
| .addSaturationBrightnessComponent() |
There was a problem hiding this comment.
Based on your comment, should this be commented out then?
There was a problem hiding this comment.
Removed this comment. It is copy pasta from my late night port of this code to use the AndroidStudio specific classes. The AndroidStudio version doesn't have the same bug.
| @@ -216,6 +216,7 @@ shout.level.title=Shout | |||
| settings.try.out.features.still.under.development=Try out features still under development (a restart may be required) | |||
| settings.experimental.flutter.logging.view=Replace the Run and Debug console output with a custom Flutter Logging view | |||
| settings.enable.android.gradle.sync=Enable code completion, navigation, etc. for Java / Kotlin (requires restart to do Gradle build) | |||
| settings.enable.hot.ui=Enable property editing in the outline view (some features require Flutter dev channel) | |||
There was a problem hiding this comment.
Maybe "Enable Hot UI editing in the outline view" to brand the Hot UI name? Or "Enable Hot UI - property editing in the outline view"
There was a problem hiding this comment.
How about
"Enable Hot UI -- early preview of property editing in the outline view"
?
|
|
||
| final List<FlutterOutline> children = outlineNode.getChildren(); | ||
| if (children == null || children.isEmpty()) return; | ||
| // if (children == null || children.isEmpty()) return; |
There was a problem hiding this comment.
should this line be removed?
There was a problem hiding this comment.
that optimization can be added back. history is there is a separate block of work to render previews of property values inline split off from this cl.
| /** | ||
| * Class that provides the glue to render the preview view in a regular JPanel. | ||
| */ | ||
| public class PreviewViewModelPanel extends JPanel { |
There was a problem hiding this comment.
same here: rename class, fields, comments to use DeviceMirror instead of PreviewView
There was a problem hiding this comment.
added this to #4162
| }); | ||
| addMouseListener(new MouseListener() { | ||
| @Override | ||
| public void mouseClicked(MouseEvent e) { |
There was a problem hiding this comment.
perhaps add a comment here and for mouseDragged explaining why these methods are overridden and left empty
| final int endOffset = Math.min(startOffset + location.getLength(), docLength); | ||
|
|
||
| marker = document.createRangeMarker(startOffset, endOffset); | ||
| // nodeStartingWord = OutlineLocation.getCurrentWord(document, nameExpression); |
There was a problem hiding this comment.
removed. the helper class for tracking a location now has that functionality.
|
|
||
| void showColorFieldPopup() { | ||
| disposeColorPicker(); | ||
| colorPicker = ColorPickerFactory.create(); |
There was a problem hiding this comment.
Maybe assert(colorPicker == null) before create?
| // The color picker is currently only supported in the AndroidStudio version of the plugin. | ||
| // TODO(jacobr): show a toast message explaining it is missing on IntelliJ for now. | ||
| if (colorPicker != null) { | ||
| colorAtPopupLaunch = currentColor; |
There was a problem hiding this comment.
Can currentColor be passed to the show? and reset when colorPicker is disposed? Since colorPicker is created each time?
There was a problem hiding this comment.
that is a bug. fixed to pass in currentColor.
|
|
||
| public void createPopupMenu(Component comp, int x, int y) { | ||
| // The corresponding tree item may have just been selected. | ||
| // Wait short time for receiving assists from the server. |
There was a problem hiding this comment.
Was this existing code? We wait up to 100ms in 5ms increments?
There was a problem hiding this comment.
Yep. This is completely unchanged from the existing code and example of why we need to do a real audit of all the times we are locking the UI thread from the IntelliJ plugin.
Good news is this case is only triggered on right click in the outline view so it won't cause UI hangs any other times.
| // the action has just been canceled. | ||
| return; | ||
| } | ||
| // TODO(jacobr): how recently was this implemented? |
There was a problem hiding this comment.
DAS was updated in https://dart-review.googlesource.com/c/sdk/+/41520/
Look for the history of DartServerExtractMethodHandler in IntelliJ, somehow I cannot do it locally.
There was a problem hiding this comment.
Looks like that analysis server change landed Feb 14, 2018.
There was a problem hiding this comment.
Great. I've addressed the TODO and verified it still works. Not much changes as IntelliJ just selects the whole block anyway so this is what the user sees.
| screenshotBounds = new Rectangle(lastScreenshotBoundsWindow); | ||
| screenshotBounds.translate(visibleRect.x, visibleRect.y); | ||
| } else { | ||
| boolean lockUpdate =false; |
There was a problem hiding this comment.
nit: need space?
boolean lockUpdate = false;
There was a problem hiding this comment.
fixed. ran the formatter on this file.
| } | ||
|
|
||
| @Override | ||
| TextRange getActiveRange() { |
There was a problem hiding this comment.
I assume null return is okay?
There was a problem hiding this comment.
it is. annotated the base class method as @nullable to get warnings.
| forceRender(); | ||
| } | ||
|
|
||
| // @Override |
There was a problem hiding this comment.
Is this really override or just some comment to be removed?
There was a problem hiding this comment.
Removed the comment. The method still applies but is no longer specified by an interface.
|
Done with the first round of review comments. PTAL. |
| import java.awt.*; | ||
| import javax.swing.*; | ||
|
|
||
| public class ColorPickerAndroidStudio implements GenericColorPicker { |
There was a problem hiding this comment.
Chatted about using a new extension point to create different color picker instances depending on whether the flutter-studio module was loaded.
| assert(colorPicker == null); | ||
| colorPicker = ColorPickerFactory.create(); | ||
| // The color picker is currently only supported in the AndroidStudio version of the plugin. | ||
| // TODO(jacobr): show a toast message explaining it is missing on IntelliJ for now. |
There was a problem hiding this comment.
Hopefully this TODO can be removed.
| static GenericColorPicker create() { | ||
| // TODO(jacobr): provide an implementation of GenericColorPicker that works | ||
| // in IntelliJ. com.intellij.ui.colorpicker.ColorPickerBuilder would work | ||
| // if we could figure out how to make it work with bin/plugin build. |
There was a problem hiding this comment.
Yep, we modify the plugin tool to edit code during the build to handle compile problems. We could use git branches but for the small amount of changes we need to make that is more trouble than it's worth.
There was a problem hiding this comment.
had to expand the way it was modifying code a little to make it work as otherwise I get exceptions. see the stub placeholder file added.
| * The regular IntelliJ color picker implementation can't be used on Android | ||
| * Studio and vice versa. | ||
| */ | ||
| public interface GenericColorPicker { |
There was a problem hiding this comment.
See src/io/flutter/android/GradleSyncProvider.java for an example of defining an extension point.
| // Don't want to render on the wrong editor. This shouldn't happen. | ||
| return; | ||
| } | ||
| if (getEditor().isPurePaintingMode()) { |
There was a problem hiding this comment.
Can getEditor() return a different value here? If not, just use editor.
There was a problem hiding this comment.
The value should never change. However, the rest of the code will assume getEditor() versus passing in the editor to the paint call so I think using getEditor() everywhere is reasonable.
There was a problem hiding this comment.
This weird api is due to supporting RangeHighlighter where the editor is typically passed in rather than tightly coupled to the object as it is in this case.
| } | ||
|
|
||
| @Override | ||
| public void dispose() { |
There was a problem hiding this comment.
I expected to see
isDisposed = true;
isDisposed is initialized but apparently never set.
There was a problem hiding this comment.
so much cruft form all the prototyping that went into this. I've removed isDisposed.
In general the code now follows the IntelliJ best practice of managing disposables by passing in the parent Disposable object wherever possible and not manually call dispose() directly as that leads to bugs.
|
PTAL. All comments so far are addressed. |
|
I'm looking now. I'll focus on the multi-IDE support and build changes. I'll let others cover things they are more familiar with. |
| @@ -1029,10 +1029,19 @@ | |||
| <extensionPoint name="gradleSyncProvider" interface="io.flutter.android.GradleSyncProvider"/> | |||
| </extensionPoints> | |||
|
|
|||
| <extensionPoints> | |||
| <extensionPoint name="colorPickerProvider" interface="io.flutter.editor.ColorPickerProvider"/> | |||
There was a problem hiding this comment.
We should only have one <extensionPoints> tag.
| <extensions defaultExtensionNs="io.flutter"> | ||
| <gradleSyncProvider implementation="io.flutter.android.IntellijGradleSyncProvider" order="last"/> | ||
| </extensions> | ||
|
|
||
| <extensions defaultExtensionNs="io.flutter"> | ||
| <colorPickerProvider implementation="io.flutter.editor.IntellijColorPickerProvider" order="last"/> |
There was a problem hiding this comment.
Similarly, this tag can be combined with the other <extensions> for io.flutter.
| @@ -211,10 +211,19 @@ | |||
| <extensionPoint name="gradleSyncProvider" interface="io.flutter.android.GradleSyncProvider"/> | |||
| </extensionPoints> | |||
|
|
|||
| <extensionPoints> | |||
| <extensionPoint name="colorPickerProvider" interface="io.flutter.editor.ColorPickerProvider"/> | |||
There was a problem hiding this comment.
The comments I made on plugin.xml actually belong here.
| @@ -16,6 +16,10 @@ | |||
| <gradleSyncProvider implementation="io.flutter.android.AndroidStudioGradleSyncProvider" order="first"/> | |||
| </extensions> | |||
|
|
|||
| <extensions defaultExtensionNs="io.flutter"> | |||
| <colorPickerProvider implementation="io.flutter.editor.AndroidStudioColorPickerProvider" order="first"/> | |||
There was a problem hiding this comment.
Like with plugin.xml, combine the blocks.
| @@ -14,6 +14,10 @@ | |||
| <gradleSyncProvider implementation="io.flutter.android.AndroidStudioGradleSyncProvider" order="first"/> | |||
| </extensions> | |||
|
|
|||
| <extensions defaultExtensionNs="io.flutter"> | |||
| <colorPickerProvider implementation="io.flutter.editor.AndroidStudioColorPickerProvider" order="first"/> | |||
There was a problem hiding this comment.
Combine this one with previous, too.
| disposeColorPicker(); | ||
| assert (colorPicker == null); | ||
| colorPicker = ColorPickerProvider.EP_NAME.getExtensionList().get(0); | ||
| ; |
| /*if (expression == null || expression.isEmpty()) { | ||
| continue; | ||
| }*/ |
There was a problem hiding this comment.
Can this be either uncommented or removed?
| public int getEndOffset() { | ||
| if (marker == null) { | ||
| final Location location = attribute.getValueLocation(); | ||
| final int startOffset = attribute.getValueLocation().getOffset(); |
There was a problem hiding this comment.
use location.getOffset() here since you already have attribute.getValueLocation() stored
| // to the column of the actual entity. | ||
| final int docLength = document.getTextLength(); | ||
| final Location location = attribute.getValueLocation(); | ||
| final int startOffset = Math.min(attribute.getValueLocation().getOffset(), docLength); |
| path.add(outline); | ||
| if (converter.getConvertedOutlineOffset(outline) <= offset && offset <= converter.getConvertedOutlineEnd(outline)) { | ||
| if (outline.getChildren() != null) { | ||
| for (FlutterOutline child : outline.getChildren()) { |
There was a problem hiding this comment.
nit: pull outline.getChildren() out into its own variable
jacob314
force-pushed
the
hot_ui_implementation
branch
from
703414a
to
b89d051
Compare
jacob314
force-pushed
the
hot_ui_implementation
branch
from
1231f38
to
ac6616a
Compare
If I had more time I would have written less code.
I've commented on files that are not-enabled in the checked in code and files that are simple refactors of existing code to improve reuse.
High level: support in the outline view is exposed by this CL. Support directly in the code editor is protected by a flag in FlutterSettings that is hard coded to false to reduce the risk of a memory leak or performance regression.
Screenshots:
Setting to enable Hot Ui. Happy to hear suggestions for a better name for this setting. We could also call it Hot UI.
Screenshot showing what it looks like before the app is run.
A reasonable easy UI tweak would be to hide the screen mirror completely when the app is not running.
Screenshot showing when the app is running
Screenshot showing what happens when an outline node that isn't a simple NEW_INSTANCE outline node is selected.
Proof that Hot UI is disabled by default with no changes to the outline view or code editor.
