Fix "Could not add entry for distr" when adding an MRCAPrior - #129
Merged
Conversation
TestFX delivers a click to a point on the screen, so a node outside the screen is never clicked, and the click silently does nothing. New windows are cascaded down the screen as they are opened, and a BEAUti window is nearly as high as the screen, so nodes at the bottom of the window drop off the screen once enough windows have been opened in the same JVM -- which depends on how many tests ran before. That made LinkUnlinkTest.linkClocksDeleteAllTest fail as soon as one more BEAUti test class was added: its window ended up 20 pixels lower, which put the delete button 6 pixels below the bottom of the screen, so the partitions were never deleted. Move the window back onto the screen before clicking a node in it.
Adding an MRCAPrior in the priors panel popped up "Could not add entry for distr": an MRCAPrior starts out without a distribution, but the editor for its distr input assumed there is always one to edit, and threw a NullPointerException, which the input editor factory reports as that message. Switching to another tab and back showed it again, since the panel rebuilds the editors every time it is shown. The editor for a distribution valued input used to be ParametricDistributionInputEditor, which kept the object owning the input in m_beastObject and skipped the expanded initialisation when the input had no value. ScalarDistributionInputEditor puts the distribution being edited in m_beastObject instead, which is null when there is none, and expands MRCAPriors before checking whether there is anything to expand. - only take the MRCAPrior branch when there is a distribution to expand - label the editor with the name of the input when there is no distribution to take the ID from, and add the pane holding it to the editor, so that the combo box to select a distribution is shown at all - collect the templates for that combo box from the object owning the input when there is no distribution to collect them from - tolerate a null distribution in formatName and getParameters Closes #128
Member
|
Thanks for this, it looks good to me |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #128.
The bug
An MRCAPrior starts out without a distribution, but
ScalarDistributionInputEditorassumed there is always one to edit:m_beastObjectis set to the distribution being edited, which is null for a new MRCAPriorexpandedInitbefore checking whether there is anything to expandaddInputLabelthen dereferencesm_beastObject.getID()and throwsInputEditorFactory.addInputscatches that and reports it as "Could not add entry for distr". The priors panel rebuilds its editors every time it is shown, which is why switching tabs brought the message back.Regression point
Introduced by 6202bd0 ("merge BeastFX3 back into beast3 #36"), which switched BEAUti's Add Prior action to the spec
MRCAPrior(whosedistrinput is aScalarDistribution) and addedScalarDistributionInputEditorfor that type. The editor it replaced,ParametricDistributionInputEditor, kept the object owning the input inm_beastObjectand skipped the expanded initialisation when the input had no value, so it did not have this problem. The failing path has been byte-identical since that commit -- the later commits touching the file are moves -- and the reproduction below fails at 4491b15 as well.The fix
formatNameandgetParametersThe prior now shows as
Distr [selector], and picking a distribution works from there as well as from the row combo box.Tests
ScalarDistributionInputEditorTest.testEditorForMRCAPriorWithoutDistribution-- builds the editor the way BEAUti does; without the fix it fails with the reportedNullPointerException.BeautiMRCAPriorTest-- drives BEAUti through the steps from the issue (import an alignment, add an MRCA prior, switch tabs, pick a distribution) and fails on the popup without the fix.The first commit is a separate test harness fix: TestFX clicks a point on the screen, and BEAUti windows are cascaded down as they are opened, so adding another BEAUti test class pushed the delete button in
LinkUnlinkTest.linkClocksDeleteAllTestjust below the bottom of the screen, where the click never reached it. Windows are now moved back onto the screen before clicking.Full
beast-fxsuite: 25 tests, 0 failures.