Opened 6 years ago

Closed 5 years ago

#2600 closed defect (fixed)

Problem with "toolshed show"

Reported by: pett Owned by: Greg Couch
Priority: moderate Milestone: 1.0
Component: Tool Shed Version: 1.0
Keywords: Cc: chimera-staff@…
Blocked By: Blocking:
Notify when closed: Platform: all
Project: ChimeraX

Description

The first thing "toolshed show" does is to look for existing matching instances of the tool name to call display() on. One of the steps of that search is to match the tool name against the bundle name. This is problematic if a bundle registers multiple tools, one of which happens to match the bundle name.

For instance, my "Clashes" bundle register two tools: Clashes and Contacts. The bundle-name-matching code means that I can start "Clashes" and then "Contacts" okay, but not "Contacts" and then "Clashes" because when trying to start "Clashes" that code sees that the bundle name for "Contacts" is "Clashes" and just displays "Contacts" again instead of starting "Clashes".

Perhaps that code should be nuked entirely, but for now I have just commented it out pending your review.

Greg also points out that the tool-name expansion that ts.find_bundle_for_tool() performs probably should occur earlier, before the possibly shortened original tool name is compared against various values.

Change History (11)

comment:1 by Conrad Huang, 6 years ago

Cc: chimera-staff@… added; Greg Couch removed

What is the semantics we're going for with "toolshed show" (to be "ui show")? The current behavior seems wrong:

  • Look for all running instances whose display name exactly matches user input.
  • If no matching instance is found, look for all running instances whose tool name exactly matches user input.
  • If one or more running instances are found, show them all and return.
  • If no running instances are found, look for registered tools whose name starts with the user input.
    • If there is an exact match, select that registered tool.
    • If not, select a random one (first one found, but effectively random).
  • If a registered tool is selected, start the tool. Otherwise, report an error.

I propose (after brief discussion with Eric and Greg) that we change it to:

  • Look for running instances whose display name exactly matches user input. If found, show it/them and return.
  • Look for running instances whose tool name exactly matches user input. If found, show it/them and return.
  • Look for registered tools whose tool name exactly matches user input. If found, start it and return.
  • Look for running instances whose display name or tool name starts with user input. Also look for registered tools whose tool name starts with user input. If there is exactly one match, either show the instance or start the tool. Otherwise, report an error.

The down side that I can think of with this scheme is that the user cannot start a second copy of a registered tool if its instances do not change their display name. (If they do not change their names, they will all have the same tool name, and the user always match a running instance and therefore never fall through to starting another instance of the tool.)

Comments?

in reply to:  2 ; comment:2 by goddard@…, 6 years ago

Maybe the solution to starting a new instance of a tool when one already exists is to just have an additional option

	ui show MyTool new true


comment:3 by pett, 6 years ago

Well, the Tools menu uses "toolshed show" when you choose a menu item. It doesn't know if it should be adding "new true" or not. I suppose it could rummage around the running tools to try to find out, but then it would be doing the same work that "toolshed show" is doing now.

I think the logic needs to be:

1) Match running tools whose display [then tool] name exactly matches
2) Match non-running tools whose name exactly matches
3) Use "starts with" match against running tools
4) Use "starts with" match against non-running tools

Multiple matches shows all running tools, raises an error for non-running tools.

comment:4 by Conrad Huang, 6 years ago

That's essentially what I described except your (3) and (4) are prioritized and mine are lumped together. Yours has a nice and simple description for the algorithm, so I'll work with that unless/until someone objects.

comment:5 by pett, 6 years ago

If it turns out that we need to limit "toolshed show" to one of those steps, we can add keyword options later. But without keywords, it does all of those steps.

comment:6 by Conrad Huang, 6 years ago

Just to be clear, if there are exact matches, then we do not do the "start with" checks. Is that correct?

comment:7 by pett, 6 years ago

Correct.

comment:8 by Conrad Huang, 6 years ago

7050f598b9 is my first cut at the new semantics. Please try it and let me know if it works as expected.

The tool name is now case-independent. As described above, the "tool name" can now be a prefix. So "toolshed show basic" should now bring up the "Basic Actions" tool.

comment:9 by Greg Couch, 5 years ago

Milestone: 1.0

comment:10 by Greg Couch, 5 years ago

Owner: changed from Conrad Huang to Greg Couch

Improve error message when there are multiple matches to list the possibilities.

comment:11 by Greg Couch, 5 years ago

Resolution: fixed
Status: assignedclosed

Done.

Note: See TracTickets for help on using tickets.