-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not prompt for close on import #1462
Conversation
What does the view.CloseOnImport do? Why is the presenter asking the view to trigger the close? This feels weird. Also needs test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test missing and wondering about flow between view and presenter. Going to check the code
@@ -41,10 +41,13 @@ public ModalImporterPresenter(IModalImporterView view, IImporterPresenter import | |||
AddSubPresenters(importerPresenter); | |||
} | |||
|
|||
private bool _shouldPromtBeforeClose = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move the definition of this flag at the top (use resharper) and correct typo (Prompt)
_importerPresenter.OnTriggerImport += (s, d) => | ||
{ | ||
_shouldPromtBeforeClose = false; | ||
_view.CloseOnImport(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh see. This methods already exists and it is called CloseView.
From the name it looked it was triggering some event.. (Onxxx) .Naming is really everything in programming :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"There are only two hard things in Computer Science: cache invalidation and naming things"?
Phil Karlton
😅
@msevestre the test is not valid because the view.Display() method returns immediately. So, this is a blocking scenario. I have some ideas on how to implement this but I wonder if we already have some support for it. We could talk later about it if you want. |
A few remarks: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the test ok now? @abdelr
We still do not have a cancel button, but I am not sure if we want to change this. In any case, if we do, it will be just adding the button which will invoke form close but keep in mind that we already have some buttons changing captions and could be a bit confusing. We have the X button anyways. Behavior seems to work properly now... close -> prompt, import -> close silently |
Fixes #1460 (Do not prompt for close on import)