-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Autogenerate images of parts of the viewer #621
base: main
Are you sure you want to change the base?
Conversation
Adding a few links from the community meeting today: |
docs/_scripts/autogenerate_popups.py
Outdated
QTimer.singleShot(100, lambda: trigger_popup( | ||
viewer_buttons.ndisplayButton, | ||
"ndisplay_popup", | ||
lambda: trigger_popup( | ||
viewer_buttons.gridViewButton, | ||
"grid_popup", | ||
lambda: cleanup(viewer) | ||
) | ||
)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a good solution. We need to find a way to define a list of popups to iterate over it.
Maybe global list and just pass the index?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you look strictly at autogenerate-gui? I've put the most recent popup stuff in there. It's flaky, where sometimes it works and sometimes it doesn't, I've tried to include closing popups there, but yes, trying to work towards creating a list. Sorry I didn't remove this file 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in 2-3 hours
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
of course! no issue, I'm actually going to plug along on it for about another hour, so you should receive a better file by then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
your review did at least confirm some of the direction I thought it needed to go
docs/_scripts/autogenerate_popups.py
Outdated
for widget in QApplication.topLevelWidgets(): | ||
if isinstance(widget, QtPopup): | ||
popup = widget |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous popup is not closed before your code triggers creation of the next popup, so the first popup is captured again. We either need to use proper class in place of QtPopup
or use objectName()
method (with earlier setting it).
@Czaki
We can add / change more and modify callbacks in various ways, but as @willingc we can start with a merged script and go from there. I hope it's not flaky on anyone else's machine, or CI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge this as is. I would like to do a pass over the script and change some of the print statements to logging (it will benefit us when running in CI).
Sounds good, the prints were really just for debugging lol. I take them out as things start to work. Definitely anything with prints from me is a 'draft'. Can still merge though if you like We should be able to see if CI autogenerates correctly |
I would like us to merge and iterate on it. |
Can we add a comment in the script to indicate which images it is generating? |
@melissawm I added much more commenting to try to clarify what is being saved and I hope the code organization is sufficient to be self explanatory. However, if what you mean is to specifically say which images for the docs this is generating, I'm not sure that's a good idea (will be hard to maintain and keep up to date). Please let me know if I haven't accomplished the goal. I've also added various interactions with the canvas to change the display of various widgets -- we can always be very specific if we have things in mind to demonstrate. At this point, I'm very happy with this implementation, and want us to merge this to check it works on CI, and then we can look to specifically define images of interest that can be used in the docs. I also added a preliminary, rough implementation of capturing regions of the viewer as defined by the widgets. In the long run, this will be more maintainable than defining the region with our own coordinates (since inevitably these coordinates could move around). Currently, it just crops to the bounds of the defined widgets, for example: |
References and relevant issues
Description
This is really a draft of trying to autogenerate images that we can use throughout the docs. I think this would also speed up doc generation in certain areas with reusable doc images, so that nbscreenshot would not need to be as frequent. Everything is getting saved into
images/_autogenerated
and could be used elsewhere.Needs:
Examples of what should appear in autogenerated if CI does it the same was as locally
