Problem/Motivation

The four View panels form this chain today:

IslandPluginBase
└── BuilderPanel           (id: builder,  "Canvas")      743 lines
    ├── TreePanel          (id: tree,     "Navigator")   171 lines
    └── WireframePanelBase (abstract)                    246 lines
        └── ScaffoldPanel  (id: scaffold, "Scaffold")    116 lines

BuilderPanel is the Canvas and the base class of every other View panel, so it holds two unrelated jobs:

Concern Methods Used by
View panel base buildRootDropzone(), digFromSlot(), buildComponentSlot(), buildNodeAttributes(), buildSlotAttributes(), resolveComponentInfo(), replaceNode(), the island event handlers all four panels
Canvas real render buildComponentRealRender(), renderSource(), useAttributesVariable(), hasMultipleRoot(), isEmpty(), buildSingleBlock() and its placeholder policy Canvas, plus one call from ScaffoldPanel

TreePanel and WireframePanelBase override both buildSingleComponent() and buildSingleBlock(), then inherit roughly 350 lines of Canvas rendering they never call. Consequences we already hit:

  • Every improvement to the Canvas's placeholder and message policy lands, dead, in three panels that cannot reach it. The block placeholder work did exactly this.
  • BuilderPanel is the largest island by 190 lines and keeps growing, because it is where anything shared has to go.
  • Two services injected for one branch of one Canvas method are constructed for every View panel on every builder page.

Nothing here is user-visible. It is a maintenance problem: the class that new contributors read first is the one where the least of it applies to them.

Proposed resolution

  1. Add ViewPanelBase (abstract, src/Plugin/display_builder/Island/) holding the shared half of the table above, moved verbatim, names unchanged.
  2. Move the Canvas real render into a trait, e.g. RealRenderTrait: buildComponentRealRender(), renderSource(), useAttributesVariable(), hasMultipleRoot(), the render-emptiness check.
  3. BuilderPanel extends ViewPanelBase, uses the trait, and keeps buildSingleBlock() and the placeholder policy, which are its own.
  4. TreePanel extends ViewPanelBase and WireframePanelBase extends ViewPanelBase.
  5. ScaffoldPanel uses the trait as well — see the trap below.

The trap. ScaffoldPanel::buildSingleComponent() calls $this->buildComponentRealRender() for its allowlisted layout components, and parent::buildSingleComponent() for everything else. Moving WireframePanelBase off BuilderPanel removes the first of those unless the real render is reachable from Scaffold. That is the reason for the trait rather than a plain base class split.

Cleanup found on the way, worth folding in: BuilderPanel::isEmpty() duplicates RenderableBuilderTrait::isRenderEmptyOrFailing(), which the class already has through IslandPluginBase and which PreviewPanel calls. Drop the private copy instead of moving it.

Remaining tasks

  • Add ViewPanelBase and RealRenderTrait, move the methods, no behaviour change in the move commit.
  • Re-parent TreePanel and WireframePanelBase; wire ScaffoldPanel to the trait.
  • Delete BuilderPanel::isEmpty() in favour of the trait method.
  • Kernel: BuilderPanelTest, TreePanelTest, ScaffoldPanelTest, DropzoneTest must pass unchanged. If one needs editing, the move was not behaviour-neutral.
  • Playwright: tree.spec.ts, plus a pass over the four View panels in a browser: drag between panels, contextual menu, delete, undo.
  • phpcbf, php-cs-fixer, PHPStan on the module, cspell.

User interface changes

None. No plugin id, island attach_to, profile config, keyboard shortcut or stored data changes.

API changes

Internal to the module, but real for anyone subclassing a View panel: the parent of TreePanel and WireframePanelBase changes, so a custom panel extending either and relying on Canvas-only methods would need the trait. Method names and signatures are otherwise unchanged. The module is beta; noting it in the release notes is enough.

Data model changes

None.

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

mogtofu33 created an issue. See original summary.

mogtofu33’s picture

Assigned: mogtofu33 » pdureau
Status: Needs work » Needs review
Issue tags: +display_builder-1.0.0-beta7
mogtofu33’s picture

Issue tags: +AI-accelerated
pdureau’s picture

Assigned: pdureau » mogtofu33
Status: Needs review » Reviewed & tested by the community

Add ViewPanelBase (abstract, src/Plugin/display_builder/Island/) holding the shared half of the table above, moved verbatim, names unchanged.

Cool, it will make the work on BuilderPanel safer.

So, we now have:

IslandPluginBase
└── ViewPanelBase           (abstract)
    └── BuilderPanel           (id: builder,  "Canvas") use RealRenderTrait
    ├── TreePanel          (id: tree,     "Navigator")
    └── WireframePanelBase (abstract) 
        └── ScaffoldPanel  (id: scaffold, "Scaffold") use RealRenderTrait

I move to RTBC but, before merging, can we check?

  • the purpose of WireframePanelBase, only extended by ScaffoldPanel, in this new classes hierarchy?
  • is #3594176: Slot handlers as island plugins fitting well in this new organization? We will move the logic to RealRenderTrait?
mogtofu33’s picture

Status: Reviewed & tested by the community » Needs work

WireframePanelBase was a base because of legacy layers, so yes now it make less sense and is more something for a future wireframe island different than scaffold. But surely as it is a guess, better to merge scaffold, and add a base when really needed.

I guess slotHandlers could fit in RealRenderTrait if the scope is the same and it do nothing to the tree but is it? If it act on all ViewPanelBase probably better to have it's own trait if we have repeating methods? Even more if it is transverse to more islands.

pdureau’s picture

if the scope is the same and it do nothing to the tree but is it?

The scope is the same. So, RealRenderTrait seems the right place for #3594176: Slot handlers as island plugins because it acts only in BuilderPanel and ScaffoldPanel

  • mogtofu33 committed 46d66b25 on 1.0.x
    task: #3615437 Extract ViewPanelBase so Wireframe, Navigator and...
mogtofu33’s picture

Assigned: mogtofu33 » Unassigned
Status: Needs work » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.