Move Hive Progress Control to MainForm.WindowsForms

[HeuristicLab-Trac-Bot]

Issue migrated from trac ticket # 1762

milestone: HeuristicLab 3.3.7 | component: MainForm.WindowsForms | priority: medium | resolution: done

2012-01-17 13:00:24: @Shabbafru created the issue

---

Move the Hive Progress Control from Clients.Hive.Views.Progress to MainForm.WindowsForms.Controls. Also use it for the side bar and the samples panel.

[HeuristicLab-Trac-Bot]

2012-01-17 13:00:57: @Shabbafru edited the issue description

[HeuristicLab-Trac-Bot]

2012-03-07 19:14:26: @Shabbafru commented

---

r7582 moved ProgressView from Hive to MainForms

[HeuristicLab-Trac-Bot]

2012-03-07 19:27:18: @Shabbafru

[HeuristicLab-Trac-Bot]

2012-03-07 19:27:18: @Shabbafru commented

---

r7583 added missing event deregistration

[HeuristicLab-Trac-Bot]

2012-03-07 19:27:37: @Shabbafru changed status from new to accepted

[HeuristicLab-Trac-Bot]

2012-03-08 09:46:17: @Shabbafru

[HeuristicLab-Trac-Bot]

2012-03-08 09:46:17: @Shabbafru commented

---

r7585 adapted MetaOpt to changes in trunk

[HeuristicLab-Trac-Bot]

2012-06-05 10:05:21: @mkommend changed milestone from HeuristicLab 3.3.x Backlog to HeuristicLab 3.3.7

[HeuristicLab-Trac-Bot]

2012-06-20 16:10:55: @Shabbafru commented

---

Hi Andreas, maybe you can have a look at this. I would be especially interested if the way this control is used makes sense.

[HeuristicLab-Trac-Bot]

2012-06-20 16:10:55: @Shabbafru changed owner from @Shabbafru to @abeham

[HeuristicLab-Trac-Bot]

2012-06-20 16:10:55: @Shabbafru changed status from accepted to reviewing

[HeuristicLab-Trac-Bot]

2012-06-26 00:27:53: @abeham commented

---

ProgressView.cs:

• The assignment of null to the Progress property will cause a NullReferenceException RegisterProgressEvents
• Don't use "this." to reference to instance variables in general
• The cancel button doesn't call OnCanceled... I don't understand this. Why does the view raise the Canceled event instead of the Progress object? I wouldn't put the event in the control, but rather put a CancelRequested property in the IProgress object and then have the control listen to a Canceled event of the progress and only then close the window. There's no reason to close this view when the operation has not been successfully cancelled.
• I don't like the self disposing in Finish(), somebody created this object and that instance is responsible for destroying it.
• You could also add a Busy property to IProgress and a BusyChanged event instead of Finished. That way you know whether a progress has started, is in progress, has finished, or is not in progress. You can then have the control just attached to a parent view and never care for it again. You just give it a new progress object whenever you have one.
• You shouldn't need to set all control's enabled property to false. Setting the parent view to Locked # true; ReadOnlytrue; should set the enabled state of the parent's children. Why does it not work like this?
• Decide if you throw an ArgumentNullException in case parentView is null or handle that case.

IProgressReporter.cs:

• Needs only to return a progress object, the rest can be moved to the progress object itself. It's a bit strange to have part of the feedback coming directly from the ProgressReporter and other part through the IProgress object. Why not put all in one place?

I think you should implement the Cancel stuff thoroughly and also support that in Hive. I like to be able to Cancel things that are running for very long.

I improved the code a little in r8111.

[HeuristicLab-Trac-Bot]

2012-06-26 00:27:53: @abeham changed owner from @abeham to @Shabbafru

[HeuristicLab-Trac-Bot]

2012-06-26 00:27:53: @abeham changed status from reviewing to assigned

[HeuristicLab-Trac-Bot]

2012-06-27 16:58:45: @Shabbafru commented

---

r8135 fixed activation of controls after displaying the progress is finished

[HeuristicLab-Trac-Bot]

2012-06-28 12:01:04: @Shabbafru commented

---

r8145 implemented review comments:

• removed self disposing. The progress view now reacts if a progress is set and Finish() is now called on the progress object and not the view.
• Moved Cancel event from ProgressView to Progress
• throw ArgumentNullException if the parent view is null

[HeuristicLab-Trac-Bot]

2012-06-28 12:18:01: @abeham commented

---

Hmm looking over this again, I think we should not let the progress control do the parent control disabling/enabling. The parent control should do this by itself. What do you think?

[HeuristicLab-Trac-Bot]

2012-06-28 12:21:16: @Shabbafru commented

---

r8146 fixed unit test

[HeuristicLab-Trac-Bot]

2012-06-28 12:55:10: @Shabbafru commented

---

Replying to [comment:11 abeham]:

Hmm looking over this again, I think we should not let the progress control do the parent control disabling/enabling. The parent control should do this by itself. What do you think?

I thought that this is a very cool feature because the parent view doesn't need to know anything about the progress view and therefore we don't have to change the code of the parent views. Why do you think the parent control should do the disabling of the controls?

[HeuristicLab-Trac-Bot]

2012-06-28 14:27:56: @abeham commented

---

Yes it's a convenience, but then the progress control just assumes a lot of things of the parent, foremost that it should actually be disabled, that every control should be disabled and that every control should then be enabled again. Won't it also enable controls afterwards that shouldn't be enabled due to the state of the parent?

I mean this

foreach (Control c in this.parentView.Controls) 
  c.Enabled = false; 

is not really nice code. At least you would have to set the parentView's Locked and ReadOnly property so that proper locking is in place as defined by the parentView. Currently, this is just "disable everything I don't care" code. And this code

foreach (Control c in this.parentView.Controls) 
  c.Enabled = true; 

just enables everything, even stuff that should probably be disabled.

[HeuristicLab-Trac-Bot]

2012-06-28 18:49:36: @Shabbafru commented

---

r8156 Removed IProgressReporter and changed the Hive Job Manager accordingly. I think that IProgressReporter is just a result of the strange way how RefreshableJob, HiveClient and the RefreshableHiveJobView work together. It's not used in any other view so I removed it.

[HeuristicLab-Trac-Bot]

2012-06-29 10:31:31: @Shabbafru commented

---

Replying to [comment:14 abeham]:

Yes it's a convenience, but then the progress control just assumes a lot of things of the parent, foremost that it should actually be disabled, that every control should be disabled and that every control should then be enabled again. Won't it also enable controls afterwards that shouldn't be enabled due to the state of the parent?

I mean this

foreach (Control c in this.parentView.Controls) 
  c.Enabled = false; 

is not really nice code. At least you would have to set the parentView's Locked and ReadOnly property so that proper locking is in place as defined by the parentView. Currently, this is just "disable everything I don't care" code. And this code

foreach (Control c in this.parentView.Controls) 
  c.Enabled = true; 

just enables everything, even stuff that should probably be disabled.

Ok, now i see your point, you are right. The problem is that the progress view can't call SetEnabledStateOfControls on the parent. That's the reason why all users of the progress view call it themselves after the progress Finish call. I will remove the disabling of all controls. Should i also remove the setting of Locked and ReadOnly from the progress view? I'm not sure if there is a use case where the controls of the parent view should be enabled, especially because the progress view probably hides some controls of the parent anyway.

[HeuristicLab-Trac-Bot]

2012-06-29 11:27:15: @Shabbafru

[HeuristicLab-Trac-Bot]

2012-06-29 11:27:15: @Shabbafru commented

---

Replying to [comment:16 ascheibe]:

Replying to [comment:14 abeham]:

Yes it's a convenience, but then the progress control just assumes a lot of things of the parent, foremost that it should actually be disabled, that every control should be disabled and that every control should then be enabled again. Won't it also enable controls afterwards that shouldn't be enabled due to the state of the parent?

I mean this

foreach (Control c in this.parentView.Controls) 
  c.Enabled = false; 

is not really nice code. At least you would have to set the parentView's Locked and ReadOnly property so that proper locking is in place as defined by the parentView. Currently, this is just "disable everything I don't care" code. And this code

foreach (Control c in this.parentView.Controls) 
  c.Enabled = true; 

just enables everything, even stuff that should probably be disabled.

Ok, now i see your point, you are right. The problem is that the progress view can't call SetEnabledStateOfControls on the parent. That's the reason why all users of the progress view call it themselves after the progress Finish call. I will remove the disabling of all controls. Should i also remove the setting of Locked and ReadOnly from the progress view? I'm not sure if there is a use case where the controls of the parent view should be enabled, especially because the progress view probably hides some controls of the parent anyway.

Ah, i just saw that SetEnabledStateOfControls gets called when ReadOnly/Locked is set...

[HeuristicLab-Trac-Bot]

2012-06-29 12:58:58: @Shabbafru commented

---

r8159 removed blind disabling of controls

[HeuristicLab-Trac-Bot]

2012-06-29 13:07:25: @Shabbafru commented

---

r8160 removed unnecessary SetEnabledStateOfControls calls

[HeuristicLab-Trac-Bot]

2012-06-29 13:10:44: @Shabbafru commented

---

So if the parent view now wants to lock if the progress view is shown, it has to implement this in the SetEnabledStateOfControls like the RefreshableHiveJobView does it now.

[HeuristicLab-Trac-Bot]

2012-06-29 13:10:44: @Shabbafru changed owner from @Shabbafru to @abeham

[HeuristicLab-Trac-Bot]

2012-06-29 13:10:44: @Shabbafru changed status from assigned to reviewing

[HeuristicLab-Trac-Bot]

2012-06-29 23:13:06: @abeham commented

---

There were some points that weren't very appealing. I think some things were more complicated now and not intuitve. So I rewrote some parts of this feature in r8165 and hope to have improved it:

• I have no intuitive explanation for what void SignalSuccesfulCancelation() should do. I didn't find any references to it, so I removed it.
• Reviewing the cancellation options, I rethought abuot the CancelRequestedproperty and think that in the end it's not the right way to handle this. I introduced a Cancel(int timeout) function instead which raises a CancelRequested event to which there must be at least one subscriber (an exception is thrown in case the event handler is null). Maybe the timeout is overengineering this, I'm not sure.
• We shouldn't have setters in IProgress, but rather in the concrete object. Only the instance that holds the concrete object should be allowed to state updates and progress.
• The ProgressView now is a ContentView and the default view for IProgress
• I changed the whole cancel/finish thing by introducing a state enum and a state changed event.
• The ProgressView reacts to changes in the Progress object
• Cancellation works such that there is a property CanBeCanceled which determines if the operation is cancelable. If the process can be canceled, there must be at least one event handler for the CancelRequested event. If there are no event handlers a NotSupportedException is thrown. If the Cancel() function is called on the Progress, but CanBeCanceled is false nothing happens (I was throwing a NotSupportedException already, but then thought it's better to silently handle this case). The CanBeCanceled state might change during the life of a Progress object.
• I also added some API doc comments to the IProgress interface
• I noticed a problem with defining a progressView local variable in some views: The progessView will not be disposed! Since the progressView is not part of the Controls collection when it's not visible, the Dispose() method of the progressView will never be called. So I changed some code to dynamically create ProgressView instances (using (var view = new ProgressView()) { ... }). In the other views, where the lifetime of the progressView spans over several method calls, I added some code to make sure the controls are disposed correctly. I used the (De)RegisterContentEvents methods for this. This is not optimal, but I don't know of a better way for now.
  • We should think if it maybe would be better not to add/remove from the parent controls, but I can understand this is more convenient than going to the annoying forms designer which sometimes doesn't display all the controls from the solution.
• I updated all views that made use of the IProgress, Progress, and ProgressView
• Last but not least I removed the requirement for a ParentView and (hopefully) handled the case where no parent view is defined. I wanted this because, if you want to use this in a dialog for example there is no (mainform-) view, but just a Form or a UserControl.
• I also changed alignment to Anchor.None so that the control always remains in the middle of the view when it is resized. Otherwise it would sit in the same position.

Please take a look at the changes. I would also beg you to test if I broke anything. I tried it with a RefreshableHiveJobView where it seemed to work.

[HeuristicLab-Trac-Bot]

2012-06-29 23:13:06: @abeham changed owner from @abeham to @Shabbafru

[HeuristicLab-Trac-Bot]

2012-06-30 01:03:43: @abeham commented

---

r8166: Fixed error detected in unit test

[HeuristicLab-Trac-Bot]

2012-07-02 10:38:24: @Shabbafru commented

---

Thanks for improving this. I have tested all views which use the progress view and found no problems. Looks good to me.

[HeuristicLab-Trac-Bot]

2012-07-02 10:38:24: @Shabbafru changed owner from @Shabbafru to @abeham

[HeuristicLab-Trac-Bot]

2012-07-02 10:38:24: @Shabbafru changed status from reviewing to readytorelease

[HeuristicLab-Trac-Bot]

2012-07-02 10:49:41: @Shabbafru commented

---

I have forgotten to test the OptimizerHiveTaskView. If you click e.g. on the stop button to stop an Hive task the following exception is thrown: ArgumentException: Controls created on one thread cannot be parented to a control on a different thread.

[HeuristicLab-Trac-Bot]

2012-07-02 10:49:41: @Shabbafru changed status from readytorelease to assigned

[HeuristicLab-Trac-Bot]

2012-07-02 11:47:02: @abeham commented

---

Maybe this happens in those cases where the control is created in a using construct to surround the body of a method. It could be that there is an InvokeRequired check missing to delegate the creation to the UI thread.

[HeuristicLab-Trac-Bot]

2012-07-02 16:41:02: @abeham commented

---

r8181: Fixed progressView creation. I have not tested it, but I think this was the cause. I also found a similar situation in the OKBUploadExperimentView.

[HeuristicLab-Trac-Bot]

2012-07-02 16:41:02: @abeham changed owner from @abeham to @Shabbafru

[HeuristicLab-Trac-Bot]

2012-07-02 16:41:02: @abeham changed status from assigned to reviewing

[HeuristicLab-Trac-Bot]

2012-07-02 23:14:24: @Shabbafru commented

---

I have tested the changes and fixed the activation/deactivation of the progress view in r8186. Thanks a lot for your input and help with this ticket!

[HeuristicLab-Trac-Bot]

2012-07-02 23:14:24: @Shabbafru changed owner from @Shabbafru to @abeham

[HeuristicLab-Trac-Bot]

2012-07-02 23:14:24: @Shabbafru changed status from reviewing to readytorelease

[HeuristicLab-Trac-Bot]

2012-07-02 23:32:30: @abeham commented

---

One last change

r8187: Moved the ProgressView to the folder Controls instead of Views.

I had the impression that it fits better in the folder where the ViewHost and other views/controls reside. In the Views folder there are only the View, ContentView, and AsynchronousContentView bases classes.

Thx for fixing this!

[HeuristicLab-Trac-Bot]

2012-07-08 04:35:58: @mkommend removed resolution

[HeuristicLab-Trac-Bot]

2012-07-08 04:35:58: @mkommend changed status from readytorelease to closed