Skip to content

Conversation

@bparks13
Copy link
Member

@bparks13 bparks13 commented Oct 29, 2025

For all custom dialogs, there is now an added tab that contains a PropertyGrid to display all properties for a device. This is always the non-default tab, so that the priority is on the custom GUI, but it is always present should the user want to look at and change any properties that are not exposed via custom controls.

Any changes made in either the custom GUI or the PropertyGrid are operating on the same underlying object, which is the object attached to the PropertyGrid, so that all data is always synchronized.

Neuropixels V1
image

Neuropixels V2
image

Rhs2116
image

Headstage64
image

image

Fixes #515

@bparks13 bparks13 added this to the 0.7.0 milestone Oct 29, 2025
@bparks13 bparks13 requested a review from jonnew October 29, 2025 19:04
@bparks13 bparks13 self-assigned this Oct 29, 2025
@bparks13 bparks13 linked an issue Oct 29, 2025 that may be closed by this pull request
@bparks13 bparks13 requested review from aacuevas and removed request for jonnew October 30, 2025 19:48
@bparks13 bparks13 force-pushed the refactor-rhs2116-design branch from 7eae139 to 8787216 Compare November 3, 2025 19:25
Base automatically changed from refactor-rhs2116-design to main November 3, 2025 19:54
@bparks13 bparks13 marked this pull request as ready for review November 3, 2025 19:55
@jonnew jonnew self-requested a review November 11, 2025 15:01
Copy link
Member

@jonnew jonnew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed during live code review, please move the ProbeGroup editor into the the specialized stimulus waveform property editor on the RHS stim gui so that the base class does not have to be informed about its existence.

@bparks13
Copy link
Member Author

bparks13 commented Nov 17, 2025

Per the live review with Aaron, I will be changing this to utilize DataBindings in addition to using the event hooks to enable easier synchronization between the propertyGrid and the custom controls.

- Remove constructor arguments from base class
- Rhs2116 is not refactored, as the controls are not 1:1 tied to an underlying object, but are dynamic controls depending on what is selected
@jonnew
Copy link
Member

jonnew commented Dec 2, 2025

Im doing a quick user level exploration of this and noting some stuff that im finding. First issue:

  1. on the hs-64 optical stim dialog, there is no magin on the Define Stimuli tab
  2. The pulse period and x axis on plot dont have the same units (I thought I fixed this by redefining stimuli in terms of frequency for optical stimuli in some other commit, maybe that has not been pulled in?)
image

@jonnew
Copy link
Member

jonnew commented Dec 2, 2025

On Neuropixels dialog, it looks like some properties dont have an Acqusition/Configuration category attribute:

image

Please do general check for this

@jonnew
Copy link
Member

jonnew commented Dec 2, 2025

I understand why the the Properties tab exists here, but its very odd because it looks like its the same "kind of thing" as a probe and also

  1. redundantly exposes a lot of probe configuration properties.
  2. has clickable ellipses on the Probe Configuration property that don't do anything
image

I would prefer something like this:

image

@jonnew
Copy link
Member

jonnew commented Dec 2, 2025

I cant resize the properties pane to see to text on it:

image

@jonnew
Copy link
Member

jonnew commented Dec 2, 2025

Overall, I like the properties panes. Can you please have a look at the above issues before a proper review. Also take care to ensure that OOP and idiomatic programming principles are being followed before I look at the code.

- Add categories to properties
- Allow the GUI panels to be resized so the property grid can be better visualized
- Fix the stimulus sequence options so that they resize properly
- Adjust NeuropixelsV2Dialog to show device properties at all times, and display probe configuration dialogs when selected from the property grid
- NPX V2 probe configuration dialogs are not able to be opened from NPX V2e device dialogs
- Close the probe configuration dialog when any property other than ProbeConfiguration is selected
- Add text to panel giving instructions for the user on what to do to show the probe configuration dialog
@bparks13
Copy link
Member Author

@jonnew This PR is updated based on our discussion this morning, with the changes to the ellipsis and the NeuropixelsV2e dialogs. Below is a GIF showing the updated behavior of the NeuropixelsV2e dialog, showing the probe configuration dialog updating and being removed as needed, as well as the default text being shown in the background when no probe configuration property is selected.

npxv2e-dialog-update-2

- Hide certain properties when the property grid is displayed in a custom GUI, specifically hiding the DeviceName/DeviceAddress properties when a dialog comes from a ConfigureHeadstage* node to match the behavior of the Bonsai editor
@jonnew
Copy link
Member

jonnew commented Dec 17, 2025

When some property other than the probeconfiguration is selected, we get a helpful dialog in the area where the probe would be shown. I like this but I want a couple changes.

  1. When I resize the form, the text is does not remain centered

    image
  2. When I select a probe configuration, but I don't have a calibration file loaded, I would like the text to change to "Please provide a calibration file to configure your probe"

    image

Copy link
Member

@jonnew jonnew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets have a live review of this one, I'm not getting the clone() stuff.

/// Returns a deep copy of the current <see cref="ConfigureNeuropixelsV2e"/> instance.
/// </summary>
/// <returns><see cref="ConfigureNeuropixelsV2e"/> instance.</returns>
public IConfigureNeuropixelsV2 Clone()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we adding this to the public interface instead of using the copy constructor like all the other configuration operations do? It looks like you are immediately interpreting the result as concrete ConfigureNeuropixelsV2e type anyway?

Copy link
Member

@jonnew jonnew Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another issue I have with this is that its not an interface that actually describes a contract for a "ConfigureNeuropixelsV2" but for some very specific type of headstage that has two NeuropixelsV2ProbeConfiguration's and a common enable. For instance, the headstage I'm implementing for the Allen that has a NPX 2.0 and Intan cannot use this interface.

So basically, the logic is backwards here: the interface was made posthoc to accommodate a set of three properties that happened to be common to a couple specific classes in the library. It was not designed from the beginning as a contract for describing a set of generic functions that are applicable to all e-style headstages that have neuropixels v2(s).

If we create an interface for configuring those headstages that have neuropixels, it needs to be able to handle all cases

- Fix the device dialog label to remain centered when resizing
- Add label to probe configuration dialogs (V1/V2) with instructions on how to open the probe configuration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add property pane tab so device parameters are always available

3 participants