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: Refactor the toolbar handling on main widgets #13600

Merged
merged 2 commits into from
Sep 1, 2020

Conversation

goanpeca
Copy link
Member

@goanpeca goanpeca commented Aug 21, 2020

Description of Changes

  • Wrote at least one-line docstrings (for any new functions)
  • Added unit test(s) covering the changes (if testable)
  • Included a screenshot or animation (if affecting the UI, see Licecap)

Issue(s) Resolved

  • The code was simplified so instead of using a QMainWindow and QToolBars added to the ToolBar Areas, we use a QWidget and QToolBars added to QBoxLayouts. This way we have full control of the location, and stretches which fix the issue.
  • The API itself remained unchanged, the changes were internal to the PluginMainWidget.
  • This is how Help and Find in Files look (which also had the similar issue found in Pylint)
  • Included an extra fix on the find in files button, to make it have the same size, regardless of Search/Stop text.
  • The spinner widget is "permanently visible" so that it takes width and there are no jumps when it starts spinning (and is actually displayed).
  • Added a fix for find in files to keep the search combobox and exclude combobox the same size.

Screen Shot 2020-08-21 at 20 10 14

Help

Screen Shot 2020-08-21 at 20 03 16

Find

Screen Shot 2020-08-21 at 20 03 23

Screen Shot 2020-08-21 at 20 03 29

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: @goanpeca

@pep8speaks
Copy link

pep8speaks commented Aug 21, 2020

Hello @goanpeca! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-09-01 19:36:48 UTC

@goanpeca goanpeca self-assigned this Aug 21, 2020
@goanpeca goanpeca added this to the v5.0alpha2 milestone Aug 21, 2020
@goanpeca goanpeca force-pushed the fix/mainwidget-toolbar branch 3 times, most recently from 639c0f3 to 63bc230 Compare August 22, 2020 00:50
@goanpeca goanpeca marked this pull request as ready for review August 22, 2020 00:50
@goanpeca goanpeca mentioned this pull request Aug 22, 2020
3 tasks
@dalthviz
Copy link
Member

/show binder

@github-actions
Copy link

Binder 👈 Launch a Binder instance on this branch

@dalthviz
Copy link
Member

@ccordoba12 could you create a list of the panes we should be checking please? Thanks :)

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 @goanpeca 👍 I left some comments/questions. The idea will be now to manually test this in the different OS and once everything looks good we could merge it and rebase the other PRs to apply the changes

spyder/api/plugins.py Outdated Show resolved Hide resolved
spyder/api/plugins.py Outdated Show resolved Hide resolved
spyder/widgets/waitingspinner.py Show resolved Hide resolved
@ccordoba12
Copy link
Member

could you create a list of the panes we should be checking please? Thanks :)

Plugins that were already moved to the new API and could be affected due to this are:

  • History
  • Internal Console
  • Find
  • Profiler
  • Help
  • Online help
  • Breakpoints
  • Plots

@dalthviz
Copy link
Member

Checking on Windows seems like, except for the History pane, everything looks good:

toolbar

I'm unsure but seems like the History toolbar looks a little bit small than the rest?

History

image

Internal Console

image

@goanpeca
Copy link
Member Author

Yeah, the history one looks funny. Will see what is going on.

@dalthviz
Copy link
Member

@andfoy @steff456 @juanis2112 @ccordoba12 could you guys help here testing this? Basically the idea is to check the resize of the window, the style of the toolbars of each pane, and the overall style of the actions in the toolbars in linux and mac

@steff456
Copy link
Member

steff456 commented Aug 25, 2020

Hi! I tested the following plugins in macOS

  1. History
  2. Internal Console
  3. Find
  4. Profiler

And they all resize well and the style of the toolbars also match with 4.x branch

@goanpeca
Copy link
Member Author

goanpeca commented Aug 28, 2020

I have to work on:

  • the history button fix and add
  • option to enable a spinner as discussed with

As discussed with @dalthviz

@goanpeca
Copy link
Member Author

goanpeca commented Aug 30, 2020

Hi @dalthviz,

Added the fixes we talked about. Making some comments on the files section.

  • Only find in files has the new ENABLE_SPINNER attribute set to True
  • Help Pane does not include the extra space anymore
  • Now all MainPluginwidgets plugins, (those using Tabs and does not using them) align the "Hamburguer menu" (At least on OSX)

test

@goanpeca goanpeca requested a review from dalthviz August 30, 2020 17:00
spyder/widgets/tabs.py Outdated Show resolved Hide resolved
@dalthviz
Copy link
Member

Thanks @goanpeca , I left some comments and questions regarding the changes and some specific values for the history layout that worked for me on Windows (some tweaks to the values needed to handle a tabwidget as part of the toolbar and since for the different OS the values could change if I'm understanding correctly, right?)

@goanpeca
Copy link
Member Author

Thanks for the feedback @dalthviz. Added the suggested fixes and a question to one of the suggestions.

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.

@goanpeca left some more comments and also I was thinking that maybe we should be using the spinner for other plugins like the Online Help pane and the Help pane

spyder/api/plugins.py Show resolved Hide resolved
spyder/api/widgets/__init__.py Outdated Show resolved Hide resolved
spyder/widgets/tabs.py Outdated Show resolved Hide resolved
@goanpeca
Copy link
Member Author

goanpeca commented Sep 1, 2020

@goanpeca left some more comments

Left some more answers.

and also I was thinking that maybe we should be using the spinner for other plugins like the Online Help pane and the Help pane

Made the fixes @dalthviz.

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.

@goanpeca left a couple of comments and seems like some PEP8 issues need a fix. Besides that this LGTM 👍

@andfoy @steff456 could you test this on Linux and Mac please? Thanks!

spyder/widgets/tabs.py Outdated Show resolved Hide resolved
spyder/widgets/tabs.py Outdated Show resolved Hide resolved
@goanpeca
Copy link
Member Author

goanpeca commented Sep 1, 2020

@goanpeca left a couple of comments and seems like some PEP8 issues need a fix. Besides that this LGTM 👍

Thanks for the review @dalthviz.

I made the requested fixes.

@dalthviz
Copy link
Member

dalthviz commented Sep 1, 2020

/show binder

@github-actions
Copy link

github-actions bot commented Sep 1, 2020

Binder 👈 Launch a Binder instance on this branch

@dalthviz
Copy link
Member

dalthviz commented Sep 1, 2020

Last comment @goanpeca , please enable the spinner for the Profiler (I almost missed that pane), thanks!

@goanpeca
Copy link
Member Author

goanpeca commented Sep 1, 2020

Last comment @goanpeca , please enable the spinner for the Profiler (I almost missed that pane), thanks!

Fixed @dalthviz.

@steff456
Copy link
Member

steff456 commented Sep 1, 2020

/show binder

@github-actions
Copy link

github-actions bot commented Sep 1, 2020

Binder 👈 Launch a Binder instance on this branch

@steff456
Copy link
Member

steff456 commented Sep 1, 2020

@dalthviz we already checked this PR and it is ready for merge!

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 @goanpeca LGTM 👍

@dalthviz dalthviz merged commit 61a505e into spyder-ide:master Sep 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants