I'm not super familiar with how this code is supposed to work — I follow the basic logic, but whether you're supposed to be able to chop & change backends at will I'm not sure — so I may be misunderstanding what's going on here, but I'm a bit worried about some of your changes. I've added some comments to your PR in afw to explain my concerns. Can you set my mind at rest?

Apart from that, I'd really like to see some tests along with this code. In particular, whether or not the behaviour in my example on the PR is expected is something that would be easy to establish in the test suite so readers aren't left wondering. Would you be willing to add some? I'm receptive to push back because the code you're modifying isn't well tested so establishing "correct" behaviour may be hard, but adding one layer of uncertainty on top of another isn't helping!

John Swinbank Thank you very much for your detailed review!

• The Display initializer will now clear the optional arguments when a backend is specified.
• getDefaultBackend will optionally return the optional arguments
• Two tests of these functionalities have been added to the afw testDisplay.py suite

Would you please review the revisions?

Reviewing the developer guide, I think I have erred by squashing all my commits after the revision. Perhaps it would have been best to have three logical commits, for the three bullet points in my previous comment.

Hi David Shupe —

Sorry it's taken me so long to get around to looking at this.

Basically, the changes you've made seem ok, and I'm happy that you addressed the points in my original review. However, I have added some more comments to the PR. To summarize:

• I'd like you to take another look at the test suite, because I think that the new tests you've added will basically never do anything useful (they take a short-cut if the backend is virtualDevice, and it looks like it always is);
• I'm still not really happy with the interface. I've written a long comment about that on the PR, so I won't rehash it here, but, in short, the fact that you're carrying about a string naming the backend and, separately, a set of arguments associated with that backend seems fragile and makes the code harder to follow than it need be.

I don't have an unambiguously correct way to address the second point, but perhaps you could give it a few minutes thought? It may be that you'll disagree with me: if so, do say so & explain why I'm wrong — that's just as good a response as reworking the code.

Thank you for bringing up some excellent points about the brittleness of the implemented solution.

After talking with Xiuqin Wu [X], I propose to not merge this implementation, and to capture the issues that have been unearthed in a separate ticket for a possible future redesign of the afwDisplay interface. We already have a method of defining displays for Firefly that allow the optional arguments to be passed in. The proposal is to document that method only, and to not document use of setDefaultBackend as an alternative. Would that be acceptable?

Would that be acceptable?

Works for me!

(I'll set this ticket to "Reviewed" to indicate that I'm not intending to do anything more with it, but, given your suggested approach above, I guess you might want to close it as "Won't Fix". Your — or Xiuqin Wu [X]'s — call.)

We will document the use of the afwDisplay.Display constructor, and will not fix afwDisplay.setDefaultBackend to use optional arguments.