Select Instances from diff tree

[boatbomber]

Closes #707.

Double clicking selects the Instance, if the Instance exists. Single clicking opens the changes list, if changes exist.

Tooltips indicate which actions are available for the underlying DOM element.

2023-07-04.12-52-46.mp4

[chriscerie]

Great change. It's not immediately obvious that right click selects the instance. Especially when the behavior allows selection for left click without a changes table, I would just think that selecting is only possible for instances without changes. I'd suggest either:

• having double click select instances
• having select be the default activated behavior and including dropdown arrow to expand changes
• including tooltip when hovering to indicate right click to select

[boatbomber]

Thanks for the feedback!

• Double click seems good, and ties in with the mental model of opening files on a computer so it should be intuitive. Good idea!
• Viewing the dropdown is the primary action, plus horizontal space is pretty tight so a dropdown arrow is a no go
• A tooltip is a good call, I keep forgetting to add those!

[chriscerie]

Selecting instances probably shouldn't also trigger dropdown expansion. Otherwise if a user already has it expanded and want to select the instance, they'd need to select it and open changes back up.

The delay here can be up to debate. Microsoft has 500ms as the default, but I find 200-300ms to be sufficient for this case.

[Dekkonot]

So, as a miner pet peeve, I've noticed that it this seems to only work for the top-level of a change's tree.

As an example, if a patch like this is present:

you could double click on Common to select it in the explorer but not Hello.

My expected behavior would be that it would work for both. Is that something that can be easily fixed?

[chriscerie]

@Dekkonot Is this for connecting to rojo? It’s only able to select it in the explorer if it exists. It looks like you’re adding the script than modifying it (assuming green + is instance addition)

[Dekkonot]

@Dekkonot It’s only able to select it in the explorer if it already exists. It looks like you’re adding the script than modifying it (assuming green + is instance addition)

This is on initial sync into a new place file and then I'm clicking through the patch visualizer after confirming that sync. I was surprised it worked with Common since I thought as you did that it wouldn't work for additions. Given that it worked though, my expectation would be that it worked on all additions.

[boatbomber]

That's odd. If the instance exists (and the instanceMap is aware), it should be clickable.

[chriscerie]

It looks like instance isn't being passed to addition nodes. Passing the instance here should fix it.

rojo/plugin/src/App/Components/PatchVisualizer/init.lua

Lines 341 to 342 in be2109c

	changeList = changeList,
	})

-	for _, change in patch.added do
+	for id, change in patch.added do

		...
		
		tree:addNode(change.Parent, {
			id = change.Id,
			patchType = "Add",
			className = change.ClassName,
			name = change.Name,
			hint = hint,
			changeList = changeList,
+			instance = instanceMap.fromIds[id],
		})
	end

[boatbomber]

Good catch, thanks guys!