Rationale
Stumbled upon this issue when looking at MGNLUI-2101@jira (move of properties) and discovering MGNLUI-2809@jira (moving roles and users work via D&D but not via move action). Realizing this would be a constant source of bugs in custom implementations of apps in the future, I think we need to fix it before there's more custom implementations around. See ARCHI-15 - Getting issue details... STATUS for future improvements.
Problem
- Move has to be implemented twice, once to be invoked by action, once from
DropHandler
. - Apps customising move (e.g. security, pages) had to reimplement both to provide their functionality correctly.
- There is no connection between
Action
andDropHandler
Implementation
- in git: https://git.magnolia-cms.com/gitweb/?p=magnolia_ui.git;a=shortlog;h=refs/heads/move-fix-review
- Impl split in 3 commits:
- expose move functionality by DropHanlder
- https://git.magnolia-cms.com/gitweb/?p=magnolia_ui.git;a=commitdiff;h=9fb85c5045ac91c0eebdca8c8f6168d00a93d65d
- moved various
move*()
methods from Container intoDropHandler
- expose impl. of
DropHandler
in app context
- rewrite actions to use functionality exposed by
DropHandler
- https://git.magnolia-cms.com/gitweb/?p=magnolia_ui.git;a=commitdiff;h=8b556daaa5e8131ed3fb1b1d2cbe1230fb197505
- instead of calling JCR ops directly, delegate execution to currently configured impl of
DropHandler
- impl of above for
security-app
to confirm validity of the impl- https://git.magnolia-cms.com/gitweb/?p=magnolia_ui.git;a=commitdiff;h=c1134cf2f6d6c42fa876d7c19e8bc56d43497d05
- remove custom containers as they are not necessary
- remove custom presenters as they are no longer necessary
- provide
SecurityDropHandler
as only place needed for impl of customisation - configure
SecurityDropHandler
to be used in security app instead of default
- expose move functionality by DropHanlder
The question might pop in "why is action delegating to drop handler and not the other way around?". Answer is simply because of current dependencies between modules. Actions are delivered by content-app
while D&D functionality comes from workbench
and content-app
has dependency on workbench
.
Another question might be why is drop handler configured in app context
rather then in sub app only? Since it's shared by both action (invoked from dialog) and tree (living inside browser sub app) app context
is the closest common context both share.
- There is one additional commit, that is not really related to move, except for solving MGNLUI-2101@jira by allowing "move" for properties
- https://git.magnolia-cms.com/gitweb/?p=magnolia_ui.git;a=commitdiff;h=94abdadbeb34a705ca1626608a961d1bb2e8ad58
- and it goes together with tiny addition (
NodeUtil.moveProperty()
) to core
5 Comments
Magnolia International
I would consider the dependency problem you mention a problem indeed, rather than a reason. Maybe the drop handler should be configured to execute an arbitrary action? (Just like the workbench configures "default" actions for double-click)
Jan Haderka
How about having
MoveHandler
in app context and having both action and drop handler to delegate execution to it?Magnolia International
Yet another indirection ? What would be the benefit ? And why would other interactions not have that as well ? (e.g DeleteHandler etc)
Mikaël Geljić
Wow great stuff, I like it! I actually wasn't aware that we had two different impls for moving items... :S
Just a bit of thoughts about the dependency thing, I'm personally not too fond of using the Vaadin
DropHandler
interface when coming from the move dialog; we had a bit of the same dependency issue for inplace editing.Before 5.3 we used to have JCR logic for editing properties in
BrowserPresenter
(then invoking this from the tree with a chain of listeners + from editProperty dialog). And I wanted inplace-editing (workbench) to perform save through an action (ui-contentapp). As a side note, ideally I would have reused the same action for inplace-editing and the editProperty dialog but couldn't find a clean way back then (and use cases are a bit different too).Anyway, I introduced an
ActionEvent
that takes an action 'name', itemId and arguments to fire from the workbench — which ultimately delegates to theActionExecutor
. The idea was to enable triggering actions from any point in the UI, although I admit that relying on action name is not a first-choice solution either.That's it, just wanted to chime in, in my opinion we don't have a silver-bullet solution for this yet, but on the other hand I could definitely live with what you've done Jan, it's way much better now.
Jan Haderka
Yeah, I was not so thrilled about using vaadin class either, but i figured that it's the one least specific, w/o having to introduce yet another interface for it, but if you all agree this is a good thing to do, then I'm happy to do it. All I want is to make sure this goes into 5.3, but we should not cave on doing it right just to get it in.