Hi Semyon,
ShellFolder2.cpp
944 pIcon->Extract(szBuf, index, &hIcon, *0*, sz);
I think 0 should really be |NULL|.
The result of the call is ignored now. Is it intentional?
Win32ShellFolder2.java
1010 private static Image makeIcon(long hIcon, int bsize) {
|bsize| was called |baseSize| previously, and it conveyed the meaning
better, didn't it?
1043 if(hIcon <= 0) {
1044 if (isDirectory()) {
1045 return getShell32Icon(4, size);
1046 } else {
1047 return getShell32Icon(1, size);
1048 }
I guess I understand what hides behind 4 and 1: generic folder and
generic file icon ids. Would declaring these numbers as constants
improve readability?
|getIcon(int size)| and |getIcon(boolean getLargeIcon)| are somewhat
similar. The latter provides additional caching. Can it be simplified to
re-use portions of new |getIcon(int size)|?
Regards,
Alexey
On 22/09/2017 22:05, Semyon Sadetsky wrote:
Hi Alexey,
On 09/22/2017 01:01 PM, Alexey Ivanov wrote:
Hi Semyon,
On 22/09/2017 20:06, Semyon Sadetsky wrote:
Hi Alexey,
On 09/22/2017 10:53 AM, Alexey Ivanov wrote:
Hi Semyon,
On 22/09/2017 18:29, Semyon Sadetsky wrote:
Hi Alexey,
On 09/22/2017 09:43 AM, Alexey Ivanov wrote:
Hi Semyon,
On 22/09/2017 17:13, Semyon Sadetsky wrote:
Hi Alexey,
Thank you for your exact clarification.
On 09/22/2017 04:22 AM, Alexey Ivanov wrote:
<SNIP>
As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using
Windows API to retrieve the recommended size for small and
large icon size rather than defaulting to 16×16 and 32×32. If
HiDPI is in effect, the icons must be larger.
I also found this as most suitable approach for the moment.
Later this may be changed, for example, if Swing JFC is
re-factored to support shell determined icon sizes at HiDPI.
Swing UI scales to accommodate HiDPI settings. If fonts are
larger then icons should be larger too. Otherwise icons are too
small compared to surrounding text.
Anyway it could be postponed to a later fix.
Does it make sense to declare the standard sizes of 16×16 and
32×32 as constants at least in Java sources? This way, it would
be easier to find the places in code where a change is necessary.
This topic requires more investigations. At first, we need to keep
the API cross-platform and this requires comparing all supported
platforms in details. At the second, even for the existing windows
implementation there is an ambiguity in icons sizes received form
the OS shell. Windows platform has number of predefined constants
to query icon sizes (small, large, extra large, jumbo...) but
their actual size may differ depending on the shell preferences.
Those icon sizes may be changed by Windows registry settings and
may depend on the hi-res scale. I did several experiments and
found that depending on the way of desktop scaling in Windows 10
(it has two ways the new one and the old) at the same scale 2x the
returned large icon, for example, may be 32 or 64 pixels in size
(this was the root cause of the 8151385 bug).
I would postpone digging in this direction because we are not
planning to update Swing JFC dialog for better HiDPI view in the
nearest future. Also,we don't have statistics how users may use
the API. Since that, the most flexible API that leaves to the user
the decision about icon size to query seems more preferable.
I totally agree with your points. And the new API provides means
for app developer to choose better-suited size for icons.
What about using constants, private ones, for the two standard
sizes instead of using “magic” numbers?
Which constants do you mean? The next for large and small sizes?
public static final int FILE_ICON_SMALL = -1;
public static final int FILE_ICON_LARGE = -2;
they cannot be private because they are part of the new API that is
requested in the bug. The bug asks enabling the query for the large
icon that ShellFolder had been providing before. The bug itself is
not related to HiDPI. The possibility to get arbitrary icon sizes
was added because after 9 this may be in demand for HiDPI UIs.
I'm talking about these two numbers in Win32ShellFolder2.java as an
example:
public Image getIcon(final boolean getLargeIcon) {
int size = getLargeIcon ? *32* : *16*;
Does it make sense to declare constants for the size of 16 and 32. So
that the places where they're used are more easily identified if
someone decides to update the code later for supporting system icon size.
I updated the fix.
http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.01/
--Semyon
Regards,
Alexey
--Semyon
Other than that, the fix looks good to me.
Regards,
Alexey
--Semyon
Regards,
Alexey
--Semyon
Regards,
Alexey
<SNIP>
On 9/13/17 11:01, Semyon Sadetsky wrote:
Hello,
Please review fix for JDK10 (the changes involve AWT and
Swing):
bug: https://bugs.openjdk.java.net/browse/JDK-8182043
webrev:
http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/
The fix opens the part of the ShellFolder API for
getting system icons which was decided to be left closed
during the 8081722 enhancement review in 9.
Also the fix extends the API by adding possibility to
query file icons of arbitrary size and implements this
extension for Windows platform.
--Semyon
