Skip to content

[ui] XPath result highlighting on the designer#1192

Merged
jsotuyod merged 44 commits into
pmd:masterfrom
oowekyala:designer-node-highlighting
Jun 24, 2018
Merged

[ui] XPath result highlighting on the designer#1192
jsotuyod merged 44 commits into
pmd:masterfrom
oowekyala:designer-node-highlighting

Conversation

@oowekyala

@oowekyala oowekyala commented Jun 16, 2018

Copy link
Copy Markdown
Member

Changelog

  • Highlight XPath results on the main code area

  • Refactor the styling logic in the code area to simplify it and be less
    wasteful. There are now two kinds of code areas. One handles only syntax
    highlighting, and one adds layer management logic. This removes the
    need for a separate StyleContext, which was a serious case of Feature Envy
    and prevented the abstraction of the code area.

    • Layers are identified using the constants of an enum for external code.
    • Nodes are styled in batches instead of one by one. Previously,
      to highlight 100 nodes, we'd have created 100 StyleSpans with one
      node in each and overlaid them all on each subsequent paintCss (including
      when syntax highlighting changes, ie potentially tens of times a second).
      Since when we want to style many nodes, typically most of them share the
      same style (eg one focus node, 100 XPath results), we batch all nodes that
      share the same style into a UniformStyleCollection. When we want to update
      the style of the area, a single StyleSpans is computed for each style
      collection, which takes the number of spans to overlay from potentially many
      hundreds to only 3-4. Flattening the nodes of a style collection into a
      StyleSpans takes care of nesting depth and assigns a special CSS class for
      each depth. Styling is still taken care of in the CSS
    • Syntax highlighting updates now don't trigger the overlaying of all the style
      layers. That's because, for our main editor area, each text change
      triggers a reparsing anyway, which will update the positions of the
      highlight spans to the correct one. If syntax highlighting triggered an
      overlay, then we'd be overlaying outdated spans, which causes visible
      twitches in the colors
    • The code area doesn't need to manage an ExecutorService field, it's now
      captured in a closure, which streamlines the process of switching syntax
      highlighters
  • XPath results are now displayed with rich text in the list view.

  • The recent files changed order on each startup, which is now fixed

  • We use the newest version of RichtextFX because of Consecutive border/underline styles that are the same are rendered with multiple shapes, not one FXMisc/RichTextFX#709

  • We now use a Less compiler to compile our stylesheets. I expect this to
    greatly simplify the maintenance of the stylesheets, though I didn't touch
    the main stylesheet for now to not overclutter the PR

  • Cosmetic updates: mainly, the focus node is now bold instead of blue, to keep
    syntax highlighting visible

  • Scrolling to the focus node now makes the whole node visible if possible

  • Line numbers on the main code area now mark the currently selected node

Future work

  • When the code is reparsed, node selection is lost (and so is the node info +
    highlighting). We could at least try to find the old selection in the new tree
    to make the experience more continuous
  • Highlighting parse errors would be really nice. But the current layering
    design only allows highlighting nodes, so we need to abstract the layering algorithm
    to not handle only nodes.
  • It occurred to me, that we could consider making XPath a PMD language, so
    that we can perform sanity checks/ flag possible simplifications directly in
    the XPath area. I don't think it would be too hard to implement per se,
    and it could provide more incentive to pursue [core] RFC: Analyzing embedded snippets from other languages #237 if we gather some interesting rules.

oowekyala added 30 commits June 16, 2018 00:15
Styling improvements. The previous paradigm for highlighting nodes
was to change the fill and make it bold, I'm now more inclined to
change the background/box or just use bold weight for the focus
node.

With syntax highlighting, it's more readable
We scroll so that the node is made entirely visible
It previously was reevaluated on an out-of-date compilation
unit. It makes no sense to keep an out-of-date root node in
the AstManager so we now use Optional to indicate there is
no compilation unit.
Cache hit rate is great when you're inspecting the AST or XPath results without changing the text
This caused unexpected behaviour for dubious performance improvements
@oowekyala oowekyala added an:enhancement An improvement on existing features / rules in:ui labels Jun 16, 2018
@oowekyala oowekyala added this to the 6.5.0 milestone Jun 16, 2018
oowekyala added a commit to pmd/build-tools that referenced this pull request Jun 16, 2018
@oowekyala

Copy link
Copy Markdown
Member Author

The build fails because I needed to update a rule in pmd/build-tools. Probably as soon as tomorrow, Travis will use the latest artifact and the build will succeed.

We turn off the css logger, and allow RichTextFX to access internal javafx API to avoid
the illegal access warning.
Doesn't compile on Java 9. We may be able to use reflection to bypass this,
but I leave it for future work
Comment thread pmd-ui/pom.xml
<artifactId>lesscss-maven-plugin</artifactId>
<version>1.7.0.1.1</version>
<configuration>
<sourceDirectory>${project.basedir}/src/main/resources/net/sourceforge/pmd/util/fxdesigner/less</sourceDirectory>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as these files are in the src/main/resources folder they are still copied into the final jar. We should probably exclude them from resource processing.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I'll do that right away

sourceEditorController.clearNameOccurences();

Optional.ofNullable(declaration.getNode().getScope().getDeclarations().get(declaration))
.ifPresent(sourceEditorController::highlightNameOccurrences);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like an over-complex way of just doing if null (allocating an object along the way). I love functional, but feel this is more an abuse than a proper use. My totally personal and biased opinion 'though, so do as you feel is best.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum yes I get your point. I have a tendency to abuse functional style a bit, more often than not it's terser -- but somewhat less clear.

Unrelated thought: I think, that the big call chain completely breaks encapsulation: declaration.getNode().getScope().getDeclarations().get(declaration)
We should probably brush up the symbol table API.

@jsotuyod jsotuyod merged commit 9706cd0 into pmd:master Jun 24, 2018
@oowekyala oowekyala deleted the designer-node-highlighting branch June 25, 2018 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

an:enhancement An improvement on existing features / rules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants