Skip to content
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

PR: Improve UI/UX of the Run plugin configuration widgets #22141

Merged
merged 57 commits into from
Jun 18, 2024

Conversation

ccordoba12
Copy link
Member

@ccordoba12 ccordoba12 commented Jun 3, 2024

Description of Changes

  • The new architecture for the Run plugin (added in PR: Generalize Run plugin to support generic inputs and executors #17467) is quite powerful and flexible. However, it's not easy to understand how to make use of it.
  • This PR aims to improve that by
    • Adding default execution parameters to our config system. That will show them in the Preferences page for Run, which will allow users to easily modify them. In addition, that page won't appear empty, which is a bit odd.
    • Simplifying significantly the UI of the dialog that appears when you go to Run > Open run settings. In addition, that dialog was modified so that it allows to only set run settings per file (just as in Spyder 5). That makes it simpler to understand and leaves creating global execution configs to the Run entry in Preferences.
  • Add a new CollapsibleWidget widget, which is based on QCollpasible from SuperQt, and use it in RunDialog. That required increasing our minimal constraint on SuperQt to >=0.6.2.

Visual changes

Configuration per file

  • Before

    imagen

  • After

    imagen

Preferences page

  • Before

    imagen

  • After

    imagen

Dialog to create global run configurations

  • Before

    imagen

  • After

    imagen

Issue(s) Resolved

Fixes #21878
Fixes #20700
Fixes #20569

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: @ccordoba12

- Instead, show label with the current file name.
- That helps to decrease the complexity of this dialog.
This also helps to decrease the complexity of that dialog.
- The name of that config is the "Custom" string localized.
- If the config is not named differently, additional customizations will
also be saved in "Custom".
- Also, only show global configs (i.e. those created in Preferences) or
the ones that correspond to the file currently displayed in RunDialog.
- For the dialog:
  * Move configuration name to the beginning.
  * Simplify layout of extension and context comboboxes.
  * Show localized executor name in dialog's title.
  * Prevent saving configs without a name.
- For the conf page:
  * Don't show configs set for files.
  * Code style improvements.
- Also, remove several unused constants.
Also, disable delete and clone buttons if there's no index selected in
RunParametersTableView and add a test for this new functionality.
- Also, improve the main label of its config page and fix the name of
one of its actions.
- This widget is based on superqt's QCollapsible widget.
- It has several style customizations on top to match our style.
…ialog

- Also, move QLineEdit to name the config to the top and show it inside
a QGroupBox.
- And move extra spacing between QGroupBoxes to the style of the
Preferences dialog.
- IPython console group: Remove grid layout to give more space to
introduce command line options.
- External terminal: Change text of of several widgets to make options
easier to understand.
- Profiler: Minor inheritance problems.
…leWidget

- We use PointingHandCursor for that, which will better indicate to
users that the button is clickable.
- That requires a more recent version of superqt, which exposes that
button publicly.
- Correctly display the entries of its comboboxes now that they are
instances of SpyderComboBox.
- Fix error in Python 3.10+ when centering RunDialog.
- Fix error in its config page when an executor doesn't have parameters
for specific files.
This is done to follow the same convention present in the Profiler and
External Terminal plugins.
This will give more space to users to enter them.
- Remove extra margins added by the config dialog style that this dialog
belongs to.
- Make the dialog fit exactly to the size of the widgets inside it.
- Increase inner padding.
- Remove unnecessary widget that acted as container for the others.
- Reorganize code for clarity.
The previous solution was not correct because it cropped the title of
those QGroupBoxes for some font types.
Those extensions are correctly added by other plugins.
- That way those configurations will be displayed in the Run confpage,
which will allow users to easily check what are the default parameters
for each one.
- Also, fix showing the last selected parameters for the file after
opening the dialog.
- Simplify text for its opening label and buttons.
- Add label to the left of the executor combobox to make clearer what
it's about.
- Don't show parameters table inside a QGroupBox to decrease its
importance (they depend on the executor).
- Add left and right horizontal margins to the parameters table to
improve the table's left alignment.
- Show parameters name in the first column, then extension and finally
context.
- Reorganize parameters so that Python and IPython extensions are shown
first and second by default.
- Simplify column names.
- If parameters are default, show a localized version of "Default"
instead of its actual name. That's useful in case users switch between
select different languages in the interface.
That way it'll be easier for users to understand that they can edit even
the default configurations.
- Use localized version of "Default" string instead of the params name.
- Don't allow to change the params name.
@ccordoba12 ccordoba12 force-pushed the improve-run-config-ux branch from 81744d7 to b193699 Compare June 10, 2024 00:08
@ccordoba12
Copy link
Member Author

ccordoba12 commented Jun 10, 2024

To @dalthviz:

Information tooltip refering to the Ok button at first glance make me think that it was refering to the Save globally button instead of the OK button at the bottom of the dialog itself. Not sure if it was discussed before, but now that I'm trying the dialog is kind of strange to have buttons in different places of the dialog

We discussed this with @conradolandia and agree with you. That's why I removed "Save globally" and moved the Delete button to the dialog button box at the bottom.

To help users understand where to set global configurations, I added a new entry called like that to the Run menu:

imagen

Also, the Delete button refers to a global delete of the config right?

No, it refers to deleting file configurations only. To clarify that, I changed the behavior of that button so that it's disabled for default or global configurations.

Being able to trigger a file run config delete when I haven't save anything yet (even a confirmation dialog appears and then the run dialog gets closed)

It should be fixed now by the change I mentioned above.

Triggering a Save globally without entering a name/leaving the default one triggers a confirmation dialog before showing and red circle x over the config name lineedit

It's no longer a problem.

Trying to save a new global config without a name changes the lineedit placeholder to a message indicating a name is needed. Maybe to make things more clear putting the red circle x over the lineedit

That's a great suggestion to make things consistent. I implemented it in one of my last commits.

Seems like trying to modify a global config from the run config dialog ends up deleting the global config?

It shouldn't be a problem anymore.

A traceback when using Reset changes and Edit selected buttons on the Run preference page

Thanks for noticing it. It should be fixed now.


To @jitseniesen:

Just looking at the pictures, I would remove the heading "Runner settings" in the dialog windows

Great suggestion! I did that in one of my last commits (see updated screenshots in the OP).

@ccordoba12
Copy link
Member Author

In addition to fixing the problems found in review, I took the opportunity to improve the design of the Run page in Preferences (see the updated OP).

@dalthviz
Copy link
Member

Gave another check and I think the difference between the local vs global configs is clearer now 👍 However, there are a couple of things that seem like bugs/I'm still not sure about:

  • Cloning/duplicationg a global config and then removing the selected config removes all the created global configs:
    run_21

  • Create a new local config, open the dialog again, try to modify the config without changing the name lineedit. The circular red x appears. Following that, try to set as preset configuration the default from the combobox (or use the reset button), still unable to close the dialog via the Ok button

run_22

  • Open the run dialog and choose a different preset or even just open and try to close it via the Ok button the dialog will be kept open due to the name:

run_23

So my guess is that it is not possible to override an existing local config? In fact, as things are right now, is not possible to select one of the already created configs/presets without creating a new config, right? If that is the case, probably more feedback about the error needs to be provided. Some ideas:

  • Add some sort of message somewhere indicating that a new name is needed
  • When showing the collapsible section, remove the text along side showing the circle red x and move focus to the line edit to point people not only that something is invalid but that they need to take action to overcome it

From the checks I have been doing, I would say that from the run dialog it would be nice to support the following operations:

  • Select a config from the presets and use it without having to do any modification (without having to deal with the Custom configuration section)
  • Be able to select a preset and change a specific config without having to create a new one
  • Be able to select a preset and use it as a base for a new config with a different name

Not sure how much of the these things are feasible/fall under the scope you had for this PR/the general functionality of the run dialog @ccordoba12 but sharing the ideas that came to mind while checking this

- Show error status icon when name is empty or already taken.
- Disable changing the config name if it's a saved one.
- Show the same messages in both ExecutionParametersDialog and
RunDialog.
- Show current text on their tooltips and use icons instead.
- That allows us to organize the buttons in a horizontal layout, which
is more compact and lean.
- For this it was useful to extend the create_button method of
SpyderConfigPage to simplify the creation of buttons in confpages.
- Also, fix extra padding in tooltips of SidebarDialog.
This avoids an additional level of nesting that probably doesn't make
much sense to users (as suggested in review).
This will allow users to easily understand where they need to go to set
global configurations.
That allows to correctly delete confs that haven't been saved yet.
@ccordoba12 ccordoba12 force-pushed the improve-run-config-ux branch 2 times, most recently from e8322fb to 08ffabc Compare June 11, 2024 02:38
@ccordoba12
Copy link
Member Author

ccordoba12 commented Jun 11, 2024

Thanks @dalthviz for the additional review! About your comments:

Cloning/duplicationg a global config and then removing the selected config removes all the created global configs

Good catch! It should be fixed now.

  • Create a new local config, open the dialog again, try to modify the config without changing the name lineedit. The circular red x appears.
  • Open the run dialog and choose a different preset or even just open and try to close it via the Ok button the dialog will be kept open due to the name
  • So my guess is that it is not possible to override an existing local config?

These problems were an oversight on my side. The thing is I added a validation so that users can't introduce repeated config names in the run dialog. But I forgot to skip it when the config name and the one shown in the preset combobox are the same. So, please test again.

Select a config from the presets and use it without having to do any modification (without having to deal with the Custom configuration section)

This should be fixed now.

Be able to select a preset and change a specific config without having to create a new one

This is not possible anymore, i.e. with the changes done here. I think it was before, but then it gave users the odd impression that settings were not saved (see issue #20700, which was reported several times).

However, to not force users to provide a name when they customize global parameters, @conradolandia and I decided it'd be a good idea to set Default (custom) as the config name in that case. If users generate a custom config and then go back to Default to change it again, we set Default (custom 1) as the configuration name, and so on.

Be able to select a preset and use it as a base for a new config with a different name

This should be possible now. Please check again.

- To do that we automatically set a custom name for global configs. That
way users will be able to customize those configs as many times as they
want.
- Don't run config name validations for global configs if they haven't
been customized.
@ccordoba12 ccordoba12 force-pushed the improve-run-config-ux branch from 08ffabc to 353e4bd Compare June 14, 2024 17:47
@dalthviz
Copy link
Member

Gave this another check and seems like things are now working as expected. Will give the code another check but for the most part I would say that this looks good to me 👍

Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thanks @ccordoba12 ! Left a couple of questions/comments but this LGTM 👍

Also, on a last manual check, I noticed that seems like you can create a global config with the same name of an already create per file config:

run_config

Not sure if this could be thought as a bug or not but letting you know. I'm approving this so feel free to merge if you think is ready @ccordoba12

spyder/app/tests/test_mainwindow.py Outdated Show resolved Hide resolved
spyder/app/utils.py Show resolved Hide resolved
# height or position) that other widgets can need to position relative to
# it.
# * **DO NOT** use it to access other plugins functionality through it.
app.main_window = main
Copy link
Member

Choose a reason for hiding this comment

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

Rather than giving access to the whole main window shouldn't be created specific accessors? So, leaving this as app._main_window = main and creating a set of functions to get the values needed so get_mainwindow_width, get_mainwindow_height, get_mainwindow_position, etc?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a very good idea! Thanks @dalthviz for the suggestion 👍🏽

I'll implement it before merging.

@@ -643,7 +643,6 @@
('run', [
'breakpoints',
'configurations',
'defaultconfiguration',
Copy link
Member

Choose a reason for hiding this comment

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

There is no need to bump the config version, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

This config option wasn't used anywhere. That's why I didn't bump CONF_VERSION.

spyder/plugins/run/container.py Outdated Show resolved Hide resolved
Co-authored-by: Daniel Althviz Moré <16781833+dalthviz@users.noreply.github.com>
@ccordoba12
Copy link
Member Author

ccordoba12 commented Jun 17, 2024

Also, on a last manual check, I noticed that seems like you can create a global config with the same name of an already create per file config

Yep, that's correct. Since global configs take precedence over file ones, I think that validation is not appropriate and so I didn't implement it. Furthermore, it could be too restrictive for users given that they can create as many file configs as they want for as many files as possible.

The good thing is that there is a subtle clue in RunDialog about which configs are global and which ones belong to the file: the Delete button. If it's enabled, it means we're dealing with a file config; if not, it means the config is global.

If users let us know that's not enough, we can think about how to disambiguate them. But for now, I think that's ok.

Also, use those accessors in RunDialog to center it when uncollapsing
the custom config widget.
@ccordoba12
Copy link
Member Author

One last note: this PR also contains a couple of small fixes to our tests to deal with the Numpy 2.0 release (which is causing the failures in master).

@ccordoba12 ccordoba12 merged commit 14f8853 into spyder-ide:master Jun 18, 2024
23 checks passed
@ccordoba12 ccordoba12 deleted the improve-run-config-ux branch June 18, 2024 16:30
@Matthew-Copping
Copy link

After modifying the default behaviour and restarting spyder the original default configuration reappears along side my changed version. As a result my changes are not used.

Is this expected behaviour? If so how do i change the default behaviour?

image

@ccordoba12
Copy link
Member Author

Is this expected behaviour? If so how do i change the default behaviour?

No, it's not. Please open a new issue about it and add to it a small video showing what you did to better understand your problem.

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Dec 23, 2024

(Converting the OP before/after into a table for easy comparison purposes)

Configuration per file

Before After
imagen imagen

Preferences page

Before After
imagen imagen

Global run configuration

Before After
imagen imagen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants