Re: Review Request 119983: Use font metrics to scale icons for high dpi outputs
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119983/ --- (Updated April 13, 2015, 10:38 a.m.) Status -- This change has been discarded. Review request for Plasma, Kai Uwe Broulik, David Edmundson, and Vishesh Handa. Bugs: 337712 and 338308 http://bugs.kde.org/show_bug.cgi?id=337712 http://bugs.kde.org/show_bug.cgi?id=338308 Repository: plasma-framework Description --- Use font metrics to scale icons for high dpi outputs QScreen, through EDID reports bogus values for physicalDotsPerInch*(). This leads to oversized icons on monitors with bogus edid information. This patch changes the ratio underlying to the icon sizing for displays with a DPI 96 * 1.5 to use the ratio between the font as rendered (its effective pixelSize, essentially) to scale the icon sizes up. As we rely on proper font metrics throughout already, this should bring sizing in line with the font, which is something that makes sense as it means we're sharing the underlying mechanism (font metrics) for sizing in different areas. The downside of this patch is that we're essentially working around an issue that should be fixed in the hardware, the monitor's edid. Unrealistic. print dpi / sizing in dpitest Print out some useful information to deduce dpi and pixel sizing. David, and Kai Uwe, Vishesh (since I know you have a high DPI displays), could you run this patch for a bit and check if it works for you, too? I've pushed it to plasma-framework[sebas/dpi] for your git convenience. Diffs - src/declarativeimports/core/units.h ba481781a04a54cb77f99048d3d400fdae617b38 src/declarativeimports/core/units.cpp 56c0b55427c128beff5f8d18c37847a435f194c0 tests/dpi/dpitest.cpp c3d2c3e6821fd79fc8b5ed0b3559a5870f88aa36 Diff: https://git.reviewboard.kde.org/r/119983/diff/ Testing --- Ran Plasma Desktop, no apparent problems (on this hardware, which worked, anyway). Thanks, Sebastian Kügler ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119983: Use font metrics to scale icons for high dpi outputs
On Aug. 29, 2014, 1:11 a.m., David Edmundson wrote: src/declarativeimports/core/units.cpp, lines 197-198 https://git.reviewboard.kde.org/r/119983/diff/1/?file=308044#file308044line197 I'm confused. gridUnit is based on QGuiApplication::font() and we generate a ratio comparing it against QGuiApplication::font() won't that always result in the same constant? Not quite, the grid unit uses boundingRect.height(), which is in pixels, while this uses pointSize(), that's not in pixels. If I understand it correctly, same point size can have different pixel sizes depending on things. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119983/#review65459 --- On Aug. 29, 2014, 1:03 a.m., Sebastian Kügler wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119983/ --- (Updated Aug. 29, 2014, 1:03 a.m.) Review request for Plasma, Kai Uwe Broulik, David Edmundson, and Vishesh Handa. Bugs: 337712 and 338308 http://bugs.kde.org/show_bug.cgi?id=337712 http://bugs.kde.org/show_bug.cgi?id=338308 Repository: plasma-framework Description --- Use font metrics to scale icons for high dpi outputs QScreen, through EDID reports bogus values for physicalDotsPerInch*(). This leads to oversized icons on monitors with bogus edid information. This patch changes the ratio underlying to the icon sizing for displays with a DPI 96 * 1.5 to use the ratio between the font as rendered (its effective pixelSize, essentially) to scale the icon sizes up. As we rely on proper font metrics throughout already, this should bring sizing in line with the font, which is something that makes sense as it means we're sharing the underlying mechanism (font metrics) for sizing in different areas. The downside of this patch is that we're essentially working around an issue that should be fixed in the hardware, the monitor's edid. Unrealistic. print dpi / sizing in dpitest Print out some useful information to deduce dpi and pixel sizing. David, and Kai Uwe, Vishesh (since I know you have a high DPI displays), could you run this patch for a bit and check if it works for you, too? I've pushed it to plasma-framework[sebas/dpi] for your git convenience. Diffs - src/declarativeimports/core/units.h ba481781a04a54cb77f99048d3d400fdae617b38 src/declarativeimports/core/units.cpp 56c0b55427c128beff5f8d18c37847a435f194c0 tests/dpi/dpitest.cpp c3d2c3e6821fd79fc8b5ed0b3559a5870f88aa36 Diff: https://git.reviewboard.kde.org/r/119983/diff/ Testing --- Ran Plasma Desktop, no apparent problems (on this hardware, which worked, anyway). Thanks, Sebastian Kügler ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119983: Use font metrics to scale icons for high dpi outputs
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119983/#review65472 --- shouldn't be m_gridunit be calculated from fontPixelRatio as well then? to be really sure the two things are calculated with the same logic - Marco Martin On Aug. 28, 2014, 11:03 p.m., Sebastian Kügler wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119983/ --- (Updated Aug. 28, 2014, 11:03 p.m.) Review request for Plasma, Kai Uwe Broulik, David Edmundson, and Vishesh Handa. Bugs: 337712 and 338308 http://bugs.kde.org/show_bug.cgi?id=337712 http://bugs.kde.org/show_bug.cgi?id=338308 Repository: plasma-framework Description --- Use font metrics to scale icons for high dpi outputs QScreen, through EDID reports bogus values for physicalDotsPerInch*(). This leads to oversized icons on monitors with bogus edid information. This patch changes the ratio underlying to the icon sizing for displays with a DPI 96 * 1.5 to use the ratio between the font as rendered (its effective pixelSize, essentially) to scale the icon sizes up. As we rely on proper font metrics throughout already, this should bring sizing in line with the font, which is something that makes sense as it means we're sharing the underlying mechanism (font metrics) for sizing in different areas. The downside of this patch is that we're essentially working around an issue that should be fixed in the hardware, the monitor's edid. Unrealistic. print dpi / sizing in dpitest Print out some useful information to deduce dpi and pixel sizing. David, and Kai Uwe, Vishesh (since I know you have a high DPI displays), could you run this patch for a bit and check if it works for you, too? I've pushed it to plasma-framework[sebas/dpi] for your git convenience. Diffs - src/declarativeimports/core/units.h ba481781a04a54cb77f99048d3d400fdae617b38 src/declarativeimports/core/units.cpp 56c0b55427c128beff5f8d18c37847a435f194c0 tests/dpi/dpitest.cpp c3d2c3e6821fd79fc8b5ed0b3559a5870f88aa36 Diff: https://git.reviewboard.kde.org/r/119983/diff/ Testing --- Ran Plasma Desktop, no apparent problems (on this hardware, which worked, anyway). Thanks, Sebastian Kügler ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119983: Use font metrics to scale icons for high dpi outputs
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119983/#review65473 --- I'm on a standard pleb monitor, I get: $ ./dpitest dpitest(8664)/(default) Plasma::DPITest::runMain: DPI test runs: dpitest(8664)/(default) Plasma::DPITest::runMain: font.pixelSize: -1 dpitest(8664)/(default) Plasma::DPITest::runMain: font.pointSize: 9 dpitest(8664)/(default) Plasma::DPITest::runMain: devicePixelRatio: 0.963947 dpitest(8664)/(default) Plasma::DPITest::runMain: pointSize * devicePixelRatio: 8.67552 dpitest(8664)/(default) Plasma::DPITest::runMain: dpi: 92.5389 dpitest(8664)/(default) Plasma::DPITest::runMain: gridUnit: 14 dpitest(8664)/(default) Plasma::DPITest::runMain: gridUnit / pointSize : 1.6 I should have a ratio of ~1 I think, but it does seem to look OK. I'll pester vishesh to run it too. - David Edmundson On Aug. 28, 2014, 11:03 p.m., Sebastian Kügler wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119983/ --- (Updated Aug. 28, 2014, 11:03 p.m.) Review request for Plasma, Kai Uwe Broulik, David Edmundson, and Vishesh Handa. Bugs: 337712 and 338308 http://bugs.kde.org/show_bug.cgi?id=337712 http://bugs.kde.org/show_bug.cgi?id=338308 Repository: plasma-framework Description --- Use font metrics to scale icons for high dpi outputs QScreen, through EDID reports bogus values for physicalDotsPerInch*(). This leads to oversized icons on monitors with bogus edid information. This patch changes the ratio underlying to the icon sizing for displays with a DPI 96 * 1.5 to use the ratio between the font as rendered (its effective pixelSize, essentially) to scale the icon sizes up. As we rely on proper font metrics throughout already, this should bring sizing in line with the font, which is something that makes sense as it means we're sharing the underlying mechanism (font metrics) for sizing in different areas. The downside of this patch is that we're essentially working around an issue that should be fixed in the hardware, the monitor's edid. Unrealistic. print dpi / sizing in dpitest Print out some useful information to deduce dpi and pixel sizing. David, and Kai Uwe, Vishesh (since I know you have a high DPI displays), could you run this patch for a bit and check if it works for you, too? I've pushed it to plasma-framework[sebas/dpi] for your git convenience. Diffs - src/declarativeimports/core/units.h ba481781a04a54cb77f99048d3d400fdae617b38 src/declarativeimports/core/units.cpp 56c0b55427c128beff5f8d18c37847a435f194c0 tests/dpi/dpitest.cpp c3d2c3e6821fd79fc8b5ed0b3559a5870f88aa36 Diff: https://git.reviewboard.kde.org/r/119983/diff/ Testing --- Ran Plasma Desktop, no apparent problems (on this hardware, which worked, anyway). Thanks, Sebastian Kügler ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119983: Use font metrics to scale icons for high dpi outputs
On Aug. 29, 2014, 10:33 a.m., David Edmundson wrote: I'm on a standard pleb monitor, I get: $ ./dpitest dpitest(8664)/(default) Plasma::DPITest::runMain: DPI test runs: dpitest(8664)/(default) Plasma::DPITest::runMain: font.pixelSize: -1 dpitest(8664)/(default) Plasma::DPITest::runMain: font.pointSize: 9 dpitest(8664)/(default) Plasma::DPITest::runMain: devicePixelRatio: 0.963947 dpitest(8664)/(default) Plasma::DPITest::runMain: pointSize * devicePixelRatio: 8.67552 dpitest(8664)/(default) Plasma::DPITest::runMain: dpi: 92.5389 dpitest(8664)/(default) Plasma::DPITest::runMain: gridUnit: 14 dpitest(8664)/(default) Plasma::DPITest::runMain: gridUnit / pointSize : 1.6 I should have a ratio of ~1 I think, but it does seem to look OK. I'll pester vishesh to run it too. actaully kickoff is a bit big: http://imgur.com/Kg8BXcI - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119983/#review65473 --- On Aug. 28, 2014, 11:03 p.m., Sebastian Kügler wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119983/ --- (Updated Aug. 28, 2014, 11:03 p.m.) Review request for Plasma, Kai Uwe Broulik, David Edmundson, and Vishesh Handa. Bugs: 337712 and 338308 http://bugs.kde.org/show_bug.cgi?id=337712 http://bugs.kde.org/show_bug.cgi?id=338308 Repository: plasma-framework Description --- Use font metrics to scale icons for high dpi outputs QScreen, through EDID reports bogus values for physicalDotsPerInch*(). This leads to oversized icons on monitors with bogus edid information. This patch changes the ratio underlying to the icon sizing for displays with a DPI 96 * 1.5 to use the ratio between the font as rendered (its effective pixelSize, essentially) to scale the icon sizes up. As we rely on proper font metrics throughout already, this should bring sizing in line with the font, which is something that makes sense as it means we're sharing the underlying mechanism (font metrics) for sizing in different areas. The downside of this patch is that we're essentially working around an issue that should be fixed in the hardware, the monitor's edid. Unrealistic. print dpi / sizing in dpitest Print out some useful information to deduce dpi and pixel sizing. David, and Kai Uwe, Vishesh (since I know you have a high DPI displays), could you run this patch for a bit and check if it works for you, too? I've pushed it to plasma-framework[sebas/dpi] for your git convenience. Diffs - src/declarativeimports/core/units.h ba481781a04a54cb77f99048d3d400fdae617b38 src/declarativeimports/core/units.cpp 56c0b55427c128beff5f8d18c37847a435f194c0 tests/dpi/dpitest.cpp c3d2c3e6821fd79fc8b5ed0b3559a5870f88aa36 Diff: https://git.reviewboard.kde.org/r/119983/diff/ Testing --- Ran Plasma Desktop, no apparent problems (on this hardware, which worked, anyway). Thanks, Sebastian Kügler ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request 119983: Use font metrics to scale icons for high dpi outputs
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119983/ --- Review request for Plasma, Kai Uwe Broulik, David Edmundson, and Vishesh Handa. Bugs: 337712 and 338308 http://bugs.kde.org/show_bug.cgi?id=337712 http://bugs.kde.org/show_bug.cgi?id=338308 Repository: plasma-framework Description --- Use font metrics to scale icons for high dpi outputs QScreen, through EDID reports bogus values for physicalDotsPerInch*(). This leads to oversized icons on monitors with bogus edid information. This patch changes the ratio underlying to the icon sizing for displays with a DPI 96 * 1.5 to use the ratio between the font as rendered (its effective pixelSize, essentially) to scale the icon sizes up. As we rely on proper font metrics throughout already, this should bring sizing in line with the font, which is something that makes sense as it means we're sharing the underlying mechanism (font metrics) for sizing in different areas. The downside of this patch is that we're essentially working around an issue that should be fixed in the hardware, the monitor's edid. Unrealistic. print dpi / sizing in dpitest Print out some useful information to deduce dpi and pixel sizing. David, and Kai Uwe, Vishesh (since I know you have a high DPI displays), could you run this patch for a bit and check if it works for you, too? I've pushed it to plasma-framework[sebas/dpi] for your git convenience. Diffs - src/declarativeimports/core/units.h ba481781a04a54cb77f99048d3d400fdae617b38 src/declarativeimports/core/units.cpp 56c0b55427c128beff5f8d18c37847a435f194c0 tests/dpi/dpitest.cpp c3d2c3e6821fd79fc8b5ed0b3559a5870f88aa36 Diff: https://git.reviewboard.kde.org/r/119983/diff/ Testing --- Ran Plasma Desktop, no apparent problems (on this hardware, which worked, anyway). Thanks, Sebastian Kügler ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119983: Use font metrics to scale icons for high dpi outputs
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119983/#review65459 --- src/declarativeimports/core/units.cpp https://git.reviewboard.kde.org/r/119983/#comment45732 I'm confused. gridUnit is based on QGuiApplication::font() and we generate a ratio comparing it against QGuiApplication::font() won't that always result in the same constant? - David Edmundson On Aug. 28, 2014, 11:03 p.m., Sebastian Kügler wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119983/ --- (Updated Aug. 28, 2014, 11:03 p.m.) Review request for Plasma, Kai Uwe Broulik, David Edmundson, and Vishesh Handa. Bugs: 337712 and 338308 http://bugs.kde.org/show_bug.cgi?id=337712 http://bugs.kde.org/show_bug.cgi?id=338308 Repository: plasma-framework Description --- Use font metrics to scale icons for high dpi outputs QScreen, through EDID reports bogus values for physicalDotsPerInch*(). This leads to oversized icons on monitors with bogus edid information. This patch changes the ratio underlying to the icon sizing for displays with a DPI 96 * 1.5 to use the ratio between the font as rendered (its effective pixelSize, essentially) to scale the icon sizes up. As we rely on proper font metrics throughout already, this should bring sizing in line with the font, which is something that makes sense as it means we're sharing the underlying mechanism (font metrics) for sizing in different areas. The downside of this patch is that we're essentially working around an issue that should be fixed in the hardware, the monitor's edid. Unrealistic. print dpi / sizing in dpitest Print out some useful information to deduce dpi and pixel sizing. David, and Kai Uwe, Vishesh (since I know you have a high DPI displays), could you run this patch for a bit and check if it works for you, too? I've pushed it to plasma-framework[sebas/dpi] for your git convenience. Diffs - src/declarativeimports/core/units.h ba481781a04a54cb77f99048d3d400fdae617b38 src/declarativeimports/core/units.cpp 56c0b55427c128beff5f8d18c37847a435f194c0 tests/dpi/dpitest.cpp c3d2c3e6821fd79fc8b5ed0b3559a5870f88aa36 Diff: https://git.reviewboard.kde.org/r/119983/diff/ Testing --- Ran Plasma Desktop, no apparent problems (on this hardware, which worked, anyway). Thanks, Sebastian Kügler ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
