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

fix blurry favicons on hidpi displays #3129

Merged
merged 1 commit into from
Oct 25, 2017

Conversation

foliea
Copy link
Contributor

@foliea foliea commented Oct 15, 2017

This fixes blurry favicons on hidpi displays.

Related to #1585

Results on non hidpi display:
2017-10-15-192153_2560x1080_scrot

Results on hidpi display:
2017-10-15-192249_3200x1800_scrot


This change is Reviewable

@The-Compiler
Copy link
Member

Thank you! It'll probably take me a bit until I get to properly reviewing this, as I was really busy with v1.0 and the attention it has received over the past few days.

FWIW the docs say:

Make QIcon::pixmap() generate high-dpi pixmaps that can be larger than the requested size. Such pixmaps will have devicePixelRatio() set to a value higher than 1. After setting this attribute, application code that uses pixmap sizes in layout geometry calculations should typically divide by devicePixelRatio() to get device-independent layout geometry.

The only place qutebrowser uses QIcon::pixmap is here in tabwidget.py:

        icon = opt.icon.pixmap(opt.iconSize, icon_mode, icon_state)
        p.drawPixmap(layouts.icon.x(), layouts.icon.y(), icon)

so this should be fine.

@jgkamat since you looked at tab sizing stuff recently, want to take a look?

@jgkamat jgkamat self-assigned this Oct 15, 2017
@jgkamat jgkamat self-requested a review October 15, 2017 18:04
@jgkamat jgkamat removed their assignment Oct 15, 2017
Copy link
Member

@jgkamat jgkamat left a comment

Choose a reason for hiding this comment

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

I took a look at the logic, and I don't think this will break anything. I don't have a HiDPI monitor, so I can't test this too deeply though. It does work for me as expected on my computers.

Could you do me a favor and make sure pinned tab's widths aren't cut off or too long with this patch? I don't suspect it will make a difference though.

@dvs1
Copy link

dvs1 commented Oct 17, 2017

Pinning tabs seems the same on this iMac.

Regardless of pinning, depending on the website, this change can invoke a different favicon.
That's not a problem for me, I just thought it was worth mentioning.

@dvs1
Copy link

dvs1 commented Oct 17, 2017

For example, I get the "For Apple devices" icon with this patch,
and "All others" without it.

<!-- favicon -->
<!-- For Apple devices -->
<link href='/assets/apple-touch-icon-9f7c9cc3923b4817fbb25bfeb0dd5fd34638827fd9c82b5a03f7b68e6e90806f.png' rel='apple-touch-icon'>
<!-- For Nokia devices -->
<link href='/assets/apple-touch-icon-9f7c9cc3923b4817fbb25bfeb0dd5fd34638827fd9c82b5a03f7b68e6e90806f.png' rel='shortcut icon'>
<!-- All others -->
<link href='/assets/favicon-6c483382d6196fb46215c44e6bdaffcdd019cc0a8e12fd1a1b62356721de1476.png' rel='shortcut icon'>

source: https://diasp.org

@The-Compiler
Copy link
Member

For example, I get the "For Apple devices" icon with this patch, and "All others" without it.

I can't reproduce this (Linux, Qt 5.9, both QtWebKit and QtWebEngine) - I get the normal (red star on black circle) icon both with and without the patch.

@nicktelford
Copy link

nicktelford commented Oct 20, 2017

I get the white star on black background "for Apple devices" icon (Linux, Qt 5.9, QtWebEngine, 4K High DPI display).

I suspect it's actually selecting the "Nokia devices" icon, as that's the same PNG, but with the shortcut icon type. I think it's selecting the higher resolution icon if the screen resolution is high enough to warrant it.

@The-Compiler
Copy link
Member

Hmm, seeing that this seems to display favicons which aren't really suitable for qutebrowser's small default favicon size, I feel like this should be an option. @foliea is this something you'd like to work on? Let me know if you need help.

@dvs1
Copy link

dvs1 commented Oct 25, 2017

...aren't really suitable for qutebrowser's small default favicon size...

In what sense? They are scaled to be the same size in qutebrowser.

@The-Compiler
Copy link
Member

I don't follow - from what it sounds, qutebrowser show the icon intended for high resolutions as something like a 10x10px icon, right?

@dvs1
Copy link

dvs1 commented Oct 25, 2017

The icon seems to scale with c.fonts.tabs, so that it is always the appropriate size.

With the default:

screen shot 2017-10-25 at 11 35 20

With 14pt:

screen shot 2017-10-25 at 11 36 10

@The-Compiler
Copy link
Member

yes - what I'm saying is that e.g. https://diasp.org/ has two favicons - a small one:
image
and a bigger one (for phone launchers?):
image

If I'm understanding what you said above correctly, you get the bigger one, but scaled down to 10x10 or so? If that's happening with this patch, I think it should be using a setting.

@dvs1
Copy link

dvs1 commented Oct 25, 2017

Actually, in testing just now, it appears that c.fonts.tabs being set to 14pt is what triggers the alternative icon.

Default:

screen shot 2017-10-25 at 11 41 27

14pt:

screen shot 2017-10-25 at 11 41 58

@The-Compiler The-Compiler merged commit 57e1135 into qutebrowser:master Oct 25, 2017
@The-Compiler
Copy link
Member

Ah, that seems fine to me then - thanks everyone for the PR, reviews and tests!

@foliea foliea deleted the fix-blurry-favicon branch October 27, 2017 14:36
@The-Compiler The-Compiler mentioned this pull request Nov 6, 2017
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

6 participants