Fold experiments qt6 #2724

mmalczak · 2025-04-16T21:29:42Z

ARTIQ Pull Request

Description of Changes

Refactor widgets creation - move to smaller methods.
Split layout into always visible layout and foldable layout with scheduler settings.
Adjust sizes and resizing behavior.
Save/restore state of the layout.

Type of Changes

	Type
✓	✨ New feature

All Pull Requests

Use correct spelling and grammar.
Check the copyright situation of your changes and sign off your patches (git commit --signoff, see copyright).

Code Changes

Run flake8 to check code style (follow PEP-8 style). flake8 has issues with parsing Migen/gateware code, ignore as necessary.
Test your changes or have someone test them. Mention what was tested and how.
Add and check docstrings and comments
Check, test, and update the unittests in /artiq/test/ or gateware simulations in /artiq/gateware/test

Git Logistics

Split your contribution into logically separate changes (git rebase --interactive). Merge/squash/fixup commits that just fix or amend previous commits. Remove unintended changes & cleanup. See tutorial.
Write short & meaningful commit messages. Review each commit for messages (git show). Format:
```
topic: description. < 50 characters total.

Longer description. < 70 characters per line
```

Licensing

See copyright & licensing for more info.
ARTIQ files that do not contain a license header are copyrighted by M-Labs Limited and are licensed under LGPLv3+.

Signed-off-by: Miłosz Malczak <[email protected]>

SimonRenblad · 2025-04-17T07:07:26Z

Thank you for the PR! Can you split it up into smaller ones / push the refactor separately?

SimonRenblad

Folding experiment settings:
I agree with the premise that we can potentially hide the scheduler settings, although I am not sure about this approach. Even when collapsed, the "Expand scheduler settings" button is still taking up real estate. I feel like this could be resolved through other means. For example, by adding a checkbox menu item to the existing context menu on the experiment dock. See also how docks in the rest of the application can be closed through a context menu.

Resizing changes made to experiment dock/scheduler settings:
Personally, I don't think the resizing changes made here look good enough to justify the change, but I suppose that's up to debate. Can it be a separate PR?

SimonRenblad · 2025-04-17T07:08:48Z

artiq/dashboard/experiments.py

    def __init__(self, manager, expurl):
-        QtWidgets.QMdiSubWindow.__init__(self)


Why? We do not use super in most other qt subclasses.

SimonRenblad · 2025-04-17T07:14:00Z

artiq/dashboard/experiments.py

+        self.on_fold_toggle()
+
+    def on_fold_toggle(self):
+        """Toggle the visibility of the options."""


remove docstring

SimonRenblad · 2025-04-17T07:27:41Z

artiq/dashboard/experiments.py

+                ideal_width = self.sizeHint().width()
+                self.resize(ideal_width, self.height())
+            if self.height() < 285:
+                self.resize(self.width(), 285)


Are these resizing changes necessary to get the folding behavior? I am skeptical of manual resizing with magic values unless really necessary.

IIRC this was to prevent issues when expanding settings on small window.
E.g. as shown here (last picture):
elhep#8 (review)

I agree that magic values are not the best way to do it, however otherwise we may need to calculate the height based on all visible elements.

If it can be easily resolved by the user resizing the window, I personally think it's acceptable that it looks a bit weird when the windows are small. This is common for many GUI applications as a trade-off for being flexible. It also would not block getting the folding behavior (so it could go in a later PR).

SimonRenblad · 2025-04-17T07:29:59Z

artiq/dashboard/experiments.py

-        buttons.layout.setColumnStretch(3, 1)
-        self.setItemWidget(self.bottom_item, 1, buttons)
+        self.setItemWidget(self.bottom_item, 0, load_hdf5)
+        self.setItemWidget(self.bottom_item, 1, recompute_arguments)


nit: I personally think this change looks worse than before. Also is it necessary?

Yes, it looks a bit worse, however it was a simple way to retain the visibility of these buttons when the window is narrow. Screenshots from the original PR:

Minimum width is too low when scheduler settings are hidden -

maybe make a single large row for recompute and load buttons
maybe base minimum width on parameters - this might be tricky if there are groups

Was this always the case, or introduced by your layout changes? If it was always the case, I still maintain that it can go in a separate PR as it addresses a separate concern (not related to folding behavior).

It was introduced - before it was impossible due to minimum width of the window being much bigger.

SimonRenblad · 2025-04-17T08:04:22Z

artiq/dashboard/experiments.py

        }

    def restore_state(self, state):
        self.argeditor.restore_state(state["args"])
        self.restoreGeometry(QtCore.QByteArray(state["geometry"]))
        self.hdf5_load_directory = state["hdf5_load_directory"]
+        if "options_expanded" in state:	 # Only useful for the upgrade


#2721 (comment)

SimonRenblad · 2025-04-17T08:07:19Z

artiq/gui/entries.py

@@ -23,7 +23,7 @@ def __init__(self):
            set_resize_mode = self.header().setSectionResizeMode
        else:
            set_resize_mode = self.header().setResizeMode
-        set_resize_mode(0, QtWidgets.QHeaderView.ResizeMode.ResizeToContents)
+        set_resize_mode(0, QtWidgets.QHeaderView.ResizeMode.Stretch)


We want less width to go to the argument title (typically a short label) and more width to go to the argument widget (typically multiple components, sliders etc). Now its a 50/50 split. Why the change?

marmeladapk · 2025-04-17T11:43:23Z

The driving idea behind this PR was to make smaller windows possible so that more experiments fit in a window. If you have an experiment that takes little to no arguments, then scheduler settings take majority of space and limit how small the window can be. That's why layout and sizing changes are in a single PR.

Corresponding PR in which you can trace the origin of these changes in relation to problems that I found:
elhep#8

Even when collapsed, the "Expand scheduler settings" button is still taking up real estate. I feel like this could be resolved through other means. For example, by adding a check box menu item to the existing context menu on the experiment dock. See also how docks in the rest of the application can be closed through a context menu.

Yes, it's still taking up space, however I wanted this button to be still visible because:

I feel that this state should be per experiment window not globally, so that you can still expand these settings selectively on experiment that use e.g. due date.
In general I feel that context menus are not easily discoverable.
In a future PR we want to add a highlight to indicate that there are non-default values in scheduler settings.

This way if the scheduler doesn't behave the way you were expecting it to then you at least have a visible indicator that there are some additional settings. This may help also in a shared environment, where someone else might have made the changes.

SimonRenblad · 2025-04-18T04:38:27Z

The driving idea behind this PR was to make smaller windows possible so that more experiments fit in a window. If you have an >experiment that takes little to no arguments, then scheduler settings take majority of space and limit how small the window can >be. That's why layout and sizing changes are in a single PR.

At least the widget refactor should be separate. I understand that it's the same idea, but sizing changes can be implemented without folding and vice versa. Minimal and atomic changes are preferred.

Yes, it's still taking up space, however I wanted this button to be still visible because:

I feel that this state should be per experiment window not globally, so that you can still expand these settings selectively on experiment that use e.g. due date.

A context menu can be per experiment window, and it is what I am suggesting.

In general I feel that context menus are not easily discoverable.

A context menu is:

easier to implement
minimally invasive: does not alter the existing layout
makes copying this behavior to other areas of the application significantly easier
can be easily combined with more discoverable approaches (such as adding a toolbar for each window)
is IMO sufficiently discoverable considering the niche / optional use case presented here

In a future PR we want to add a highlight to indicate that there are non-default values in scheduler settings.

This can be done without tying it to a button.

I would also accept:

A toolbar per experiment dock with a "View" menu, commonplace in many GUI apps and could be extended to hiding the arguments area (if the user has no arguments and doesn't care to see it).
Using a tree widget or other Qt widget that has built in collapsible children.

My point is that a PR concerned with looking better / getting more functionality when height is compressed would want to consider a way to achieve this without adding another fixed height component into the layout.

mmalczak · 2025-04-18T16:19:28Z

I added separate PR for widgets refactor:
#2730

easier to implement
The changes are already implemented

minimally invasive: does not alter the existing layout
I would argue that moving the changes to context menu would be a bigger change

Probably fixed height could be removed somehow.
The issue is when you make the window as small as possible with collapsed settings and expand the settings.
Then you get:

It resizes properly when you eg. move the window. It must be missing some kind of repaint somewhere.

If there is no fixed height, would the change be more acceptable for you?

SimonRenblad · 2025-04-22T03:37:14Z

If there is no fixed height, would the change be more acceptable for you?

That is one factor to consider, and a point in favor of the context menu. I only mentioned it specifically since the motivation of the PR is to get more functionality out of a small window. I don't think that having some kind of "variable height" button is a good substitute.

It resizes properly when you eg. move the window. It must be missing some kind of repaint somewhere.

Can it be solved with something like an update call?

It is preferable if the solution can be easily replicated to other areas of the application, by encapsulating the collapsible logic in a separate class.

If you are insistent on going the route of the button, here are a few other suggestions:

Drop the "Expand/Collapse". The title of the section with the arrow is sufficient to communicate the functionality
Styling: Hide the border of the push button and add a horizontal spacer to the right of it, as is common for this kind of component.

marmeladapk · 2025-04-22T17:04:21Z

can be easily combined with more discoverable approaches (such as adding a toolbar for each window)

is IMO sufficiently discoverable considering the niche / optional use case presented here

My point is that a PR concerned with looking better / getting more functionality when height is compressed would want to consider a way to achieve this without adding another fixed height component into the layout.

Hmm, toolbar would also add a fixed height component for each window. But one could also move existing context menus there which would be beneficial imho. I'm still strongly opposed to them. :)

If you are insistent on going the route of the button, here are a few other suggestions:

How would you feel about having no text and putting this button in the same row as submit/terminate buttons? That way we don't add any height to the layout. Notification about changed settings there could be done e.g. by styling the button border? Right now nothing better comes to my mind. And if settings are encapsulated as you suggested then it could be moved into a toolbar later.

Like this, but in a single row (ignore weird scaling, that was early version).

SimonRenblad · 2025-04-23T03:31:08Z

Hmm, toolbar would also add a fixed height component for each window. But one could also move existing context menus there which would be beneficial imho. I'm still strongly opposed to them. :)

A fixed height component that is above the layout and not in between existing widgets, but I see your point :). Clearly my preference is for a context menu on it's own. I don't quite understand the opposition to context menus, we already have plenty of features in context menus / shortcuts (the applet dock comes to mind). I mentioned fixed height, but also ease of implementation and repeatability. I think reduced discoverability is a fair trade-off in this specific case.

Given your strong opposition to a context menu, perhaps look into using a QSplitter in between the entry area and the experiment settings that snaps to hide either widget when pulled to it's respective limit. It would probably avoid some of the layout problems that come with a small window size.

How would you feel about having no text and putting this button in the same row as submit/terminate buttons? That way we don't add any height to the layout. Notification about changed settings there could be done e.g. by styling the button border? Right now nothing better comes to my mind. And if settings are encapsulated as you suggested then it could be moved into a toolbar later.

Having it be below does not seem intuitive for a collapsible widget, and either way you then introduce a fixed width. My point was about the footprint of a button (some fixed height/width) vs a context menu (none). If you insist on using a button, keep it on top and make it look like a collapsible section header (no border, and optionally a horizontal spacer on the right of the title). Then make it a generic widget that can be reused in other areas of the application with minimal modification.

dnadlinger · 2025-04-23T09:32:33Z

For a QSplitter/hidden section approach, it would also be possible to relcaim further space by not showing the Submit/Request termination buttons, as many expert users use the keyboard shortcuts for this (Ctrl+Enter, Ctrl+Backspace).

mmalczak added 8 commits April 16, 2025 20:09

Refactor widgets creation, add smaller methods #3

21f98d0

Signed-off-by: Miłosz Malczak <[email protected]>

Split layout into always_visible_layout and foldable_layout #3

8337797

Signed-off-by: Miłosz Malczak <[email protected]>

Refactor objects ownership #3

010dc34

Signed-off-by: Miłosz Malczak <[email protected]>

Resizing adjustments #3

f653377

Signed-off-by: Miłosz Malczak <[email protected]>

Make sure that the expand button text appears #3

92355ea

Signed-off-by: Miłosz Malczak <[email protected]>

Remember state of managment expansion #3

66d58ca

Signed-off-by: Miłosz Malczak <[email protected]>

Stretch the first entry element #3

6b26da1

Signed-off-by: Miłosz Malczak <[email protected]>

Add argeditor to master layout when recomputing arguments #3

01e18d4

Signed-off-by: Miłosz Malczak <[email protected]>

mmalczak force-pushed the fold_experiments_qt6 branch from 94fd1de to 01e18d4 Compare April 16, 2025 21:36

SimonRenblad suggested changes Apr 17, 2025

View reviewed changes

SimonRenblad reviewed Apr 17, 2025

View reviewed changes

SimonRenblad mentioned this pull request Apr 22, 2025

Refactor widgets creation, add smaller methods #2730

Open

4 tasks

		def __init__(self, manager, expurl):
		QtWidgets.QMdiSubWindow.__init__(self)

Fold experiments qt6 #2724

Are you sure you want to change the base?

Fold experiments qt6 #2724

Uh oh!

Conversation

mmalczak commented Apr 16, 2025

ARTIQ Pull Request

Description of Changes

Type of Changes

All Pull Requests

Code Changes

Git Logistics

Licensing

Uh oh!

SimonRenblad commented Apr 17, 2025

Uh oh!

SimonRenblad left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimonRenblad Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marmeladapk commented Apr 17, 2025

Uh oh!

SimonRenblad commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmalczak commented Apr 18, 2025

Uh oh!

SimonRenblad commented Apr 22, 2025

Uh oh!

marmeladapk commented Apr 22, 2025

Uh oh!

SimonRenblad commented Apr 23, 2025

Uh oh!

dnadlinger commented Apr 23, 2025

Uh oh!

Uh oh!

SimonRenblad left a comment •

edited

Loading

SimonRenblad Apr 17, 2025 •

edited

Loading

SimonRenblad commented Apr 18, 2025 •

edited

Loading