[MGNLUI-3416] EditorCallback#onCancel not called upon closing a dialog with x button instead of the cancel action Created: 11/May/15  Updated: 21/Aug/15  Resolved: 19/Aug/15

Status: Closed
Project: Magnolia UI
Component/s: dialogs, forms
Affects Version/s: 5.3.8
Fix Version/s: 5.3.11, 5.4.2

Type: Bug Priority: Major
Reporter: Zdenek Skodik Assignee: Aleksandr Pchelintcev
Resolution: Fixed Votes: 0
Labels: support
Remaining Estimate: 0d
Time Spent: 2d
Original Estimate: 0.5d

Issue Links:
Relates
relates to MGNLUI-1372 Dialog does not give event when closi... 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)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:
Sprint: Sprint 3 (Vietnam), Sprint 6 (Basel)
Story Points: 8

 Description   

Except of choose dialogs that are working fine since MGNLUI-1372.



 Comments   
Comment by Aleksandr Pchelintcev [ 28/May/15 ]
  • Considering a choose dialog fix in MGNLUI-1372 we now have three implementations of the same pattern => refactoring is needed.
    • At least this isExecutingAction field can go to the BaseDialogPresenter and be handled there for all the
    • Also what would happen if action would fail during execution? Looks like isExecutingAction would stay set to true => maybe makes sense to wrap action execution in try/finally?
    • I am 100% not sure, but maybe even that CloseHandler could be handled in the base presenter simply calling some abstract method which the child presenters would implement and invoke corresponding callbacks.
Comment by Evzen Fochr [ 28/May/15 ]

I am afraid that i cannot handle it in base presenter. In FormDialogPresenterImpl is not called super.start and there are 3 different callback interfaces (MoveActionCallback, EditorCallback, ChooseDialogCallback) with different functions. (or do not have any idea how to do it)

Comment by Aleksandr Pchelintcev [ 28/May/15 ]
  • isExecutingAction is now moved to the base presenter, but it is still handled separately in all three places, why?
  • In fact FormDialogPresenterImpl calls super.start(..) method - there's just no "super" word.
  • About three callback interfaces - I written about that in my previous comment:
    • Yes, the BaseDialogPresenter does not know about the callback type (unfortunately)
    • But could it at least set up the CloseHandler and invoke some abstract method that the extending presenters would override and invoke the callback that is specific to them?
Comment by Aleksandr Pchelintcev [ 29/May/15 ]

This is not exactly what I was trying to say in the previous comments:

  • I didn't mean that every presenter has to have a method that would create a CloseHandler
  • What I meant was that the CloseHandler is created in BaseDialogPresenter only(!!!) and invokes some abstract method in case the action is not executed.
  • What it gives:
    • You can make the isExecutingAction field private (it should private anyway, in worst case - create a getter!)
    • In extending presenters you would only write what to do in case the dialog is closing not from action (i.e. invoking a callback)

also:

  • getNewDialogCloseHandler is a quite bad name for a method.
  • with your solution if I created a new dialog presenter and wouldn't override this method - I'd get an NPE.

finally:

  • Test case is needed.
Comment by Aleksandr Pchelintcev [ 02/Jun/15 ]

Looks much better now!

  • The only thing that I would improve is I would move the bunch of newly introduced class fields in the tests into the setUp() method (making them local variables) so that the classes are not cluttered.
  • Also I'd maybe investigate whether mocking of i18nizer cannot be done simpler now.
Comment by Aleksandr Pchelintcev [ 05/Jun/15 ]

The solution causes stack overflow for the case of form dialogs.

Comment by Mikaël Geljić [ 05/Jun/15 ]

FYI also mind a potential Internal Error e.g. when opening Change template light-dialog in pages app, and hitting ESC.

Comment by Evzen Fochr [ 15/Jun/15 ]

In some callbacks is called closeDialog or close view functions, which is causing recursion.

Comment by Evzen Fochr [ 02/Jul/15 ]

After removing closeDialog() from callbacks, dialogs aren close after commit or cancel action.

Comment by Trang Truong [ 30/Jul/15 ]

It's fixed on branch MGNLUI-3416-cancel.

Comment by Aleksandr Pchelintcev [ 12/Aug/15 ]

trang.truong the fix provided indeed breaks the infinite EditorCallback <-> DialogPresenter's close handler infinite loop by removing the close handlers before invoking them. However, if you consider the situation when there are several close handlers attached to a dialog, then a callback number i where i is > 0 will be called at least 2 times.

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