DefaultDisplayService: Choose matching display with highest priority #368
Conversation
|
Mh, my test it not great because all tests now have this dummy class |
|
@frauzufall wrote:
I had a similar issue in scijava/scijava-table#8 where I added a |
| final String name = "Text"; | ||
| final String value = "Hello"; | ||
| Display<?> display = displayService.createDisplay(name, value); | ||
| assertEquals(CustomTextDisplay.class, display.getClass()); |
There was a problem hiding this comment.
In addition to asserting that CustomTextDisplay gets chosen, can we also test that we actually do get multiple candidate Displays that are able to handle Strings? Otherwise the name of this method would be misleading, no?
I recently made it easier to create Context ctx = new Context(...); // whatever services you want
PluginInfo<Display> info = PluginInfo.create(CustomTextDisplay.class, Display.class);
info.setPriority(Priority.HIGH);
ctx.service(PluginService.class).addPlugin(info);
... now do the actual test ...And then do not annotate Typically the context creation would be in the |
…oose first by priority #48 * before, the first display which was able to display an object was chosen * now, all matching displays are collected and the one with the highest priority is used
…h the highest priority
frauzufall
force-pushed
the
display-priority
branch
from
fd0095e to
4e49725
Compare
ctrueden
left a comment
ctrueden
left a comment
There was a problem hiding this comment.
Looks good! I think the display plugin selection code can be simplified, though—can you check?
| final List<Display<?>> matchingDisplays = getMatchingDisplays(o); | ||
| if(matchingDisplays.size() > 0) { | ||
| // use the display with the highest priority | ||
| Display<?> display = matchingDisplays.stream().max(Comparator.comparing(Prioritized::getPriority)).get(); |
There was a problem hiding this comment.
I don't think you need this. The displays should already always be returned from the PluginService in high-to-low priority. Did you observe otherwise?
|
Hi @ctrueden, you are right, the PluginService returns the displays in the correct order, which means this PR is not needed at all. I did not properly check if the test I added was actually failing before (reminded me of our discussion about whether the PR related test should be the first commit or the last one 🙃 ).. I also cannot reproduce my use case where it failed. Probably put the priority on the wrong class.... Next time I start with an issue. Sorry for the trouble! I still learned something from your comments 👍 |
|
We could still add the test for multiple displays to test if the highest priority wins. I think the current |
I ran into issue #48 and though this PR does not give the user a chance to choose which display to use, it at least makes the
DefaultDisplayServicechoose the one with the highest priority.I also added a test, but could not yet test it myself, because I get a bunch of unrelated errors with this repo which I have to investigate. But if anyone already has an opinion on this in the meantime or can try it out, let me know.