Conversation
…perly- doesn't quite work yet
|
Tomorrow I will fix the tests and look at the PR description again, then it's ready for eyes |
|
I think this is "done"- if there are other tweaks to the workflow I'd like to include them here. If there are widgets that need to be renamed, now is a great time, but we can also do it after this merge but before the tag. If there are stylistic changes to the widgets themselves, I'd like to do them in separate PRs. |
| This is how you would convert a .ui file with macro substitution that is normally used with PyDMEmbeddedDisplay into a designer widget served from here. | ||
| This is how you would convert a .ui file with macro substitution that is normally used with `PyDMEmbeddedDisplay` into a designer widget served from here. | ||
|
|
||
| Note that we can currently only run designer with custom widgets on our Rocky9 OS machines at LCLS! |
There was a problem hiding this comment.
Is this a duplicate or did you mean to have it here and one line 42?
There was a problem hiding this comment.
It's repeated intentionally based on feedback that it's an easy thing to miss
| def __init_subclass__(cls): | ||
| super().__init_subclass__() | ||
| # Extend the _qt_designer_ marker if it exists to include a quick editor for macro vals | ||
| # Create _qt_designer_ for pydm if designer_options is present |
There was a problem hiding this comment.
When would designer_options not be present in a subclass?
There was a problem hiding this comment.
This would only happen if you edited the generated file to remove/rename it, or if you decided to make an intermediate base class that you don't want to be directly selectable in designer.
| # For running the again after pyproject.toml is generated in make all | ||
| venv-again: | ||
| ./build_local_venv.sh |
There was a problem hiding this comment.
Can't you just list venv as a dependency again in the all recipe since it's phony ?
There was a problem hiding this comment.
I'm not a make expert, but I some people cautioning against this due to the ordering of targets not being guaranteed (particularly when considering parallel make): SO
If we were to follow that pattern here, we'd want to use make recursively, rather than listing phony's in sequence as we do here?
There was a problem hiding this comment.
When I had venv listed twice it was only building the first time. I'm not sure if I was doing something wrong or if I saw a ghost but this made it rebuild consistently.
I didn't consider parallel make- that would definitely break the current setup, which really needs these steps to be done in order.
Probably my best option is to then follow the guidance of that stackoverflow post as Robert suggested. It's somewhat gross but it avoids all the obvious issues and avoids the need for a duplicate target.
There was a problem hiding this comment.
This ended up working out pretty well:
- The order is always respected now.
- There is no longer a duplicate target.
- If you do a parallel make, it can still do the build target (n python files to generate) in parallel, but it will do the rest one-by-one in sequence.
tangkong
left a comment
There was a problem hiding this comment.
I think this looks good on the whole, I mostly stared at the readme, but I can take it for another spin if desired
|
|
||
| group: str | ||
| is_container: bool | ||
| icon: str | QIcon | None |
There was a problem hiding this comment.
This is far beyond the scope of this PR, but wonder if it's possible to have pylance / linters know what font packs are pre-loaded. I originally thought this might take the form of an enum, where the linter would expose the available options. The ultimate goal would be to give feedback before compilation, but I realize this is also high effort - low payoff.
Maybe it's not possible to know before installation? Perhaps defining all the icon sources is also a poorly defined problem.
Just some food for thought
There was a problem hiding this comment.
It's definitely possible to generate an enum from the keys defined in this json dict: https://github.com/slaclab/pydm/blob/master/pydm/utilities/fontawesome-charmap.json
I wonder if also it would be easy to make an entrypoint that renders all the options- or maybe I should do that as a pydm PR?
If we add other icon sources we could expand the enum accordingly.
There was a problem hiding this comment.
In principle the options would change as pydm updates, but in practice it never has (see that last commit date/user)
There was a problem hiding this comment.
I forgot that character map existed (or perhaps I never knew). It'd be nice to keep these kinds of info dumps in one place, so grabbing it from pydm seems like the most efficient way. Not that I've thought too heavily about it here.
May it stay the same for another 5 years 🙏
There was a problem hiding this comment.
I had some initial trouble with the enum, so I made this a union, but that didn't autocomplete so I'm going back to an enum. The names of the enum members need to change slightly from the actual string because most of them are not valid python identifiers.
I think it will be pretty easy to take this list of icons and generate a definitive grid of icons to browse.
I think this is worth doing because I was finding it pretty annoying to pick icons myself.
There was a problem hiding this comment.
Making a gui to show all the options opened a can of worms- something like 1/3 of all icons render corrupted, largely all the coporate icons. I started going through them and noting which ones don't work but it's a lot. I'm not 100% sure if I'm going to finish this manually, find a deterministic way to exclude them from the list, or drop the ida.
There was a problem hiding this comment.
I managed to cut it down to only the actual valid icons without further manual inspection
|
|
||
| 1. Create a widget as a PyDM screen | ||
| - Use qt `designer` to define the layout (saves a .ui file) | ||
| - Use `PyDM` macros to define user inputs |
There was a problem hiding this comment.
Should we link to documentation for setting up PyDM macros here?
This is the easiest page to find: https://slaclab.github.io/pydm/tutorials/intro/macros.html
But some help for setting this up in Designer might be good too. I guess the pydm tutorials are as close as we can get to that here? https://slaclab.github.io/pydm/tutorials/action/designer_inline.html
There was a problem hiding this comment.
Good finds for the pydm documentation- I'll add these links right here.
It's probably also time to admit that this setup does not support this part of the pydm macro substitution as defined in the first link:
you can insert macros anywhere in a .ui file using a text editor
because this would require completely destroying and rebuilding the uic class every time someone changed the properties
There was a problem hiding this comment.
Some concessions must be made in the name of progress
| - Use qt `designer` to define the layout (saves a .ui file) | ||
| - Use `PyDM` macros to define user inputs | ||
| 2. Try it! | ||
| - Use `PyDMEmbeddedDisplay` to include your widget in other screens |
There was a problem hiding this comment.
If I were completely new to these widgets, I might not know how to do this or what this means.
Do we mean something similar to the steps here?: https://slaclab.github.io/pydm/tutorials/action/python.html
There was a problem hiding this comment.
You're right- a lot of familiarity with pydm is assumed here.
Possibly this one is the best example of an embedded display in use?
https://slaclab.github.io/pydm/tutorials/action/designer_main.html
There was a problem hiding this comment.
I think that's appropriate, at least if someone ends up there and confused they're surrounded by other documentation
| # For running the again after pyproject.toml is generated in make all | ||
| venv-again: | ||
| ./build_local_venv.sh |
There was a problem hiding this comment.
I'm not a make expert, but I some people cautioning against this due to the ordering of targets not being guaranteed (particularly when considering parallel make): SO
If we were to follow that pattern here, we'd want to use make recursively, rather than listing phony's in sequence as we do here?
There was a problem hiding this comment.
NITPICK: I'd have had the examples/instructions be gathered and at a higher heading level. I think that's most of what people will be looking for in this README, and making it big and easy to find would probably be appreciated.
Maybe I just find it slightly jarring to go between notes on naming conventions and how-to steps.
There was a problem hiding this comment.
It's worth rethinking the whole README tree at this point- I was hedging between documenting different widget types, naming conventions, etc.
There was a problem hiding this comment.
This is the final remaining TODO: I think I'm happy with the content of the readme, but the organization should be more in-line with the tutorial/how-to/reference/explanation framing, with a focus on tutorial first and foremost
…to skip install in some cases
tangkong
left a comment
There was a problem hiding this comment.
I like this a lot, the docs are lot clearer to me now as well. one last little nitpick and I think it's time to get this in
| - In `pydm`, you can edit a ui file by hand and add a macro anywhere. This is not supported for composite widgets. | ||
|
|
||
|
|
||
| ### Adding a Symbol-based Vacuum Widget |
There was a problem hiding this comment.
This section seems to not have anything to do with Vacuum, seems like we could be more general?
There was a problem hiding this comment.
I think you're right, in practice this has only been used for vacuum widgets but in principle it doesn't need to be. I'll revise.
There was a problem hiding this comment.
I get no such option: --uploaded-prior-to if I try to run this without sourcing a pcds_conda environment first
There was a problem hiding this comment.
Actually I can't successfully build the uv environment with pcds_conda either
There was a problem hiding this comment.
I think this was meant to be run with no environment sourced at all.
--uploaded-prior-to should come in as an option when we update pip to the version in the base environment specified here.
There was a problem hiding this comment.
I'll spend at least a little bit time trying to figure out how to make this robust against having an environment active
There was a problem hiding this comment.
This works for me in both variants with or without a conda env sourced. I'm curious to see your error outputs.
There was a problem hiding this comment.
I restarted moba and can't reproduce it now. Maybe I accidentally interrupted the venv build before pip was updated?
|
I'm not pleased that one of the test builds has segfaulted- hopefully this is rare |
KaushikMalapati
left a comment
There was a problem hiding this comment.
Briefly tried the scripts and making my own composite/symbol widgets. Everything in the readme is well-explained to me and I appreciate how easy it is to build/customize widgets. I imagine people will ask for a lot of tweaks once this releases, so maybe you should plan to do a mini-update after a month or two.
Description
This adds a pipeline for converting composite PyDM widgets into standalone widgets for inclusion as one unit in PyDM screens.
It also adds four widgets from ecs-pydm-ui, identified by Vincent as good examples.
The main point here is that we can take a .ui widget (tested with
PyDMEmbeddedDisplay) and add it as a first-class widget in designer without editing the .ui file at all. Here, I did edit the ui files to standardize widget sizing and to bootstrap this widget-in-a-widget situation, but in general you wouldn't need to.The pipeline involves generating .py files from these .ui files. I'm doing this at build time, not at runtime, because:
To make this simpler to use, I created a small-ish
Makefileso if you have the venv sourced you can justmakeand it will generate everything it needs to. The default target,make all, will do the following in order:This means you can clone the repo, drop in a ui file in the correct directory,
make, and then try it out quickly (using mytry_in_designer.shandtry_in_pydm.shscripts).Encoded into this are some requirements for widget sizing and naming to make sure widgets get added in a consistent way in the future. These are enshrined in unit tests to make sure we stick to them.
Style was updated in this PR using ruff- apologies for the seemingly unrelated diff sections.
I'm also gave uv a trial for this PR- I think I like it a lot and would use it again. I'm not sure how well it meshes with our general --system-site-packages workflow, but otherwise I'm all +++ on it. Using uv is why some of the package structure changed and a few boilerplate files were added.
Motivation and Context
There is an effort starting now to put more effort into tools for manually creating PyDM screens on the LCLS photon-side.
We would like to build screens in a sustainable way- sharing curated widgets and reducing the barrier of entry for doing so is one step in this effort.
How Has This Been Tested?
Interactive testing with designer and with pydm.
I added some basic unit tests to make sure the plugins are set up OK and all the widgets have the designated naming and sizes.
Where Has This Been Documented?
I rewrote the README
I have some thoughts about the overall workflow in
https://confluence.slac.stanford.edu/spaces/PCDS/pages/683344859/ECS+UI+UX+Screen+Guidelines+and+Best+Practices#ECSUI%2FUXScreenGuidelinesandBestPractices-ImplementingCustomWidgets
But probably the README is more direct for this purpose.
Screenshots (if appropriate):