Skip to content

Conversation

@sandman7920
Copy link
Contributor

Enable HiDPI on Windows by default #2468

@MKleusberg
Copy link
Member

Looks good to me but I can't really test it. Maybe @justinclift can make a build for @chrisjlocke to check this doesn't change anything for non-HiDPI monitors?

@justinclift
Copy link
Member

Yep, I can make a special build that includes this, so we can make sure it doesn't break standard resolution displays too.

I'll get around it to after figuring out the macOS 3.12.1 release build problem, probably tonight some time. 😄

@sandman7920
Copy link
Contributor Author

Are icons/resources available in higher resolution?
Qt::AA_UseHighDpiPixmaps will try to load @2x,@3x etc.
With this PR everything is scaled properly, but icons are blurry.

@justinclift
Copy link
Member

Good question. I think we use one of the more common OSS icon sets that's around, I'm just not personally sure which one(s). Someone will know though.

Hopefully that icon set has some SVG's too, which might be useful for solving that problem. 😄

@chrisjlocke
Copy link
Member

If you mean the button icons, then they're all derived from 16x16 PNGs.... so no high resolution variants are available.
http://www.famfamfam.com/lab/icons/silk/

@sandman7920
Copy link
Contributor Author

https://codefisher.org/pastel-svg/

@justinclift
Copy link
Member

@sandman7920 Oh cool, that looks promising. 😄

@chrisjlocke
Copy link
Member

Excellent, thanks @sandman7920 - useful link! I use that icon set a lot in my own projects, so having larger versions is handy.

@justinclift
Copy link
Member

This is a build of our latest master branch, with this PR applied on top:

No icon adjustments in it though. 😉

@chrisjlocke Would you be ok to see how it looks on your (non HiDPI?) setup? 😄

@chrisjlocke
Copy link
Member

Runs without any issues at all. 👍

@justinclift
Copy link
Member

justinclift commented Nov 11, 2020

Excellent, thanks @chrisjlocke and @sandman7920! 😄

@justinclift justinclift merged commit 1dc7ffe into sqlitebrowser:master Nov 11, 2020
@justinclift
Copy link
Member

Ok, this is merged now. We still need to figure out updated icons for HiDPI users though.

@chrisjlocke
Copy link
Member

I assume you'd need some 'resources' directory where DB4S pulls the resources as necessary. If it detects HiDPI (or command-switch?) it uses an alternative directory which contains SVGs or larger PNGs.

@sandman7920
Copy link
Contributor Author

sandman7920 commented Nov 11, 2020

@chrisjlocke one could add application_go.png ,[email protected],[email protected],etc in icons.qrc and then Qt will auto load necessary file depending on screen scale factor.

Edit
I see that DB4S use resource aliasing
Alias must be added for all scale factors.

<file alias="open_data_in_app">application_go.png</file>
<file alias="open_data_in_app@2x">[email protected]</file>

@chrisjlocke
Copy link
Member

chrisjlocke commented Nov 11, 2020

For the avoidance of doubt (that and I'm a dummy) if the normal 16x16 icon is application_go.png, then [email protected] would be 32x32, @3x.png would be 48x48, etc.
If I did that, threw it to Justin to lob in a build, etc, would you be OK to test it? I don't have an HiDPI screen to verify it would work. Would 64x64 be the max to go? It's going to take a while to do all icons, so didn't want to spend ages doing every conceivable option...

@sandman7920
Copy link
Contributor Author

I think @3x.png will be enough.
I will try to add some resources form link above and compile with Qt 5.15.1 and post some screenshots here.

You can test for yourself (windows)
set QT_AUTO_SCREEN_SCALE_FACTOR=0
set QT_SCALE_FACTOR=2
DB Browser for SQLite.exe

@chrisjlocke
Copy link
Member

Excellent, thanks!

@sandman7920
Copy link
Contributor Author

sandman7920 commented Nov 11, 2020

I have made test with database_add.png (new database).
It turns out that all toolbars have icon size set to 30 which means currently all icons are almost 100% upscaled.
I think it will be better icons to have 32px base.

Screenshots below are made with base 32 for database_add.png

Screenshot_2
Screenshot_3
Screenshot_1

@sandman7920
Copy link
Contributor Author

sandman7920 commented Nov 11, 2020

Arggh not all icons exists in the hi-res set
found.txt
not_found.txt

@chrisjlocke
Copy link
Member

Might be able to use variants and alternatives until they fill the gaps... there are also some ico to svg converters .... not perfect, but better than nothing.... maybe..... ;)

@sandman7920
Copy link
Contributor Author

I don't think project is still alive.
Otherwise with 32 base everything look crisp.
The best solution will be SVG format (one set of files, works on all resolutions).

@sandman7920
Copy link
Contributor Author

Digging into Qt documentation iconSize-prop

The default size is determined by the application's style and is derived from the QStyle::PM_ToolBarIconSize pixel metric.
It is the maximum size an icon can have. Icons of smaller size will not be scaled up.

This means iconSize it's a platform dependant, or the iconSize for all toolbars must be set manually.

@sandman7920
Copy link
Contributor Author

I have made build with SVG resources and everything looks great on my side (4K display).
icons.zip

@chrisjlocke may you test this build sqlitebrowser.zip

@justinclift maybe it's better HiDPI to be enabled on all platforms?

Some of UI files had to be changed, hard link to PNG files (not aliasing)

Replacements
view-refresh.png -> arrow_refresh_small.svg
comment_block.png -> text_padding_top.svg
database_link_broken.png -> link_break.svg
document-open.png -> folder_page.svg
internet-web-browser.png -> page_link.svg
document-link.png -> script_link.svg
page_foreign_key.png -> table_key.svg
save_all.png -> database_save.svg
style.png -> font.svg
style_add.png -> font_add.svg
style_delte.png -> font_delte.svg
style_edit.png -> font_edit.svg
text_indent.png -> text_padding_left.svg
text_underline.png -> text_underlined.svg

TODO
clear_filters.png
clear_sorting.png
cut.png
filter.png
hourglass.png
package_rename.png
package_save.png
page_copy_sql.png
page_paintbrush.png
sqlitebrowser.png
text_paintbrush.png
text_replace.png

@sandman7920
Copy link
Contributor Author

New vs Old

Screenshot_1

@justinclift
Copy link
Member

This looks like it's going down a pretty awesome path. 😄

maybe it's better HiDPI to be enabled on all platforms?

No objection here. Seems like we could drop the #ifdef Q_OS_WIN (and matching close) lines, which should stop it from being Windows-only.

That would at least let people on other platforms try it out, report problems, etc. 😄

@sandman7920
Copy link
Contributor Author

@justinclift could you try to build https://github.com/sandman7920/sqlitebrowser/tree/SVG on Mac OS

Replacement

view-refresh.png -> arrow_refresh_small.svg
comment_block.png -> text_padding_top.svg
database_link_broken.png -> link_break.svg
document-open.png -> folder_page.svg
internet-web-browser.png -> page_link.svg
document-link.png -> script_link.svg
page_foreign_key.png -> table_key.svg
save_all.png -> database_save.svg
style.png -> font.svg
style_add.png -> font_add.svg
style_delte.png -> font_delte.svg
style_edit.png -> font_edit.svg
text_indent.png -> text_padding_left.svg
text_underline.png -> text_underlined.svg
page_paintbrush.png -> color_hsl.svg
text_paintbrush.png -> color_wheel.svg

I couldn't find replacement for

clear_filters.png
clear_sorting.png
cut.png
filter.png
hourglass.png
package_rename.png
package_save.png
page_copy_sql.png
text_replace.png

Someone else should try to find replacement, I am pretty bad with those kind of things.

@sandman7920
Copy link
Contributor Author

@justinclift someone should test this, I don't have Mac at home and won't be going to the office for two weeks.

@justinclift
Copy link
Member

@revolter Would you have time to test if the icons in either of those two builds looks better on Retina / HiDPI screens than the standard (eg 3.12.1) build?

@revolter
Copy link
Member

Yes, they're sharp now, but too big:

image

image

In this case, the icons except the first one are still blurry:

image

@sandman7920
Copy link
Contributor Author

sandman7920 commented Nov 18, 2020

Toolbar icons are platform dependent.

QToolbar::iconSize
The default size is determined by the application's style and is derived from the QStyle::PM_ToolBarIconSize pixel metric.
It is the maximum size an icon can have. Icons of smaller size will not be scaled up.

Which means iconSize it's tuned to the platform Guidelines, from the screenshots above one could see that height of the Toolbar is the same as before (only icons are scaled up), Icons of smaller size will not be scaled up (before was 16px).

We could use something like this:

class DB4SProxyStyle : public QProxyStyle {
public:
  int pixelMetric(QStyle::PixelMetric metric,
                  const QStyleOption *option = nullptr,
                  const QWidget *widget = nullptr) const override {

    if (metric == QStyle::PM_ToolBarIconSize) {
      return 16; // or maybe 22
    }

    return QProxyStyle::pixelMetric(metric, option, widget);
  }
};

...

int main( int argc, char ** argv )
{
// Create application object. All the initialisation stuff happens in there
    Application a(argc, argv);
    a.setStyle(new DB4SProxyStyle);

This will override default style with custom value.

@justinclift , @revolter what do you think

@revolter
Copy link
Member

So we're not specifying their size at all, not directly nor indirectly? I suppose we're using the icons in src/icons, which looks like have a size of 16x16 pixels, while they are now displayed as 32x32 instead. So, are they really not scaled up?

@sandman7920
Copy link
Contributor Author

Yes, default size is set by platform style. On Windows is 30 on Linux 22, but if supplied resource is less than theme (platform default style) value Qt doesn't perform upscaling.
Here SVG is scalable by definition and it's upscaled to the theme value.

@revolter
Copy link
Member

I think that 16 would be perfect on macOS. For example, Safari uses 16pt icons too.

@sandman7920
Copy link
Contributor Author

Screenshots with 16px on Windows, everything looks OK.
@justinclift could you rebuild package with latest changes (style proxy)

https://github.com/sandman7920/sqlitebrowser/tree/SVG

image

@justinclift
Copy link
Member

@sandman7920 Yep, can do. Are Windows or macOS or both needed? 😄

@sandman7920
Copy link
Contributor Author

On Windows everything looks OK.

@justinclift what is the next step, make new pull request?

@justinclift
Copy link
Member

Meh, I'll do both. macOS ones are building atm. They should be online in about 45 mins or so...

@justinclift
Copy link
Member

Ahhh. Yeah, I'd say make a new PR. Since the new macOS build is already running though, we might as well check that just to make sure too. 😄

@sandman7920
Copy link
Contributor Author

In master branch?

@justinclift
Copy link
Member

Yep. master is the right target branch to make the PR against. 😄

@justinclift
Copy link
Member

@revolter
Copy link
Member

The buttons are also smaller now, but I personally actually like it more than before:

image

I don't know if it's related to this, but the text on these buttons gets truncated on hover:

image

@sandman7920
Copy link
Contributor Author

@revolter before on Mac OS iconSize was set 22 (i think), which means height of the ToolBar was 22 and icons were drawn in the middle. Now height is 16, I also think it's looks better now.

I don't know if it's related to this, but the text on these buttons gets truncated on hover

I don't think so, but you can try to start application with
env QT_AUTO_SCREEN_SCALE_FACTOR=0

@justinclift
Copy link
Member

The buttons are also smaller now, but I personally actually like it more than before

Awesome. Seems like an all-around win then. 😄

Thanks heaps for checking those builds @revolter. 😄

@sandman7920 You up for creating an appropriate PR (against master branch)? 😄

@revolter
Copy link
Member

I don't think so, but you can try to start application with
env QT_AUTO_SCREEN_SCALE_FACTOR=0

Meaning

QT_AUTO_SCREEN_SCALE_FACTOR=0 open "~/Downloads/DB Browser for SQLite.app"

?

@sandman7920
Copy link
Contributor Author

@revolter yes

@revolter
Copy link
Member

Then it doesn't fix that issue.

@justinclift
Copy link
Member

justinclift commented Nov 23, 2020

QT_AUTO_SCREEN_SCALE_FACTOR=0 open "~/Downloads/DB Browser for SQLite.app"

I don't think 'open' will set the environment variable when it runs the .app.

Instead, run the executable in the ... .app/Contents/MacOS/ folder directly. It'll be something like:

$ QT_AUTO_SCREEN_SCALE_FACTOR=0 "~/Downloads/DB Browser for SQLite.app/Contents/MacOS/DB Browser for SQLite"

@revolter
Copy link
Member

Oh, true. But it still doesn't fix the issue.

@sandman7920
Copy link
Contributor Author

sandman7920 commented Nov 23, 2020

@justinclift is this a Qt 5.15.x build

@justinclift
Copy link
Member

@sandman7920 Yep. Qt 5.15.1.

@justinclift
Copy link
Member

As a data point, the test build has also been confirmed as working by someone on Twitter a few weeks ago:

https://twitter.com/hakanu_/status/1341405851677057025

(Just noticed now, as I haven't been keeping an eye on Twitter)

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.

5 participants