[MGNLUI-3837] Action to expand folders Created: 04/Apr/16  Updated: 25/May/16  Resolved: 26/Apr/16

Status: Closed
Project: Magnolia UI
Component/s: None
Affects Version/s: None
Fix Version/s: 5.4.6

Type: Improvement Priority: Neutral
Reporter: Jan Haderka Assignee: Antonín Juran
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: 0d
Time Spent: 35m
Original Estimate: Not Specified

Issue Links:
Relates
relates to MGNLUI-3774 Provide action which delegates to ano... Closed
relates to MGNLUI-3820 Provide task for adding delegate acti... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Date of First Response:
Sprint: Kromeriz 41
Story Points: 3

 Comments   
Comment by Mikaël Geljić [ 12/Apr/16 ]

Maybe a little context, description and use-case could help there
At first, I don't see why you would need such an action, when you can just toggle-expand the currently-selected item (unless you're kicking that from a callback).

Anyway here's my take on the two current proposals:

  • PR #105, hardly ok in my opinion, who knows what other components, presenters or views we'd like to inject into actions; that would make a whole bunch of candidate parameters. In that case, I'd rather be interested in why the executor is not able to inject whatever is in the "current" subapp scope (perhaps the binding for the executor itself being in the wrong container?? I don't know).
  • PR #104, more low-weight I'd say. Though maybe you can also try to "hijack" the following bit in the BrowserPresenter's ContentChangeEvent.Handler:
    if (event.isItemContentChanged() && !existingSelectedItemIds.isEmpty()) {
        workbenchPresenter.expand(existingSelectedItemIds.get(0));
    }
    

    This should have the same effect.

Also worth noting that "making the UI, presenters or views easily react to the outcome of actions" is yet another pain point which has never been fundamentally addressed (besides the super-limited callbacks carrying zero useful information). Nothing we'd like to stick with for too long.

Comment by Antonín Juran [ 13/Apr/16 ]

Reason for implementation of the action (ExpandNodeAction) is request for providing defaultAction by node type (see MGNLUI-3774) and set the defaulAction for folder to expand folder (see MGNLUI-3820).
To PR # 105:
I agree with you that add WorkbenchPresenter as action argument isn't good idea for reasons you described. In this case WorkbenchPresenter injected to ExpandNodeAction is in correct subapp scope but it's another instance than instance in BrowserPresenter. Maybe would be nice to introduce provider class for WorkbenchPresenter as is e.g. ContentConnectorProvider for ContentConnector for getting the same instance.
To PR # 104:
I think your implementation is better in order to don't introduce new unnecessary event and its handling.

Comment by Mikaël Geljić [ 13/Apr/16 ]

Ok thanks for the explanation,

re: PR #105 there should be only one instance of WorkbenchPresenter throughout the whole subapp, maybe we need an extra singleton declaration there?
Anyway we might want to look into that, regardless of what happens here.

re: toggling folders, I'm not sure yet if I'd go with the delegating action *everywhere*:

  • e.g. for some time I thought an easy fix would be to move the defaultAction to individual ActionbarSectionDefinitions, but even then mapping content types would not be perfect
  • I'd rather declare this on a content type level (ideally not just JCR), and give the workbench a better notion of "folders".
    • Then content-views would be capable of handling the toggling at a lower (view) level.
    • Generally we might think about more interactions triggering more different actions too (single-click rename too?)
  • Config is already spoiled everywhere, we should eventually just simplify it

I'll copy my notes above to MGNLUI-3820 for further discussion, obviously.

For this ticket I can totally live with this action, it gives the possibility at least.

Comment by Antonín Juran [ 15/Apr/16 ]

Thanks for your comment,
re: PR # 105 yes, for subapp should have only one WorkbenchPresenter instance. I did quick test - set singleton scope to WokbenchPreseneter for subapp in module descriptor, it worked fine for one subapp but for more subapps (e.g. in Security app) I got another instances of WorkbenchPresenter s in ExpandNodeAction than in BrowserPresenter (it needs deeper investigation why).
Neither of the solutions is ideal, I'd use your implementation of handling ContentChangeEvent.

Comment by Mikaël Geljić [ 15/Apr/16 ]

Comment by Antonín Juran [ 18/Apr/16 ]

Use firing of ContentChangedEvent in ExpandNodeAction and its existing handling in BrowserPresenter.

Generated at Mon Feb 12 09:10:34 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.