jdaugherty commented on code in PR #439:
URL:
https://github.com/apache/grails-intellij-plugin/pull/439#discussion_r4211760463
##########
plugin/src/main/java/org/apache/grails/intellij/plugin/projectView/nodes/GrailsPsiDirectoryNode.java:
##########
@@ -104,15 +104,23 @@ protected void updateImpl(@NotNull PresentationData data)
{
}
/**
- * {@code PsiDirectoryNode.updateImpl} adds coloured fragments for module
content roots; ordinary
- * directories use {@code setPresentableText}, including qualified names
such as {@code grails-app.i18n}.
- * Gradle's per-source-set modules make {@code src/test} a content root,
whose fragments can render as
- * {@code test [app.test]}. The renderer prefers those fragments over {@code
presentableText}, so
- * setting only the latter to {@code Tests:unit} does not change that
content-root label.
+ * {@code PsiDirectoryNode.updateImpl} adds coloured fragments only when the
directory is a module
+ * content root ({@code ProjectRootsUtil.isModuleContentRoot}). Gradle's
per-source-set modules make
+ * {@code src/test} one, so it arrives carrying fragments that can render as
{@code test [app.test]}, and
+ * the renderer prefers those over {@code presentableText} — setting only
the latter to
+ * {@code Tests:unit} leaves the content-root label visible.
*
- * <p>{@code postprocess} applies the custom title to both the updated
presentation and the template.
- * Replacing any platform fragments and setting {@code presentableText}
keeps the title consistent
- * for both content roots and ordinary directories.
+ * <p>For every other directory the platform calls {@code
setPresentableText} and adds no fragments at
+ * all, so the qualified name such as {@code grails-app.i18n} comes from
+ * {@code ProjectViewDirectoryHelper.getNodeName} through that field, not
from a fragment. That is why
+ * the Views, Configuration and Initialization titles always rendered
without this hook. Do not go
+ * looking for directory-name fragments on a nested directory: there are
none.
+ *
+ * <p>Replacing any platform fragments and setting {@code presentableText}
keeps the title consistent
+ * for both content roots and ordinary directories. {@code postprocess} is a
choice rather than a
+ * requirement: the platform has already written the label by the time
either hook's body runs, because
+ * {@code super.updateImpl} is the first statement of the override, so
clearing the fragments in
+ * {@code updateImpl} would work equally well.
Review Comment:
This isn't right: `postprocess` is required, and moving the write into
`updateImpl` would bring back the bug #432 fixed.
`super.updateImpl` does write the platform label first, but `updateImpl`
isn't the last thing that touches the presentation. In
`AbstractPsiBasedNode.doUpdate` (262.10315.125, lines 198–200) the order is:
```java
updateImpl(data);
data.setIcon(patchIcon(...));
CompoundProjectViewNodeDecorator.get(myProject).decorate(this, data);
```
and `postprocess` only runs afterwards, from
`PresentableNodeDescriptor.getUpdatedPresentation`. Decorators therefore sit
between the two hooks. In a Gradle-linked project (every Grails 7 app),
`GradleModuleDirectoryDecorator` runs on every `PsiDirectoryNode` that is a
module content root (`GradleDirectoryDecoration.computeDecoration`):
- for a source-set module it takes `coloredText.first()`, clears, and
re-adds it in bold;
- when the source-set name differs from the directory name
(`appendModuleName`) it rewrites the label to `"<presentableText> [shortName]"`.
A title written in `updateImpl` is restyled in the first case and suffixed
in the second. Only `postprocess` comes after the decorators, so it is the only
hook where the title is final.
I checked this with a decorator shaped like the Gradle one, registered on
the `com.intellij.projectViewNodeDecorator` project extension point, and called
`node.update()`:
- this PR's head (title in `postprocess`): `[Tests:unit]`
- the same code with the title write moved to the end of `updateImpl`:
`[Tests:unit , [app.test]]`
The existing tests can't see this because they call
`updateImpl`/`postprocess` directly and skip the decorators. Separately, the
764a3ea commit message says the `updateImpl` variant "passes every test in
GrailsPsiDirectoryNodeTest", but at this head
`testTitleReplacesTheModuleContentRootFragments` fails with that change. Its
"platform fragments must still contain the original content-root label"
assertion fails.
Please replace this paragraph (and the matching AGENTS.md text) with
something like: *"`postprocess` is required: `ProjectViewNodeDecorator`s run
between `updateImpl` and `postprocess`, and in Gradle projects
`GradleModuleDirectoryDecorator` rewrites content-root fragments, so a title
set in `updateImpl` would be restyled or suffixed with `[module]`."* Please
also add a regression test so a later refactor can't quietly move the write:
```java
public void testTitleSurvivesProjectViewDecorators() {
PsiDirectory directory =
findDirectoryCreatedBy("src/test/ExampleSpec.groovy");
ModuleRootModificationUtil.updateModel(getModule(), model ->
model.addContentEntry(directory.getVirtualFile()));
// Same shape as GradleModuleDirectoryDecorator's appendModuleName branch.
ProjectViewNodeDecorator gradleLike = (node, data) -> {
String name = data.getPresentableText();
data.clearText();
data.addText(name + " ", SimpleTextAttributes.REGULAR_ATTRIBUTES);
data.addText("[app.test]", SimpleTextAttributes.REGULAR_BOLD_ATTRIBUTES);
};
getProject().getExtensionArea().getExtensionPoint("com.intellij.projectViewNodeDecorator")
.registerExtension(gradleLike, getTestRootDisposable());
GrailsPsiDirectoryNode node = new GrailsPsiDirectoryNode(directory,
ViewSettings.DEFAULT, null,
NodeWeights.TESTS_FOLDER, "Tests:unit", null, "src/test");
node.update();
assertEquals(List.of("Tests:unit"),
drawnFragments(node.getPresentation()));
}
```
This test passes on this branch and fails once the write is moved into
`updateImpl`.
##########
AGENTS.md:
##########
@@ -162,13 +162,18 @@ both ordinary directories and module content roots, which
the platform presents
**Module content-root labels use coloured fragments; ordinary directories use
`presentableText`.**
In platform 262.10315.125, `PsiDirectoryNode.updateImpl` adds coloured
fragments only when
-`ProjectRootsUtil.isModuleContentRoot` is true. For other directories it calls
`setPresentableText`
-with the name from `ProjectViewDirectoryHelper.getNodeName`, which can be
qualified (`grails-app.i18n`).
+`ProjectRootsUtil.isModuleContentRoot` is true *and* the file resolves to at
least one module; a content
+root that resolves to none falls through to the `presentableText` path too.
For those directories it calls
+`setPresentableText` with the name from
`ProjectViewDirectoryHelper.getNodeName`, which can be qualified
+(`grails-app.i18n`).
Gradle's per-source-set modules make `src/test` a module content root, so its
label can be
`test [app.test]` in fragments. The renderer prefers a non-empty fragment list
over `presentableText`;
setting only the latter to `Tests:unit` leaves the content-root label visible.
-`GrailsPsiDirectoryNode.postprocess` replaces any fragments and sets
`presentableText` on both
-the updated presentation and the template, keeping custom titles consistent
for both kinds of directory.
+`GrailsPsiDirectoryNode.postprocess` replaces any fragments and sets
`presentableText`, keeping custom
+titles consistent for both kinds of directory. `postprocess` itself is a
choice rather than a requirement:
+the platform has written the label by the time either hook's body runs,
because `super.updateImpl` is the
+first statement of the override, so clearing the fragments in `updateImpl`
would work equally well. Do not
+treat `updateImpl` as too early: `super.updateImpl` writes the label before
the rest of the override runs.
Review Comment:
Same issue as the inline comment on `GrailsPsiDirectoryNode.java`:
`updateImpl` *is* too early. Every `ProjectViewNodeDecorator` runs after it and
before `postprocess`, and in Gradle projects `GradleModuleDirectoryDecorator`
rewrites content-root fragments. Please say here that `postprocess` is required
and why, since this is the section the next reader will rely on.
##########
AGENTS.md:
##########
@@ -162,13 +162,18 @@ both ordinary directories and module content roots, which
the platform presents
**Module content-root labels use coloured fragments; ordinary directories use
`presentableText`.**
In platform 262.10315.125, `PsiDirectoryNode.updateImpl` adds coloured
fragments only when
-`ProjectRootsUtil.isModuleContentRoot` is true. For other directories it calls
`setPresentableText`
-with the name from `ProjectViewDirectoryHelper.getNodeName`, which can be
qualified (`grails-app.i18n`).
+`ProjectRootsUtil.isModuleContentRoot` is true *and* the file resolves to at
least one module; a content
Review Comment:
Nit: this qualifier is correct, but the `GrailsPsiDirectoryNode` javadoc
still says fragments are added "only when the directory is a module content
root" without it. It would be good to state the rule the same way in both
places. Also, the "resolves to no modules" fall-through can only happen when
`CodeInsightContexts.isSharedSourceSupportEnabled` is on. Otherwise `modules`
is `Collections.singletonList(fi.getModuleForFile(...))`, which is never empty.
##########
plugin/src/test/java/org/apache/grails/intellij/plugin/projectView/impl/GrailsNodeProviderTestSupport.java:
##########
@@ -100,7 +100,10 @@ protected static void assertTitleAndLocation(@Nullable
GrailsPsiDirectoryNode no
/**
* Reads a node the way the renderer does: {@code updateImpl} then {@code
postprocess}. The second call
Review Comment:
Nit: "`updateImpl` then `postprocess`" leaves out the step that makes this
helper necessary. `update()` runs `updateImpl`, then the project-view
decorators, then `postprocess`. That's why going through `update()` reflects
what the renderer sees, and calling the two hooks directly doesn't.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
