Skip to content

Conversation

@cappel89
Copy link
Contributor

@cappel89 cappel89 commented Oct 20, 2025

Description

This PR refactored and improved the device-manager view. The following things were addressed:

  • Wrote a bespoke DeviceForm to add new devices based on templates (EpicsMotor, EpicsSignal, ...) to the device manager view. The widget is embedded in form of a dialog, and hooked up to all functionality. It also allows testing the device validity and connection.

  • Refactoring of DMOphydTest widget. Although in theory working, there were a few issues with the widget, and also the test mechanism itself. The following factors were improved:
    I. Validation is tested for two stages. ConfigStatus and ConnectionStatus. This represents if the static-ophyd-test is started with "connect".
    II. Results of the static-ophyd-test are split into the above mentioned status stages. This is needed to differentiate between, my config is in principle okay, but I can't connect to the device right now.

  • All refactored widgets are combined together in the view, and are ready for testing. I attach below a snapshot how the arrangement of individual docks makes sense to me as these ratios are not yet set up in the view.

  • TODO, consider creating a legend for icons in the Toolbar. I was running a bit out of time, and would also like to receive some input first.

  • Tests need to be reviewed, and extended for the OphydTest widget.

  • Add e2e tests

  • Wait for feeedback @wyzula-jan @d-perl : If time allows, please test the functionality in depth. if possible use the widget and play a bit around. Try to break it :)

Note: Depending on the amount of feedback, I think that I could wrap up tests and feedback within a day, with exception of e2e test.

Screenshot 2025-10-23 at 20 37 30

Related Issues

closes #886
closes #887
closes #885
closes #884
closes #883
closes #882
closes #839

Definition of Done

  • Documentation is up-to-date.

@cappel89 cappel89 force-pushed the feat/add_new_form_to_dm_view branch 3 times, most recently from 6e6e0c4 to 0af5abc Compare October 23, 2025 18:49
@cappel89 cappel89 self-assigned this Oct 23, 2025
@cappel89 cappel89 force-pushed the feat/add_new_form_to_dm_view branch from 0af5abc to 02d17c2 Compare October 23, 2025 19:06
@cappel89 cappel89 requested review from d-perl and wyzula-jan October 24, 2025 14:25
Copy link
Contributor

@wakonig wakonig left a comment

Choose a reason for hiding this comment

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

a quick first round ;)

@wyzula-jan
Copy link
Contributor

If you delete all devices and try to upload the validation hangs forever:

Screen.Recording.2025-10-30.at.10.33.54.mov

@wyzula-jan
Copy link
Contributor

I would switch replace and cancel, the similar approach could be used for all other dialogs
image

Check for example this article about MS/Apple/Google UX guidelines

@wyzula-jan
Copy link
Contributor

If I click on validate connection and nothing is selected I would expect that the validation will be submitted for all devices. The current logic submits only selected field in the table:

Screen.Recording.2025-10-30.at.10.47.20.mov

@wyzula-jan
Copy link
Contributor

There is something strange happening if I try to revalidate already validated device. In GroupBox Completed Validation there are some ghost items appearing if I smash the validate button multiple times.

I also think that user should be able to rerun validation on already validated device in case that idk it was unplugged accidentally etc. (maybe with some dialog such as "Do you want to run validation again on already validated devices?"). Current implementation just do something but do not inform user what is actually happening. If validation should not be available than I would add dialog or disable button completely.

Screen.Recording.2025-10-30.at.11.17.30.mov

@wyzula-jan
Copy link
Contributor

I have an invalid device none, if I click on the toolbar action it tries to validate and I can try multiple times. If I try to do the same through the upload dialog nothing seems to happen.

Screen.Recording.2025-10-30.at.13.04.30.mov

@wyzula-jan
Copy link
Contributor

scrollbar in the upload dialog has wrong render area:

Screen.Recording.2025-10-30.at.13.07.36.mov

@wyzula-jan
Copy link
Contributor

upload do not change config it there is at least one device which is not validated. Here I had in my config bpm3a, then I removed it from gui and uploaded to redis. I got warning which I accepted but nothing is actually uploaded to redis. If this is intended behaviour then user should be notified that the upload didn't went through.

Screen.Recording.2025-10-30.at.13.11.18.mov

@wyzula-jan
Copy link
Contributor

please use material icons instead of default qt icons for these 2 buttons in the dialog
image

@cappel89 cappel89 force-pushed the feat/add_new_form_to_dm_view branch from ca18f62 to efa6e39 Compare November 4, 2025 09:49
@cappel89 cappel89 force-pushed the feat/add_new_form_to_dm_view branch from 5e4dddf to 5bfb0ac Compare November 11, 2025 12:12
@wakonig
Copy link
Contributor

wakonig commented Nov 12, 2025

@cappel89 I just played around with it a bit and noticed that the device tags are not working when opening the device form: Existing tags are not shown and upon saving, there is a None added as well.

PresetClassDeviceConfigDialog,
)

logger = bec_logger.logger
Copy link
Contributor

Choose a reason for hiding this comment

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

you can safely remove the set_splitter_weights function. If necessary, it is already in view.py

@cappel89 cappel89 force-pushed the feat/add_new_form_to_dm_view branch from 1527097 to 293b756 Compare November 13, 2025 16:57
@cappel89 cappel89 force-pushed the feat/add_new_form_to_dm_view branch from 3366e8e to 11bfef9 Compare November 14, 2025 13:16
Copy link
Contributor

@d-perl d-perl left a comment

Choose a reason for hiding this comment

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

  • in the device config dialog form, I think it's a bit confusing to show the device class when it's already selected at the top, I would hide this field and only show it for the custom device
image
  • column headings are not nicely formatted / correct width, and the checkboxes are not centred (on linux)
image
  • Default size of the config dialog could be a bit bigger
image

@wyzula-jan wyzula-jan force-pushed the feat/add_new_form_to_dm_view branch from 9da4726 to a86d1cd Compare November 18, 2025 19:08
@wyzula-jan wyzula-jan force-pushed the feat/add_new_form_to_dm_view branch from 7671ea4 to f9645f5 Compare November 21, 2025 09:57
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.

5 participants