-
-
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: Improve cells support in the Editor #20509
Conversation
Hi @dalthviz |
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 @jsbautista for working on this! Checked locally and seems like is working correctly 👍 Left some initial comments to remove some imports that seem like unused, some code style issues, suggestions for the new action icon and placement.
However, seems like the tests are failing due to a missing shortcut definition. For that you will need to define a shortcut at
Lines 416 to 417 in 450e50d
# ---- Editor ---- | |
# -- In widgets/sourcecode/codeeditor.py |
Ctrl + 2
could work for the moment).
Also could you add a GIF to preview the functionality here?
Thanks!
Co-authored-by: Daniel Althviz Moré <d.althviz10@uniandes.edu.co>
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.
Hi again @jsbautista , left a couple of new comments/suggestions but other than that this LGTM 👍
I think the failing test job is unrelated to the changes here but lets check if it fails again after applying the changes for the new review
Co-authored-by: Daniel Althviz Moré <d.althviz10@uniandes.edu.co>
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 for your work on this @jsbautista! It's really nice improvement!
Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
@jsbautista, I think you need to merge with |
One last thing to consider, but perhaps for a follow-up PR: should we try to move the code that paints cell dividers on top of panels to the paintEvent method of the base Panel class? @jsbautista, @dalthviz, I don't know if you considered that before, but it could be worth the try, don't you think? |
I think that could be a good idea. In that way if more panels are added then they will automatically add the cell line 👍 Probably we will need to validate |
Yep, that was my reasoning too.
I'd say |
Co-authored-by: Daniel Althviz Moré <d.althviz10@uniandes.edu.co>
@dalthviz, @jsbautista, I think this is ready to be merged, unless you prefer to make the changes to paintEvent we talked about here too. |
@jsbautista will working on the refactor mentioned above here 👍 |
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 for the refactoring @jsbautista ! Left some suggestions and a couple of comments about where the new paint_cell
method is being called. Other possibility is to add to the method the option to just run the logic for a given block (otherwise seems to me that we will be calling a for
inside a for
with the current usage in some of the panels).
If you have any question about the feedback let me know!
Co-authored-by: Daniel Althviz Moré <d.althviz10@uniandes.edu.co>
Note: this needs a merge with master for the tests to pass |
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 @jsbautista for your work on this! I left some additional comments for you.
Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
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.
Looks good to me now, thanks @jsbautista!
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 for all the work here @jsbautista !
Description of Changes
Issue(s) Resolved
Part of spyder-ide/ux-improvements#30
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:
@jsbautista