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: Make the width of thumbnails scrollbar resizable #9720

Merged
merged 27 commits into from
Sep 10, 2019

Conversation

jnsebgosselin
Copy link
Member

@jnsebgosselin jnsebgosselin commented Jun 29, 2019

Issue(s) Resolved

Fixes item 2 of #9367
Fixes #9747

thumbnail_scrollbar

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: Jean-Sébastien Gosselin

@jnsebgosselin
Copy link
Member Author

This is not ready for review yet @goanpeca, there is something I still need to figure out with the dark style.

@jnsebgosselin
Copy link
Member Author

Ok @goanpeca, I've added some workaround to handle some padding and margin values that are hard coded in the qdarkstyle.

I haven't been able to get rid or figure out exactly where the extra 2px * 2 comes from in the dark style... if you can find a better solution, I'm all hears.

@goanpeca
Copy link
Member

goanpeca commented Jun 30, 2019

Will take a look

goanpeca
goanpeca previously approved these changes Jul 3, 2019
@jnsebgosselin
Copy link
Member Author

Also it would be nice to have a centralized place for all these hardcoded nitpicks we will probably still need with QDark to keep things in order. Any thoughts there?

It is a good point, but in that case, since I do not really know where this padding comes from, I don't know how useful that would be.

It is just a thought, but I don't understand why there is a need to have hardcoded margin and padding in qdarkstyle, in addition to the default one. I mean, if an application is tailored to use qdarkstyle and qdarkstyle only, there is no problem, but in a scenario where we have to support multiple styles, this makes things a lot more complicated...

@jnsebgosselin
Copy link
Member Author

And @goanpeca thanks for the review btw!

@ccordoba12 ccordoba12 changed the title PR: Set the width of the thumbnails scrollbar in the Plot plugin from a calculated value instead of a hard coded value. PR: Set width of thumbnails scrollbar in the Plots plugin from a calculated value Jul 4, 2019
@spyder-ide spyder-ide deleted a comment from jnsebgosselin Jul 4, 2019
@goanpeca
Copy link
Member

goanpeca commented Jul 4, 2019

@jnsebgosselin I am fie with this PR how it is :-), anything extra you wanted to do?

@jnsebgosselin
Copy link
Member Author

@jnsebgosselin I am fie with this PR how it is :-), anything extra you wanted to do?

Yes, if the third option listed in #9747 is selected , I would like to implement it here.

@ccordoba12 ccordoba12 dismissed goanpeca’s stale review July 5, 2019 09:34

Reimplement scrollbar show/hide behavior

@ccordoba12
Copy link
Member

@goanpeca, I dismissed your review to not merge this one by accident.

goanpeca
goanpeca previously approved these changes Aug 29, 2019
@jnsebgosselin
Copy link
Member Author

Couple of questions :-p, otherwise looks good!

Awesome, I will answer them later tonight and thank you.

@goanpeca
Copy link
Member

Thanks for working on this 😄

@goanpeca
Copy link
Member

@jnsebgosselin on OSX there are the following issues:

  • For HDPI screens (like my mac) the images looks pixelated
  • The buttons on the thumbnails to the right are cropped

plots

@jnsebgosselin
Copy link
Member Author

Thanks you very much for the tests.

* For HDPI screens (like my mac) the images looks pixelated

The plots plugin generates the image from the data it receives from the console. The images look pixelated because the pngs sent by the console have a low resolution. There is not much that can be done here, I mean from the plot viewer perspective.

Does changing the resolution of the inline backend helps in the console preferences?

* The buttons on the thumbnails to the right are cropped

Ok, I do not see another solution than to increase the hard-coded padding in _calculate_figure_canvas_width. How much more padding do you think would be required?

@goanpeca
Copy link
Member

Ok, I do not see another solution than to increase the hard-coded padding in _calculate_figure_canvas_width. How much more padding do you think would be required?

Let me push a PR so we gte the right value for OSX :-)

The plots plugin generates the image from the data it receives from the console. The images look pixelated because the pngs sent by the console have a low resolution. There is not much that can be done here, I mean from the plot viewer perspective.

Does changing the resolution of the inline backend helps in the console preferences?

Yes, this is for another PR (I think I tried this before and I got bigger images... but still pixelated)

@jnsebgosselin
Copy link
Member Author

Let me push a PR so we gte the right value for OSX :-)

Ok thanks. Is this cropped in the light theme also or only in the dark one?

@goanpeca
Copy link
Member

goanpeca commented Sep 4, 2019

@jnsebgosselin there must be something missing on the space calculation for OSX, the fixed 10 value, seems to work as expected and for now I think it is good enough 🤷‍♂ ?

Ok thanks. Is this cropped in the light theme also or only in the dark one?

It was the same on both themes, dark and light!

Let me know what you think!

@jnsebgosselin
Copy link
Member Author

@jnsebgosselin there must be something missing on the space calculation for OSX, the fixed 10 value, seems to work as expected and for now I think it is good enough 🤷‍♂ ?

Ok thanks. Is this cropped in the light theme also or only in the dark one?

It was the same on both themes, dark and light!

Let me know what you think!

Ok thank you @goanpeca for your help with this. I think it is good enough for now. I really don't know where these extra padding come from...

The question now, before we can finish this, is to know whether the same problem also appears in Linux...

@ccordoba12
Copy link
Member

The question now, before we can finish this, is to know whether the same problem also appears in Linux...

I tested on Linux and buttons are not cropped there, so this is only a macOS problem.

@jnsebgosselin, please address @goanpeca's last comments so we can merge this one because I think it's almost ready. Thanks!

jnsebgosselin and others added 3 commits September 10, 2019 09:21
Co-Authored-By: Gonzalo Peña-Castellanos <goanpeca@gmail.com>
Co-Authored-By: Gonzalo Peña-Castellanos <goanpeca@gmail.com>
@jnsebgosselin
Copy link
Member Author

The plots plugin generates the image from the data it receives from the console. The images look pixelated because the pngs sent by the console have a low resolution. There is not much that can be done here, I mean from the plot viewer perspective.

Does changing the resolution of the inline backend helps in the console preferences?

Yes, this is for another PR (I think I tried this before and I got bigger images... but still pixelated)

@goanpeca I think this should be true for the inline plots in the IPython console, but is it also the case for the plots plugin?

@ccordoba12
Copy link
Member

I think this should be true for the inline plots in the IPython console, but is it also the case for the plots plugin?

@jnsebgosselin, we need to tell the inline backend to generate retina display plots (there's an option for that in IPython). However, that's a tricky business now because users can move Spyder or the IPython console window (if detached) to a HiDPI monitor from a low resolution one and vice-versa.

So we have to detect when the window is moved to another monitor and its screen resolution to tell all kernels when to turn on/off retina display plots automatically.

@ccordoba12
Copy link
Member

@jnsebgosselin, for next time please add all code change suggestions to the batch and apply them as a single commit. Thanks!

@jnsebgosselin
Copy link
Member Author

@jnsebgosselin, for next time please add all code change suggestions to the batch and apply them as a single commit. Thanks!

This is done from the GitHub interface? OK I will do that next time, thanks for the tip.

@ccordoba12
Copy link
Member

This is done from the GitHub interface? OK I will do that next time, thanks for the tip.

Yeah, there's a button in each suggestion called Add suggestion to batch.

@jnsebgosselin
Copy link
Member Author

I think this should be true for the inline plots in the IPython console, but is it also the case for the plots plugin?

@jnsebgosselin, we need to tell the inline backend to generate retina display plots (there's an option for that in IPython). However, that's a tricky business now because users can move Spyder or the IPython console window (if detached) to a HiDPI monitor from a low resolution one and vice-versa.

So we have to detect when the window is moved to another monitor and its screen resolution to tell all kernels when to turn on/off retina display plots automatically.

Ok, but the plots plugin doesn't work exactly like the IPython console because it allow for the figures to be resized. So I thought that increasing the dpi of the inline figures would increase the quality of the images and thumbnails shown in the plots plugin and offer a temporary solution to this problem.

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this @jnsebgosselin! Pretty nice addition!

@ccordoba12 ccordoba12 merged commit 4a84d7c into spyder-ide:master Sep 10, 2019
@goanpeca
Copy link
Member

Awesome! thanks @jnsebgosselin :-)

@jnsebgosselin
Copy link
Member Author

Yeah, thanks a lot for your generous patience and help with this. This has proven to be more tricky than I anticipated...

@jnsebgosselin jnsebgosselin deleted the imp_thumbnail_size branch September 11, 2019 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Separator on plots plugin acts more like a button than a resize area
3 participants