I39: Add Execute All in judge button to bypass stop on first failure #918
Conversation
|
One general question I have before I actually start testing the PR is: I noticed you duplicated all the " |
|
The "Steps for Testing the PR" shows a sample GUI screenshot, which is apparently supposed to be related somehow to testing the PR. However, as it shows in the GUI header, there is only ONE total test case for the problem. Using a problem with only one test case doesn't really help in explaining how to test the ability to execute multiple test cases after the first one fails... |
| * refreshes the resultspanebuttonpanel | ||
| * includes/removes execute all button based on stop-on-first-failure | ||
| */ | ||
| private void updateResultsPaneButtonPanel() { |
There was a problem hiding this comment.
This method essentially duplicates the contents of method getResultsPaneButtonPanel() (the singleton accessor/constructor for the button panel), then conditionally adds the ExecuteAll button. The problem with doing it this way is that if in the future a change needs to be made to the layout of the button panel (say, another button needs to be added), that change now needs to be made in TWO places. This makes the code very brittle and subject to errors when future changes are made. The code from the accessor/constructor should not be duplicated; rather, the original accessor/constructor should be updated to call a separate method which conditionally adds (or removes) the ExecuteAll button. That separate method can then be called separately when it is necessary to update the button panel, without the need to reconstruct the entire panel separately. (It will probably take some minor rearrangement of how the methods work and when they are called, but doing this will greatly improve the maintainability of the code.)
There was a problem hiding this comment.
I kind of figured out a way to fix this however one small issue that arises is that the Execute All button now comes up in the end after the Close button. So far, I'm unable to figure out how to change the ordering of the buttons. I'm still looking into it. Basically, the issue is that there is a JPanel in FlowLayout with a bunch of components, how to now add some new components in between some existing components.
There was a problem hiding this comment.
One solution, admittedly not necessarily ideal, would be to create two sub-panels in the "Button Panel": one to hold the "permanent" buttons (the checkbox, plus the Select All, Unselect All, Compare Selected, and Execute All buttons), and a second panel to hold (just) the Close button. Then you could add/remove the Execute All button from the "left sub-panel" and the "right sub-panel" containing (just) the Close button would always appear to the right of whatever was in the "left sub-panel". This however would certainly add some additional (perhaps undesirable) complexity to the GUI. Another solution might be to use a GridBagLayout for the button panel -- but if you've never fooled with those, fair warning: they are pretty complicated (but, very powerful for controlling layouts).
A better solution of course would be if there's a way to manage the insertion point of a component in a JPanel with a flow layout -- but I'm not sure that's possible without rebuilding the entire panel (which as I pointed out isn't really desirable -- at least, there shouldn't be two different places where it's done...)
If none of the above helps, my own feeling is that it's better to have proper code organization, at the expense of having the Execute All button show up on the far left (others may feel differently...) Overall, I would investigate the "two sub-panels" idea first; it might be relatively clean and easy to do.
There was a problem hiding this comment.
Updated with the "two sub-panels" approach
There was a problem hiding this comment.
I'm a little late to the party, but I have been doing some testing on this branch.
Instead of a left/right panel, why not just always add the execute all button, but just set its visiblity:
getExecuteAllButton().setVisible(isExecuteAllButtonVisible) ? Basically the button will always "be there", just not visible for the cases we don't want to see it. I suppose you could do the same for the spacing strut too. I quickly tried this and it seems to work. Basically, as a test, I just added:
//add space
getResultsPaneButtonPanelLeft().add(getExecuteAllHorizontalStrut());
// add a control button to execute all test cases
getResultsPaneButtonPanelLeft().add(getExecuteAllButton());
getExecuteAllButton().setVisible(false);
to your "getResultsPaneButtonPanelLeft()" method. And changed updateResultsPaneButtonPanelLeft() to:
private void updateResultsPaneButtonPanelLeft() {
if (currentProblem != null && currentProblem.isStopOnFirstFailedTestCase()) {
isExecuteAllButtonVisible = true;
}
else {
isExecuteAllButtonVisible = false;
}
getExecuteAllButton().setVisible(isExecuteAllButtonVisible);
}
I think this would work as it was before you added the left and right panels. Or, did you try this and find it didn't work? It seems cleaner, assuming of course it works without the left/right panels.
There was a problem hiding this comment.
Updated as per suggestion. Earlier I had tried this approach, must have done something wrong as I was seeing a large space when the Execute All button set to be invisible.
| // Add action listener whether Test results pane button is clicked | ||
| testResultsFrame.addActionListenerToExecuteAll(new ActionListener() { | ||
| @Override | ||
| public void actionPerformed(ActionEvent e) { | ||
| new Thread(new Runnable() { | ||
| public void run() { | ||
| executeRun(true); | ||
| } | ||
| }).start(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This block of code (adding an actionListener() to the ExecuteAll button) is a bit awkward; normally, code like this belongs in the singleton accessor/constructor (in this case, in TestResultsPane.getExecuteAllButton(), right after the executeAllButton = new JButton() statement). As presently coded, this block makes calls through a series of methods in different classes (TestResultsFrame.addActionListenerToExecuteAll() --> TestResultsPane.addActionListenerToExecuteAll() --> TestResultsPane.getExecuteAllButton()). If changes are made later to methods in that sequence, it can result in an error in the calling class (SelectJudgementPaneNew). In other words, there is a lot of tight coupling between these classes, which in general is not good.
I do recognize that moving this block of code into the singleton accessor/constructor would raise the problem of how the ActionListener code is going to be able to execute the statement executeRun(true); since the execute() method is in the SelectJudgementPaneNew class, not in the TestResultsPane class where the ExecuteAll button is defined. I suspect that moving the code to the ExecuteAll constructor would require adding code to allow it to reference back to the SelectJudgementPaneNew.execute() method, so I'm not necessarily requesting a change. It just seems awkward to have this long sequence of addActionListenerToExecuteAll() method calls.
There was a problem hiding this comment.
One workaround for it can be that TestResultsPane object should have an object of SelectJudgementPaneNew as a member variable. When an object of TestResultsPane is created inside SelectJudgementPaneNew we can set the TestResultsPane 's member with this (referring to the SelectJudgementPaneNew calling the function) and use the executeRun function from there. I'm going to explore this approach and might require making the executeRun function public.
There was a problem hiding this comment.
Yes, this is one approach to solve the problem -- instead of having the parent (composing) class (SelectJudgementPaneNew) have references downward to the composed classes (like TestResultsFrame and TestResultsPane), support the ability of the composed class (TestResultsPane) to have a reference upward to its composing class. This would be slightly cleaner, although it still represents tight coupling between the classes.
In any case, if you take that approach I don't think method SelectJudgementPaneNew.executeRun() should be public; it should be protected -- which will allow classes in the same package to access its methods while prohibiting general public access. This will work since both SelectJudgementPaneNew and TestResultsPane are in the same package (edu.csus.ecs.pc2.ui).
| @@ -2319,6 +2394,7 @@ public void setData(Run run, RunFiles runFiles, Problem problem, ProblemDataFile | |||
| this.currentRunFiles = runFiles; | |||
| this.currentProblem = problem; | |||
| this.currentProblemDataFiles = problemDataFiles; | |||
| updateResultsPaneButtonPanel(); | |||
There was a problem hiding this comment.
As noted above, the contents of this method (updateResultsPaneButtonPanel()) should not be reconstructing the entire button panel from scratch. Rather, the method should simply be updating what is (potentially) changing : it should adjust the ExecuteAll button, not reconstruct the entire panel.
There was a problem hiding this comment.
I have built the code and done a run-time test; it appears to work as intended (and is a nice improvement).
I have made a number of comments in the code about things I'd "recommend" being fixed/updated. Most of them are minor and I could live with them as-is. However, there is one change which I think needs to be made prior to approval: see the comment in class TestResultsPane regarding the inappropriate duplication of code in method updateResultsPaneButtonPanel(). That method should not be recreating the entire button panel from scratch (for the reasons I listed in the code-review comment). Rather, it should only be "updating" the button panel by adding/removing the "Execute All" button, as appropriate.
Updated the screenshots |
…removing execute all button
| // Add action listener whether Test results pane button is clicked | ||
| testResultsFrame.addActionListenerToExecuteAllButton(new ActionListener() { | ||
| @Override | ||
| public void actionPerformed(ActionEvent e) { | ||
| new Thread(new Runnable() { | ||
| public void run() { | ||
| executeRun(true); | ||
| } | ||
| }).start(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
I still do not understand why the code to add the actionListener() to the Execute All button is here, in the SelectJudgementPaneNew class. The Execute All button is created as an object when method TestResultsPane.getExecuteAllButton() is invoked. That method is invoked from only three places: methods addActionListenerToExecuteAllButton(), enableExecuteAllButton(), and updateResultsPaneButtonPaneLeft().
The FIRST time one of those methods calls getExecuteAllButton(), the button will be created. The button does not go away (cease to exist) just because method updateResultsPaneButtonPanelLeft() chooses to add it to, or remove it from, the "left panel" -- it remains as a valid object in memory, with its current state intact (including any actionListeners attached to it. If method updateResultsPaneButtonPanelLeft() removes it from the panel, and then subsequently adds it back, it is the exact same button (object) which is being "added back". Therefore, there's no reason to continually "update" the actionListener attached to the button.
The singleton accessor/constructor for the button (method getExecuteAllButton()) should add the actionListener to the button once, when the JButton is constructed. This avoids all the unnecessary code which is constantly updating the button's actionListener. (It does of course require implementing the facility for the TestResultsPane to have an upward access (reference) to the SelectJudgementPaneNew which contains the executeRun() method which the actionListener needs to call. But that seems a small price to pay for eliminating all the "action listener setting", and for being able to keep the entire construction of the JButton in a single place.)
There was a problem hiding this comment.
I have reviewed the updated code and also done a run-time test. The updates fixed the main issue I had requested a change on, and the new code works fine.
I made one comment about the (IMHO) unnecessary updating of the actionListener for the Execute All button; I think it makes more sense to add that actionListener when the button is first constructed (in TestResultsPane.getExecuteAllButton()). See my comments in the "conversation" for details.
I'm prepared to approve this PR if you will either fix the issue with the actionListener, or provide a description of why the changes required to fix it (that is, the code to give the TestResultsPane access to the parent SelectJudgementPaneNew object so the actionListener can be added in the singleton constructor/accessor method) are more complicated than what would be gained by removing the complicated update action listener methods.
There was a problem hiding this comment.
On further consideration, I am changing my mind regarding my previous review comments. While I don't like the fact that the actionListener for the Execute All button is being set from a composing class (specifically, SelectJudgementPaneNew) rather than being set in the singleton accessor/constructor for the button, there are other considerations.
Specifically, changing it so the ActionListener is set in the constructor method would require adding the same type of code to TestResultsPane that already exists in SelectJudgementPaneNew and TestResultsPane -- code to access UP the hierarchy of the composition (instead of DOWN the hierarchy as it presently does). So making the change is probably a net-zero gain.
In addition, this hierarchy is a classic example of a composition. In general, the design rules for a composition include that the parent controls the lifetime of its children. This can be extended to say that the parent "knows about" its children, but that the children in general do not know about their parents. While there are valid reasons for making exceptions to this rule, there doesn't seem to be one here; and as coded, the parents (SelectJudgementPaneNew and TestResultsFrame) do know about the "child" (TestResultsPane and its subcomponents). So I think the current structure is fine (I wish I could come up with a better one, but I can't...)
So I'm adding an Approval to this PR.
There was a problem hiding this comment.
Looks good and seems to work as advertised. The "how to test" instructions were a bit misleading since they imply there will be more than one problem to select from when turning off computer judging. When I loaded 'mini' there was exactly one problem, sumit. No big deal, just pointing it out.
I would like a to see a response to the comment I made about left/right panels and the setVisible() business I suggested before I approve this.
There was a problem hiding this comment.
I reviewed the updated code, then built and ran the system. Works great. @johnbrvc 's solution (toggling visibility instead of using two sub-panels) was a better suggestion than mine; I'm glad you went ahead and implemented it.
Marking PR as "Approved".
Description of what the PR does
Add
Execute Allbutton onTestResultsPane. It can bypass stop on first failure condition and run the code on all testcases if a judge desires.Issue which the PR addresses
Fixes #39
Environment in which the PR was developed (OS, IDE, Java version, etc.)
Windows 10, Eclipse 2021-12 R, JDK 8u381 (1.8), C++ 13.2.0
Precise steps for testing the PR (i.e., how to demonstrate that it works correctly)
Problemspane make sure one of the problems have judging type set to Manual, if not edit and set to Manual. Also check the box for stop-on-first-failure.Auto Judgepane make sure one of the judges have Problems(none selected), if not edit and set.New Runspane and request run.Select Judgement for Run {:id}window click on Execute Run.Execute Allbutton in Test Results Pane. It should now execute all test cases despite failures.