-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: Restore buttons that control the debugger to the main toolbar #22702
base: master
Are you sure you want to change the base?
Conversation
@dalthviz, could you manually check that the following things are working for you? Thanks!
|
c6a7fab
to
d86d1c8
Compare
@dalthviz, in my last commit I also added some tests to cover the new functionality of the Toolbar plugin. |
d86d1c8
to
5aaaeab
Compare
That way it'll be easier to tell them apart.
- That's because not all toolbars are created with the create_application_toolbar method. - Also, remove repeated code between create and add app toolbars.
We'll need this to interate over the toolbars by id in the plugin
We were not correctly loading toolbars that are not visible.
- That restores the buttons available in Spyder 5, which was requested by users. - The toolbar is not visible by default to avoid showing repeated functionality in the Debugger plugin and main toolbar.
Also, fix some block comments
Also do that at first startup,
Instead, add all its buttons to the Debug toolbar
Those buttons will be visible only if there's a debugging session active in the current console.
5aaaeab
to
15f1a15
Compare
Thanks @dalthviz for the review!
This should be fixed now, so please check again. Also, I changed the implementation according to the UX team feedback. Now the buttons that control the debugger are shown in/hidden from the Debug toolbar when a debugging session is started/stopped (see the OP), or if the current console is in a debugging session or not. That avoids a serious issue with my previous approach: users would need to know that a new toolbar is available and enable it to get the Spyder 5 UX. In addition, this approach prevents cluttering the interface because the new buttons will be hidden when not needed. So, could you check if that's working as expected for you? |
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.
Thanks @ccordoba12 ! Gave a local check and seems like things are working as expected 👍 Left a couple comments with nitpicks and a question regarding the tests wait logic (taking into account that now there is a signal available to determine when a toolbar gets rendered; maybe the wait are there to give some time for the toolbars to be rendered/rerendered after some action is done to them?) .
Anyhow, leaving this approved so feel free to merge if you think is ready
|
||
# Move the debug toolbar to be before the file one | ||
toolbar.get_main().insertToolBar(file_toolbar, debug_toolbar) | ||
qtbot.wait(500) |
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.
Maybe for some of these qtbot.wait
calls over these new tests could be replaced for a qtbot.waitSignal
checking for the new toolbars sig_is_rendered
signal?
container.create_toolbars_menu() | ||
container.load_last_visible_toolbars() | ||
|
||
def on_close(self, _unused): | ||
container = self.get_container() | ||
|
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.
Should this blank be removed?
@@ -10,7 +10,7 @@ | |||
|
|||
# Standard library imports | |||
from collections import OrderedDict | |||
from spyder.utils.qthelpers import SpyderAction | |||
import logging |
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.
Shouldn't import logging
go first/before from collections import OrderedDict
too?
Description of Changes
SpyderToolbar
to inform when it's rendered. That's necessary to get the widgets corresponding to the control debugger actions in the Debug toolbar, so we can hide/show them.Visual changes
New buttons shown when a debugging session is started and hidden when it's stopped
Issue(s) Resolved
Fixes #22434
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: