Re: Review Request 108504: [High-dpi issues] Fix KIconDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108504/#review25914 --- kio/kfile/kicondialog.cpp http://git.reviewboard.kde.org/r/108504/#comment19799 I should've googl'd a bit more thorougly. Using q-setInitialSize works here :) - Kai Uwe Broulik On Jan. 20, 2013, 11:15 a.m., Kai Uwe Broulik wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108504/ --- (Updated Jan. 20, 2013, 11:15 a.m.) Review request for kdelibs and KDE Usability. Description --- This makes the KIconDialog (the dialog where you can choose icons for eg. folders) respect the global icon size. Almost all sizes were hardcoded but the patch does away with all of this and works fine with all icon sizes and big font sizes. Also made it aware of FontMetrics (atm with bigger fonts, they also get clipped) and adjusts the grid height accordingly. Was fun diving into that ancient code :) Diffs - kio/kfile/kicondialog.cpp b7d646f Diff: http://git.reviewboard.kde.org/r/108504/diff/ Testing --- Yup, see screenshot. The only issue that remains is the initial size of the dialog to make it show 4 rows of icons. In the current implementation it just adds another 100px to the dialog height (cf. line 490), which is easy, if all the sizes are known and fixed, but with variing sizes this becomes an issue and I could not think of a proper solution. I probably need to add a sizeHint (tried in the private class, didn't help there)? The easiest but not neccessarily best solution would be to just set the minimumHeight to 4 rows and done. File Attachments Icon Dialog with 200dpi http://git.reviewboard.kde.org/media/uploaded/files/2013/01/20/icondialog.png Icon Dialog with 200dpi (without patch) http://git.reviewboard.kde.org/media/uploaded/files/2013/01/20/icondialog2.png Thanks, Kai Uwe Broulik
Re: Review Request 108504: [High-dpi issues] Fix KIconDialog
On Jan. 20, 2013, 11:53 a.m., Thomas Pfeiffer wrote: File Attachment: Icon Dialog with 200dpi http://git.reviewboard.kde.org/r/108504/#fcomment5 Why is that icon smaller than the others? Is it because the bigger version of it does not exist? Kai Uwe Broulik wrote: Also wondered but just checked, it is only available in max. 48x48 Ok. So from the usability POV, this is definitely an improvement. The code has to be checked by code people ;) (and IIRC, the usbaility team isn't supposed to click Ship It, to avoid confusion) - Thomas --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108504/#review25835 --- On Jan. 20, 2013, 11:15 a.m., Kai Uwe Broulik wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108504/ --- (Updated Jan. 20, 2013, 11:15 a.m.) Review request for kdelibs and KDE Usability. Description --- This makes the KIconDialog (the dialog where you can choose icons for eg. folders) respect the global icon size. Almost all sizes were hardcoded but the patch does away with all of this and works fine with all icon sizes and big font sizes. Also made it aware of FontMetrics (atm with bigger fonts, they also get clipped) and adjusts the grid height accordingly. Was fun diving into that ancient code :) Diffs - kio/kfile/kicondialog.cpp b7d646f Diff: http://git.reviewboard.kde.org/r/108504/diff/ Testing --- Yup, see screenshot. The only issue that remains is the initial size of the dialog to make it show 4 rows of icons. In the current implementation it just adds another 100px to the dialog height (cf. line 490), which is easy, if all the sizes are known and fixed, but with variing sizes this becomes an issue and I could not think of a proper solution. I probably need to add a sizeHint (tried in the private class, didn't help there)? The easiest but not neccessarily best solution would be to just set the minimumHeight to 4 rows and done. File Attachments Icon Dialog with 200dpi http://git.reviewboard.kde.org/media/uploaded/files/2013/01/20/icondialog.png Icon Dialog with 200dpi (without patch) http://git.reviewboard.kde.org/media/uploaded/files/2013/01/20/icondialog2.png Thanks, Kai Uwe Broulik
Re: Review Request 108504: [High-dpi issues] Fix KIconDialog
On Jan. 20, 2013, 2:40 p.m., Thomas Lübking wrote: kio/kfile/kicondialog.cpp, line 141 http://git.reviewboard.kde.org/r/108504/diff/2/?file=108215#file108215line141 Would it for such static sizes (why 60px in the first place?) not be more correct to use sh. like 60*100*2/(widget-physicalDpiX()+widget-physicalDpiY()) where 100 wold be the standard dpi? (could also be eg. 72 or 96) Right now (in latter RRs) you assign various configurable icon sizes to remotely related views. That might be correct and intended, but it's also a behavioral change and this time, desktop icons /can/ be assigned to 256px through the GUI - even on a 72dpi screen - and that's unlikely the intended size here. From what I can tell all the iconButtons are using Desktop icon sizes (either properly or some constant), so using it there which is triggered by the iconButton made sense to me. And since KDE is nowhere near dpi independent, is the easiest and best way to do this imho. Otherwise we should just patch KIcon to return an icon size based on the screen dpi … but we don't use SVG icons and only have a limited amount of icon sizes (there is for example no 96x96 which is double 48x48 icon), so I see no other way. - Kai Uwe --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108504/#review25844 --- On Jan. 20, 2013, 11:15 a.m., Kai Uwe Broulik wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108504/ --- (Updated Jan. 20, 2013, 11:15 a.m.) Review request for kdelibs and KDE Usability. Description --- This makes the KIconDialog (the dialog where you can choose icons for eg. folders) respect the global icon size. Almost all sizes were hardcoded but the patch does away with all of this and works fine with all icon sizes and big font sizes. Also made it aware of FontMetrics (atm with bigger fonts, they also get clipped) and adjusts the grid height accordingly. Was fun diving into that ancient code :) Diffs - kio/kfile/kicondialog.cpp b7d646f Diff: http://git.reviewboard.kde.org/r/108504/diff/ Testing --- Yup, see screenshot. The only issue that remains is the initial size of the dialog to make it show 4 rows of icons. In the current implementation it just adds another 100px to the dialog height (cf. line 490), which is easy, if all the sizes are known and fixed, but with variing sizes this becomes an issue and I could not think of a proper solution. I probably need to add a sizeHint (tried in the private class, didn't help there)? The easiest but not neccessarily best solution would be to just set the minimumHeight to 4 rows and done. File Attachments Icon Dialog with 200dpi http://git.reviewboard.kde.org/media/uploaded/files/2013/01/20/icondialog.png Icon Dialog with 200dpi (without patch) http://git.reviewboard.kde.org/media/uploaded/files/2013/01/20/icondialog2.png Thanks, Kai Uwe Broulik
Re: Review Request 108504: [High-dpi issues] Fix KIconDialog
On Jan. 20, 2013, 2:40 p.m., Thomas Lübking wrote: kio/kfile/kicondialog.cpp, line 141 http://git.reviewboard.kde.org/r/108504/diff/2/?file=108215#file108215line141 Would it for such static sizes (why 60px in the first place?) not be more correct to use sh. like 60*100*2/(widget-physicalDpiX()+widget-physicalDpiY()) where 100 wold be the standard dpi? (could also be eg. 72 or 96) Right now (in latter RRs) you assign various configurable icon sizes to remotely related views. That might be correct and intended, but it's also a behavioral change and this time, desktop icons /can/ be assigned to 256px through the GUI - even on a 72dpi screen - and that's unlikely the intended size here. Kai Uwe Broulik wrote: From what I can tell all the iconButtons are using Desktop icon sizes (either properly or some constant), so using it there which is triggered by the iconButton made sense to me. And since KDE is nowhere near dpi independent, is the easiest and best way to do this imho. Otherwise we should just patch KIcon to return an icon size based on the screen dpi … but we don't use SVG icons and only have a limited amount of icon sizes (there is for example no 96x96 which is double 48x48 icon), so I see no other way. Thomas Lübking wrote: Just checked, the view does indeed use the Desktop icons (not only group but also size) and simply caps them when they're 60x60 - what is obviously even wronger than wrong ;-) So fixed it must be, question is how - ie. is using the Desktop size really intended here or just some hardcoded messup between 48 and Desktop? Mho is that this is not the desktop and the user cannot actually expect the desktop size to be used here and while s/he might be happy with apple-a-like 3 or 4 128px icons on the desktop, that size renders the dialog unusable on even wuxga - not to mention all those 1366x768 notebooks. should just patch KIcon to return an icon size based on the screen dpi You mean when you ask for the Desktop icon size? Otherwise no option (you cannot return a 128px icon when the user code explcitly asked for 64px since you don't know what it's used for) there is for example no 96x96 which is double 48x48 icon - nearest integer, eg. coming from 72dpi, the correct size for 200dpi would be 133.33px - 128px (but of course there's always a chance that you run into a gap between the available sizes) If you seriously want to get KDE hires supportive, there must be fixes reg. the usable icon sizes (in doubt via a dumb lanczos scale or some pixel art algorithm) and global dpi awareness of the paintdevice. For the particular situation, relying on the desktop size is actually reasonable due to the mentioned bug, but that cannot be a global strategy :-( Well, Desktop defaults to 48x48 and if he increases it, he will probably have a reason for that. But yes, I also considered Dialog icons, or doubling dialog icons but this will give me for eaxmple 96x96 icons which get blurry. But that Desktop icon size has no real use case anyway since with Plasma Desktop there is no Desktop icon size. You either have an icon widget which can be scaled freely or the folderview which has its own slider for the icon size (and doesn't respect that setting anyway iirc) You mean when you ask for the Desktop icon size? Nope, that whenever you do anything with pixels it is automatically matched to your dpi so KDE becomes completely resolution independent xD - Kai Uwe --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108504/#review25844 --- On Jan. 20, 2013, 11:15 a.m., Kai Uwe Broulik wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108504/ --- (Updated Jan. 20, 2013, 11:15 a.m.) Review request for kdelibs and KDE Usability. Description --- This makes the KIconDialog (the dialog where you can choose icons for eg. folders) respect the global icon size. Almost all sizes were hardcoded but the patch does away with all of this and works fine with all icon sizes and big font sizes. Also made it aware of FontMetrics (atm with bigger fonts, they also get clipped) and adjusts the grid height accordingly. Was fun diving into that ancient code :) Diffs - kio/kfile/kicondialog.cpp b7d646f Diff: http://git.reviewboard.kde.org/r/108504/diff/ Testing --- Yup, see screenshot. The only issue that remains is the initial size of the dialog to make it show 4 rows of icons. In the current implementation it just adds another 100px to the dialog
Review Request 108504: [High-dpi issues] Fix KIconDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108504/ --- Review request for kdelibs and KDE Usability. Description --- This makes the KIconDialog (the dialog where you can choose icons for eg. folders) respect the global icon size. Almost all sizes were hardcoded but the patch does away with all of this and works fine with all icon sizes and big font sizes. Also made it aware of FontMetrics (atm with bigger fonts, they also get clipped) and adjusts the grid height accordingly. Was fun diving into that ancient code :) Diffs - kio/kfile/kicondialog.cpp b7d646f Diff: http://git.reviewboard.kde.org/r/108504/diff/ Testing --- Yup, see screenshot. The only issue that remains is the initial size of the dialog. In the current implementation it just adds another 100px to the dialog height, which is easy, if all the sizes are known and fixed, but with variing sizes this becomes an issue and I could not think of a proper solution. See code comment below. File Attachments Icon Dialog with 200dpi http://git.reviewboard.kde.org/media/uploaded/files/2013/01/20/icondialog.png Icon Dialog with 200dpi (without patch) http://git.reviewboard.kde.org/media/uploaded/files/2013/01/20/icondialog2.png Thanks, Kai Uwe Broulik
Re: Review Request 108504: [High-dpi issues] Fix KIconDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108504/#review25834 --- kio/kfile/kicondialog.cpp http://git.reviewboard.kde.org/r/108504/#comment19726 Didn't help here - Kai Uwe Broulik On Jan. 20, 2013, 11:13 a.m., Kai Uwe Broulik wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108504/ --- (Updated Jan. 20, 2013, 11:13 a.m.) Review request for kdelibs and KDE Usability. Description --- This makes the KIconDialog (the dialog where you can choose icons for eg. folders) respect the global icon size. Almost all sizes were hardcoded but the patch does away with all of this and works fine with all icon sizes and big font sizes. Also made it aware of FontMetrics (atm with bigger fonts, they also get clipped) and adjusts the grid height accordingly. Was fun diving into that ancient code :) Diffs - kio/kfile/kicondialog.cpp b7d646f Diff: http://git.reviewboard.kde.org/r/108504/diff/ Testing --- Yup, see screenshot. The only issue that remains is the initial size of the dialog. In the current implementation it just adds another 100px to the dialog height, which is easy, if all the sizes are known and fixed, but with variing sizes this becomes an issue and I could not think of a proper solution. See code comment below. File Attachments Icon Dialog with 200dpi http://git.reviewboard.kde.org/media/uploaded/files/2013/01/20/icondialog.png Icon Dialog with 200dpi (without patch) http://git.reviewboard.kde.org/media/uploaded/files/2013/01/20/icondialog2.png Thanks, Kai Uwe Broulik
Re: Review Request 108504: [High-dpi issues] Fix KIconDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108504/ --- (Updated Jan. 20, 2013, 11:15 a.m.) Review request for kdelibs and KDE Usability. Changes --- Stupid Review Board discarded my code comments .. damnit, publish already! Description --- This makes the KIconDialog (the dialog where you can choose icons for eg. folders) respect the global icon size. Almost all sizes were hardcoded but the patch does away with all of this and works fine with all icon sizes and big font sizes. Also made it aware of FontMetrics (atm with bigger fonts, they also get clipped) and adjusts the grid height accordingly. Was fun diving into that ancient code :) Diffs - kio/kfile/kicondialog.cpp b7d646f Diff: http://git.reviewboard.kde.org/r/108504/diff/ Testing (updated) --- Yup, see screenshot. The only issue that remains is the initial size of the dialog to make it show 4 rows of icons. In the current implementation it just adds another 100px to the dialog height (cf. line 490), which is easy, if all the sizes are known and fixed, but with variing sizes this becomes an issue and I could not think of a proper solution. I probably need to add a sizeHint (tried in the private class, didn't help there)? The easiest but not neccessarily best solution would be to just set the minimumHeight to 4 rows and done. File Attachments Icon Dialog with 200dpi http://git.reviewboard.kde.org/media/uploaded/files/2013/01/20/icondialog.png Icon Dialog with 200dpi (without patch) http://git.reviewboard.kde.org/media/uploaded/files/2013/01/20/icondialog2.png Thanks, Kai Uwe Broulik
Re: Review Request 108504: [High-dpi issues] Fix KIconDialog
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108504/#review25844 --- kio/kfile/kicondialog.cpp http://git.reviewboard.kde.org/r/108504/#comment19729 Would it for such static sizes (why 60px in the first place?) not be more correct to use sh. like 60*100*2/(widget-physicalDpiX()+widget-physicalDpiY()) where 100 wold be the standard dpi? (could also be eg. 72 or 96) Right now (in latter RRs) you assign various configurable icon sizes to remotely related views. That might be correct and intended, but it's also a behavioral change and this time, desktop icons /can/ be assigned to 256px through the GUI - even on a 72dpi screen - and that's unlikely the intended size here. - Thomas Lübking On Jan. 20, 2013, 11:15 a.m., Kai Uwe Broulik wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108504/ --- (Updated Jan. 20, 2013, 11:15 a.m.) Review request for kdelibs and KDE Usability. Description --- This makes the KIconDialog (the dialog where you can choose icons for eg. folders) respect the global icon size. Almost all sizes were hardcoded but the patch does away with all of this and works fine with all icon sizes and big font sizes. Also made it aware of FontMetrics (atm with bigger fonts, they also get clipped) and adjusts the grid height accordingly. Was fun diving into that ancient code :) Diffs - kio/kfile/kicondialog.cpp b7d646f Diff: http://git.reviewboard.kde.org/r/108504/diff/ Testing --- Yup, see screenshot. The only issue that remains is the initial size of the dialog to make it show 4 rows of icons. In the current implementation it just adds another 100px to the dialog height (cf. line 490), which is easy, if all the sizes are known and fixed, but with variing sizes this becomes an issue and I could not think of a proper solution. I probably need to add a sizeHint (tried in the private class, didn't help there)? The easiest but not neccessarily best solution would be to just set the minimumHeight to 4 rows and done. File Attachments Icon Dialog with 200dpi http://git.reviewboard.kde.org/media/uploaded/files/2013/01/20/icondialog.png Icon Dialog with 200dpi (without patch) http://git.reviewboard.kde.org/media/uploaded/files/2013/01/20/icondialog2.png Thanks, Kai Uwe Broulik
Re: Review Request 108504: [High-dpi issues] Fix KIconDialog
On Jan. 20, 2013, 2:40 p.m., Thomas Lübking wrote: kio/kfile/kicondialog.cpp, line 141 http://git.reviewboard.kde.org/r/108504/diff/2/?file=108215#file108215line141 Would it for such static sizes (why 60px in the first place?) not be more correct to use sh. like 60*100*2/(widget-physicalDpiX()+widget-physicalDpiY()) where 100 wold be the standard dpi? (could also be eg. 72 or 96) Right now (in latter RRs) you assign various configurable icon sizes to remotely related views. That might be correct and intended, but it's also a behavioral change and this time, desktop icons /can/ be assigned to 256px through the GUI - even on a 72dpi screen - and that's unlikely the intended size here. Kai Uwe Broulik wrote: From what I can tell all the iconButtons are using Desktop icon sizes (either properly or some constant), so using it there which is triggered by the iconButton made sense to me. And since KDE is nowhere near dpi independent, is the easiest and best way to do this imho. Otherwise we should just patch KIcon to return an icon size based on the screen dpi … but we don't use SVG icons and only have a limited amount of icon sizes (there is for example no 96x96 which is double 48x48 icon), so I see no other way. Just checked, the view does indeed use the Desktop icons (not only group but also size) and simply caps them when they're 60x60 - what is obviously even wronger than wrong ;-) So fixed it must be, question is how - ie. is using the Desktop size really intended here or just some hardcoded messup between 48 and Desktop? Mho is that this is not the desktop and the user cannot actually expect the desktop size to be used here and while s/he might be happy with apple-a-like 3 or 4 128px icons on the desktop, that size renders the dialog unusable on even wuxga - not to mention all those 1366x768 notebooks. should just patch KIcon to return an icon size based on the screen dpi You mean when you ask for the Desktop icon size? Otherwise no option (you cannot return a 128px icon when the user code explcitly asked for 64px since you don't know what it's used for) there is for example no 96x96 which is double 48x48 icon - nearest integer, eg. coming from 72dpi, the correct size for 200dpi would be 133.33px - 128px (but of course there's always a chance that you run into a gap between the available sizes) If you seriously want to get KDE hires supportive, there must be fixes reg. the usable icon sizes (in doubt via a dumb lanczos scale or some pixel art algorithm) and global dpi awareness of the paintdevice. For the particular situation, relying on the desktop size is actually reasonable due to the mentioned bug, but that cannot be a global strategy :-( - Thomas --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108504/#review25844 --- On Jan. 20, 2013, 11:15 a.m., Kai Uwe Broulik wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108504/ --- (Updated Jan. 20, 2013, 11:15 a.m.) Review request for kdelibs and KDE Usability. Description --- This makes the KIconDialog (the dialog where you can choose icons for eg. folders) respect the global icon size. Almost all sizes were hardcoded but the patch does away with all of this and works fine with all icon sizes and big font sizes. Also made it aware of FontMetrics (atm with bigger fonts, they also get clipped) and adjusts the grid height accordingly. Was fun diving into that ancient code :) Diffs - kio/kfile/kicondialog.cpp b7d646f Diff: http://git.reviewboard.kde.org/r/108504/diff/ Testing --- Yup, see screenshot. The only issue that remains is the initial size of the dialog to make it show 4 rows of icons. In the current implementation it just adds another 100px to the dialog height (cf. line 490), which is easy, if all the sizes are known and fixed, but with variing sizes this becomes an issue and I could not think of a proper solution. I probably need to add a sizeHint (tried in the private class, didn't help there)? The easiest but not neccessarily best solution would be to just set the minimumHeight to 4 rows and done. File Attachments Icon Dialog with 200dpi http://git.reviewboard.kde.org/media/uploaded/files/2013/01/20/icondialog.png Icon Dialog with 200dpi (without patch) http://git.reviewboard.kde.org/media/uploaded/files/2013/01/20/icondialog2.png Thanks, Kai Uwe Broulik
