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

[bugfix][hidpi] Scalable SVG theme cursors #5737

Merged
merged 5 commits into from
Dec 1, 2017
Merged

Conversation

elpaso
Copy link
Contributor

@elpaso elpaso commented Nov 27, 2017

Add support for SVG cursors:

  ZoomIn,
  ZoomOut,
  Identify,
  CrossHair,
  CapturePoint,
  Select,
  Sampler,

Icons are provisional: they need some love from
a decent graphics designer.

Fixes #12671

Add support for SVG cursor:

      ZoomIn,
      ZoomOut,
      Identify,
      CrossHair,
      CapturePoint,
      Select,
      Sampler,

Icons are provisional: they need some love from
a decent graphics designer.

Fixes qgis#12671
This should work well on different DPI screens.

Still needs some testing on the various platforms.
@nyalldawson
Copy link
Collaborator

This is a big improvement on my hi dpi screen! I think we should merge and then update the cursor graphics.

Oddly, I've noticed that the Qt OpenHandCursor/ClosedHandCursor cursors are tiny too, even though other Qt cursors work fine. Maybe we should provide svg versions of these too?

@elpaso
Copy link
Contributor Author

elpaso commented Nov 28, 2017

@SrNetoChan what do you think? Do you have some better icons yet?

@elpaso
Copy link
Contributor Author

elpaso commented Nov 28, 2017

@SrNetoChan I've added your icons, they look good!!

@elpaso
Copy link
Contributor Author

elpaso commented Nov 28, 2017

@nyalldawson the "hand" icons on KDE are perfect but I guess that with cursors the look and feel actually changes from one platform to another, from one version to another and from one desktop manager to another 😞
It might make sense to provide our own cursors for the "hands" icons too.

@SrNetoChan
Copy link
Member

@elpaso I am currently checking how they look in "non-hidpi" screens. In my manual tests, the halo looks a bit pixelated because of pixmap interpolation. But I was using 24x24 size.

@SrNetoChan
Copy link
Member

Here's the all cursors. Please overwrite the mSampler.svg and mIdentify.svg as well because I have run svg_cleaner on them.

cursors.zip

Regarding the active points (assuming 32x32):

ZoomIn (13,13)
ZoomOut (13,13)
Identify (3,6)
CrossHair (16,16)
CapturePoint (16,16)
Select (6,6)
Sampler (5,5)

I can confirm that in non-hidpi, the cursors looks very small and pixelated. could we bump them to 24x24? The halo still looks a bit strange so I have avoided it as much as I could.

Cursors are now approximately 24x24 pixels on 96dpi
@elpaso
Copy link
Contributor Author

elpaso commented Nov 28, 2017

@SrNetoChan it's getting better and better. Let me know how it looks like at 96dpi
Any win/mac user out there that can test this?

@SrNetoChan
Copy link
Member

Definitely much better now on non-hidpi. On hidpi screens can you notice that the sampler allows to see the color that it is picking?

@SrNetoChan
Copy link
Member

SrNetoChan commented Nov 29, 2017

BTW, there is still a issue with the sampler cursor when using the color picker for the layer styling dock. It is replaced by the default arrow as soon as you move the mouse. This doesn't happen using the color picker from the color selection dialog.

@elpaso
Copy link
Contributor Author

elpaso commented Nov 29, 2017

@SrNetoChan for what it worths the sampler from the styling dock is already broken in current master.

@elpaso
Copy link
Contributor Author

elpaso commented Nov 29, 2017

@SrNetoChan I couldn't fix the issue with the styling dock but at least it was not introduced by this PR so I don't see it a good reason for not merging.

(btw, when the window is not maximized and the menu drops out of the main window the issue is not there).

@SrNetoChan
Copy link
Member

(btw, when the window is not maximized and the menu drops out of the main window the issue is not there).

@elpaso interesting. I will see if there is a bug report about that. Otherwise I will create one.

@SrNetoChan
Copy link
Member

@elpaso FYI

https://issues.qgis.org/issues/17597

This is probably something for @NathanW2 or @nyalldawson

@elpaso elpaso merged commit 62ff88a into qgis:master Dec 1, 2017
@elpaso
Copy link
Contributor Author

elpaso commented Dec 1, 2017

Let's give this a try. We need some feed-back from different platforms/DMs/screen sizes.

@elpaso
Copy link
Contributor Author

elpaso commented Dec 12, 2017

@m-kuhn they already are scalable (and scaled), we could either have a better logic to define the size or make it user-configurable.

Besides, I've found no way to get the cursor size from the O.S., that would be my preferred way to get the right size.

Unfortunately the cursors size depends not only on the dpi but also on the platform, the desktop manager and the cursor theme one is using.

See the logic here: https://github.com/qgis/QGIS/pull/5737/files#diff-e46a31dc04eb1c0d7bf46ed726e5e6f3R539

I see I forgot to apply Qgis::UI_SCALE_FACTOR as I did in b6dd62f#diff-9af5c6958b37df7f10a4b3e26bffccc5R73

It would be useful if you could check if appying the factor (on win) does a better job.

@m-kuhn
Copy link
Member

m-kuhn commented Dec 12, 2017

I do not have a windows system but if you can tell me what I can fine-tune, I'll be happy to check

@elpaso
Copy link
Contributor Author

elpaso commented Dec 12, 2017

@m-kuhn (and @rduivenvoorde, @SrNetoChan)

As @nyalldawson explained to me, on windows the fontMetrics should be corrected by multiplying the value with Qgis::UI_SCALE_FACTOR, you could start by changing line https://github.com/qgis/QGIS/pull/5737/files#diff-e46a31dc04eb1c0d7bf46ed726e5e6f3R540 and mulitply the fontMetrics().height() by the scale factor.
As an alternative, you could try to reduce the 1.5 factor (but that would also make the cursors smaller on hidpi).

A quick test in python to see what looks like on different platforms is:

icon = QgsApplication.getThemeIcon('cursors/mCapturePoint.svg')
scale = QgsApplication.fontMetrics().height() * Qgis.UI_SCALE_FACTOR / 32 * 1.5
import math
cursor = QCursor( icon.pixmap( math.ceil( scale * 32 ), math.ceil( scale * 32 ) ), 0, 0)
QgsApplication.setOverrideCursor(cursor)

# later on:
QgsApplication.restoreOverrideCursor()

@elpaso elpaso deleted the hidpi-cursors branch December 12, 2017 16:12
@m-kuhn
Copy link
Member

m-kuhn commented Dec 12, 2017

Without the * 1.5 it looks perfect for me, cc @wonder-sk ?

@elpaso
Copy link
Contributor Author

elpaso commented Dec 12, 2017

@dakcarto how does that look like on (retina?) Mac?

@elpaso
Copy link
Contributor Author

elpaso commented Dec 12, 2017

@m-kuhn without the 1.5 it looks 1.5 times smaller than the standard cursor theme on my KDE 😄
I think we should make it configurable ...

@m-kuhn
Copy link
Member

m-kuhn commented Dec 12, 2017

Let's wait for the feedbacks and go for a simple KDE switch in case that's adequate ;)

@m-kuhn
Copy link
Member

m-kuhn commented Dec 12, 2017

Actually... didn't KDE always ship with oversized cursors?

@wonder-sk
Copy link
Member

I can confirm without the 1.5 multiplier the cursors look reasonably on my hidpi screen. By the way, I am using KDE.

Maybe the cursor size should not be derived from font size, but rather from "normal" cursor size somehow?

@rduivenvoorde
Copy link
Contributor

On Gnome/Debian NON-hidp,i I like the cursors best without the 1.5

The current normal cursor is a little too heavy to me currently.

Something else I see: the normal (Gnome)-cursors have a little white halo around it, which makes it a little less 'flat'. Is that an idea? Or are those 'old-fashioned' or so?

@elpaso
Copy link
Contributor Author

elpaso commented Dec 12, 2017

@wonder-sk I tried but I didn't find a way to get the size of the O.S. cursor... do you have any idea?
@rduivenvoorde the cursors that @SrNetoChan prepared do have a white halo to make them visible on a dark background, is that what you mean?

If do everybody agree we can add a user option for the multiplier (something like "tiny", "small", "normal", "big", "huge"), where 1 is "normal" and range from 0.5 to 2

@rduivenvoorde
Copy link
Contributor

rduivenvoorde commented Dec 12, 2017

@elpaso are you sure? This is what I have now:
selection_243
and the same one without the * 1.5
selection_245
(all the same scale)
compared with the old one:
selection_244

@elpaso
Copy link
Contributor Author

elpaso commented Dec 12, 2017

@rduivenvoorde the zoom-in/out has a white interior instead of the white exterior halo (same effect of being visible on a dark bg), other cursors that do not have an "interior" have the white exterior (halo), but @SrNetoChan knows better than me.

@SrNetoChan
Copy link
Member

SrNetoChan commented Dec 13, 2017

@rduivenvoorde I tried to avoid the white halo as much as I could. So I only used it in completely black cursors. The problem with that, is that for resizing the SVGs and converting it to pixels there is some kind of interpolation, that transforms the pure white in gray variations and it does not look very sharp in the end. I can remove the halo, but if you move your cursor over a full black background, it will disappear.

@rduivenvoorde
Copy link
Contributor

@SrNetoChan ah, thanks. Sorry for my critics then, I see there is already a lot of thoughts and time in it...
Thanks for doing this Alexandre!

@SrNetoChan
Copy link
Member

There might be another way of doin

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.

6 participants