-
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
Should be possible to switch between different parameter identification feedback views #1403
Should be possible to switch between different parameter identification feedback views #1403
Conversation
IParameterIdentificationFeedbackPresenter FeedbackPresenterFor(ParameterIdentification parameterIdentification); | ||
} | ||
|
||
public class MultipleParameterIdentificationFeedbackPresentersManager : IMultipleParameterIdentificationFeedbackPresentersManager |
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.
Why is it called MultipleParameterIdentificationFeedbackPresentersManager
and not ParameterIdentificationFeedbackPresentersManager
|
||
public class MultipleParameterIdentificationFeedbackPresentersManager : IMultipleParameterIdentificationFeedbackPresentersManager | ||
{ | ||
static private IContainer _container; |
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.
Why is this static? remove
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.
and use resharped to create it as readonly etc..
public class MultipleParameterIdentificationFeedbackPresentersManager : IMultipleParameterIdentificationFeedbackPresentersManager | ||
{ | ||
static private IContainer _container; | ||
private Cache<ParameterIdentification, IParameterIdentificationFeedbackPresenter> _currentParameterIdentifications = new Cache<ParameterIdentification, IParameterIdentificationFeedbackPresenter>(); |
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.
this should also be readonly. Please run resharper to do this stuff for yoy automatically
|
||
public IParameterIdentificationFeedbackPresenter FeedbackPresenterFor(ParameterIdentification parameterIdentification) | ||
{ | ||
if (_currentParameterIdentifications.Contains(parameterIdentification)) |
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.
I fear that this might be called from multiple threads no? Should it be thread safe?
IListener<ParameterIdentificationIntermediateResultsUpdatedEvent>, | ||
IListener<ProjectClosedEvent> | ||
{ | ||
bool ShouldRefreshFeedback { get; set; } | ||
void OnParameterIdentificationStarted(ParameterIdentification parameterIdentification); |
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.
OnXxx looks like an event handler but it not. Why not just call it ParmaeterIdentificationStarted?
@@ -142,6 +130,7 @@ public void Handle(ProjectClosedEvent eventToHandle) | |||
clearFeedbackReferences(); | |||
resetFeedback(); | |||
_view.NoFeedbackAvailable(); | |||
_view.Hide(); |
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.
Why do you had the view? the user won't see that the PI is finished or even see the optimal results etc... or am i missing something?
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.
If you close the project, should we keep the view open? To me, closing the project means you are finished with everything on it, including checking if the PI has finished or not. Actually, the references will be cleared meaning you should not be able to keep the view opened, right?
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.
yeah ok. makes sense. What was the current behaviour? It would just stay open?
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.
Looks good. A few questions and reshaper should be run to format the file properly
I have just fixed the problem with resharper so I hope there will not be any further problem with it anymore ;) |
{ | ||
lock (_locker) | ||
{ | ||
_currentParameterIdentifications.Clear(); |
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.
You are holding a reference all the IParameterIdentificationFeedbackPresenter
s here.
They need to be ReleaseFrom
explicitly as they are registered in the EventPublisher on construction. This is a memory leak
so something like
_currentParameterIdentifications.Each(x=>x.ReleaseFrom(_eventPublisher));
where _eventPublisher is injected into the constructor of the Manager
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.
I am wondering if ApplicationController is not doing the work already?
public virtual ISingleStartPresenter Open<TSubject>(TSubject subject, ICommandCollector commandCollector)
{
var presenterToOpen = PresenterFor(subject);
if (presenterToOpen == null)
{
presenterToOpen = CreatePresenterForSubject(subject);
presenterToOpen.Closing += removePresenter;
presenterToOpen.InitializeWith(commandCollector);
_allOpenedPresenters.Add(presenterToOpen);
}
return presenterToOpen;
}
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.
You are not resolving the presenter usting the applicationController but you are using the container.
But this is in fact a good point. I am wondering if we need this ParameterIdentifiactionManager at all since we already have everything in the applicationController. Why aren't you using this?
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.
Yeah the ApplictionController does exactly what you need I feel for almost everything. Let's talk about it during the call later today
{ | ||
_feedbackPresenter = feedbackPresenter; | ||
_multipleParameterIdentificationFeedbackPresentersManager = multipleParameterIdentificationFeedbackPresentersManager; |
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.
rename this variable to match type
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 is a leak that I missed on first pass
The build is failing for some reasons... |
@msevestre There was already a ISingleStartPresenter registered to edit a ParameterIdentification so I needed to create a wrapper (ParameterIdentificationFeedback) for the ParameterIdentificationFeedbackView/Presenter. Additionally I needed a manager which will always return the same object for the same ParameterIdentification and add some "registrations". I actually do not like fully the code in MoBiApplicationController with the long if else for resolving a presenter for a subject. I would prefer to use the self aware pattern we discussed recently (we ask the presenters if they know how to handle the subject instead of having a long list with all the bindings, but I do not want to draw my attention now on this direction. Maybe for the future? In any case, I am still solving some unforeseen issues. I will let you know when this is finished. |
public void Handle(ParameterIdentificationStartedEvent eventToHandle) | ||
{ | ||
var parameterIdentification = eventToHandle.ParameterIdentification; | ||
lock (_locker) |
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.
Call GetFeedback method here (or to follow nomenclature rename it to FeedbackFor) instead of duplicating the code.
|
||
public void Edit(ParameterIdentificationFeedback objectToEdit) | ||
{ | ||
_key = objectToEdit; |
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.
I don't understand why you need this. Can't you save the _parameterIndentification as member?
{ | ||
_key = objectToEdit; | ||
if (objectToEdit.Running) | ||
Handle(new ParameterIdentificationStartedEvent(objectToEdit.ParameterIdentification)); |
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.
This is also very weird. THis is not how the publisher works. Want to have a call?
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.
I don't understand why you need this new Object structure. Let's have a chat
I will call you in about 20 minutes. |
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.
You mentioned a leak when clsoing. It is still missing right?
…en cancelled before opening the presenter
setParameterIdentificationToStarted(objectToEdit.ParameterIdentification); | ||
break; | ||
case RunStatusId.Canceled: | ||
setParameterIdentificationToStarted(objectToEdit.ParameterIdentification); |
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.
This is weird. Why start and then cancel. I am sure there is a reason but I would comment this in the code
_parameterIdentification = eventToHandle.ParameterIdentification; | ||
_view.Caption = Captions.ParameterIdentification.FeedbackViewFor(_parameterIdentification.Name); | ||
showParameterIdentificationFeedback(); | ||
if (eventToHandle.ParameterIdentification != _parameterIdentificationFeedback.ParameterIdentification) |
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.
Much better. You even have a _parameterIdentification member so this could be
if (!Equals(_parameterIdentification , eventToHandle.ParameterIdentification))
return;
Same later. on public void Handle(ParameterIdentificationTerminatedEvent eventToHandle)
//Nothing to do here | ||
} | ||
|
||
public void Edit(ParameterIdentificationFeedback objectToEdit) |
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.
rename objectToEdit to parameterIdentificationFeedback
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.
Minor changes. Looks very good. The onky really required is the comment to explain why you call the start on cancel. The rest is more for consistency purposes. Fix what you see fit and please merge
// event is triggered after a started event was triggered and the feedback view | ||
// should evolve through both of them. If terminated is invoked directly, then the | ||
// view would should its initial state asking the user to start the parameter identification | ||
// instead of showing the terminated parameter identification and its last state. |
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.
yes!!
The build if failing @abdelr Can you check? |
Fixes #1372 (Should be possible to switch between different parameter identification feedback views )