<Swing Dev> <AWT Dev> [12] JDK-8182043: Access to Windows Large Icons
Phil Race
philip.race at oracle.com
Wed Jan 30 23:30:19 UTC 2019
I am not sure how an application is supposed to know what size to use,
which seems a bit of a problem with this API.
And does this work equally well on all platforms and resolutions ?
The extreme weirdness that you give it the name of a system object the
application
has to find and extract icons from is very odd.
I don't like this API at all. I'd rather do nothing.
And the supported size range is odd, and the potential for null returns
isn't specified.
-phil.
On 1/22/19 10:03 AM, Shashidhara Veerabhadraiah wrote:
>
> Hi All, Please review the below new fix compared to earlier webrevs.
> Now the new api to get the icons of different sizes is being made part
> of a new class SystemIcon. Please consider this as a different
> solution than the one that was under review for a long time till now.
>
> http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.05/
>
> Part of the fix is being borrowed from Semyon’s fix and would like to
> thank Semyon for that.
>
> Thanks and regards,
>
> Shashi
>
> *From:*Alexey Ivanov
> *Sent:* Thursday, October 11, 2018 4:32 AM
> *To:* Shashidhara Veerabhadraiah
> <shashidhara.veerabhadraiah at oracle.com>; Prasanta Sadhukhan
> <prasanta.sadhukhan at oracle.com>; swing-dev
> <swing-dev at openjdk.java.net>; awt-dev <awt-dev at openjdk.java.net>
> *Subject:* Re: <AWT Dev> <Swing Dev> [12] JDK-8182043: Access to
> Windows Large Icons
>
> Hi Shashi,
>
> Thank you for updating the review.
>
> With updated copyright years, the patch does not apply cleanly because
> some files already have 2018. It's a minor nuisance which could be
> easily resolved.
>
> Other comments inline:
>
> On 28/09/2018 09:58, Shashidhara Veerabhadraiah wrote:
>
> Hi Alexey, Thank you for your thorough review. I have updated the
> copyrights as well and please see below for my comments:
>
> Here is the new Webrev:
> http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.04/
> <http://cr.openjdk.java.net/%7Esveerabhadra/8182043/webrev.04/>
>
> Thanks and regards,
>
> Shashi
>
> *From:*Alexey Ivanov
> *Sent:* Tuesday, September 25, 2018 3:00 AM
> *To:* Shashidhara Veerabhadraiah
> <shashidhara.veerabhadraiah at oracle.com>
> <mailto:shashidhara.veerabhadraiah at oracle.com>; Prasanta Sadhukhan
> <prasanta.sadhukhan at oracle.com>
> <mailto:prasanta.sadhukhan at oracle.com>; swing-dev
> <swing-dev at openjdk.java.net> <mailto:swing-dev at openjdk.java.net>;
> awt-dev <awt-dev at openjdk.java.net> <mailto:awt-dev at openjdk.java.net>
> *Subject:* Re: <AWT Dev> <Swing Dev> [12] JDK-8182043: Access to
> Windows Large Icons
>
> Hi Shashi,
>
> Please see my comments inline:
>
> On 21/09/2018 23:22, Shashidhara Veerabhadraiah wrote:
>
> Hi Alexey, Thanks for your review and below is the new Webrev.
>
> http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.03/
> <http://cr.openjdk.java.net/%7Esveerabhadra/8182043/webrev.03/>
>
> Please see below for inline comments.
>
> Thanks and regards,
> Shashi
>
> *From:*Alexey Ivanov
> *Sent:* Friday, September 21, 2018 2:09 PM
> *To:* Shashidhara Veerabhadraiah
> <shashidhara.veerabhadraiah at oracle.com>
> <mailto:shashidhara.veerabhadraiah at oracle.com>; Prasanta
> Sadhukhan <prasanta.sadhukhan at oracle.com>
> <mailto:prasanta.sadhukhan at oracle.com>; swing-dev
> <swing-dev at openjdk.java.net>
> <mailto:swing-dev at openjdk.java.net>; awt-dev
> <awt-dev at openjdk.java.net> <mailto:awt-dev at openjdk.java.net>
> *Subject:* Re: <AWT Dev> <Swing Dev> [12] JDK-8182043: Access
> to Windows Large Icons
>
> Hi Shashi,
>
> SystemIcon.java
> What is the purpose of new SystemIcon class?
> It's not used anywhere but the provided test. Is this class
> really needed then?
> Is it supposed to become the public API for accessing system
> icons?
> Why can't FileSystemView be used for that purpose as it was
> proposed in Semyon's review?
> */[Shashi] /*SystemIcon is going to be the front face to
> access the icons and that is the purpose of this class. The
> reason for choosing this is that FileSystemView class can be
> used internally and did not wanted to expose it externally
> too. Externally exposing may cause certain restriction in
> maintaining the classes hence the indirection.
>
>
> Still, I cannot understand the rationale for a new class the only
> purpose of which is to provide public access to
> getSystemIcon(File, int, int).
> FileSystemView is already a public class, and it's used
> internally. (I guess it would not have existed, if it hadn't.) It
> has a public method getSystemIcon(File). As such, extending its
> functionality to get access to larger icons seems logical. This is
> what the new protected getSystemIcon(File f, int size) does.
>
> It can be made public to facilitate access to file icons.
> After all, protected method is also a contract, it cannot be
> changed without affecting backward compatibility.
>
> It is this new protected method that performs the task of getting
> the icon from the system.
>
> Do we really need other methods?
>
> */[Shashi] I think that system icons functions as part of
> filesystemview class is also a kind of corrupted creation of the
> filesystemview class. Icons forms a different functionality
> compared to file system and should have been kept as a separate
> class in the first place./*
>
>
> I agree to some extent… Yet FileSystemView.getSystemIcon(File) is part
> of this class since 1.4. Having this in mind, I see no reason why an
> extended version getSystemIcon(File file, int size) cannot be public.
>
> If the new method is public, the access to large icons, or rather
> icons of arbitrary size, is provided.
>
> Semyon's review made the new method public:
> http://mail.openjdk.java.net/pipermail/awt-dev/2017-September/013016.html
> http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/
> and the latest version
> http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.02/
>
>
> I'm still not convinced the new method should be exposed via a new
> class SystemIcon. I see no advantages to this compared to making the
> new method public in FileSystemView.
>
> The fact that you have to implement the abstract method
> createNewFolder() from FileSystemView for which you cannot provide a
> reasonable implementation only emphasizes it's a bad design decision.
>
>
> */Regarding the methods, am not sure does it required or not but I
> added them to be complete. Some variant is required to provide the
> original unmodified icon as the system returns and provide the
> scaled icon as well./*
>
>
> I don't think they're needed.
> Other helper methods merely scale the icon fetched via
> FileSystemView.getSystemIcon(File file, int size). Such scaling is not
> unique to icons, it's applicable to any images. As such, they should
> be in a dedicated utility class, shouldn't they?
>
>
> 278 public File createNewFolder(File containingDir) throws
> IOException {
> 279 return null;
> 280 }
> You had to implement the abstract method from FileSystemView. It's
> one more point to make system icon available right from
> FileSystemView.
> This implementation should rather throw an exception.
>
> */[Shashi] IOException? I think it’s not needed as we won’t be
> performing any such actions. But yes I had to implement this place
> holder to be complete./*
>
>
> That's exactly the problem. It's a public method which can be called
> by whoever wants to do it, and the caller rightfully expects the
> method to fulfil its contract. But this implementation can never
> fulfil its contract.
>
> It is the reason why I think SystemIcon class is not the way to go.
>
>
> 60 protected File file;
> This field is redundant, in my opinion. It would be quite
> expensive to instantiate SystemIcon object for each file. It can
> safely be removed, then only methods which take additional File
> parameter will be left.
> (The field could be final as it cannot be changed and should not
> be changed.)
>
> */[Shashi] Updated./*
>
>
> Thanks!
> Yet my main point was that you can safely remove this field and then
> leave only methods which take it as parameter.
>
>
> <SNIP>
>
> 112 public Icon getSystemIcon(int width, int height) {
> Are methods with width / height parameters needed? Icons are
> usually square.
>
> */[Shashi] Flexibility is ok I think. It would fall back to the
> same function though. Though the native does not have the function
> and since because of scaling we can support that. Am not sure
> where it can be useful but being flexible is ok I think!!/*
>
>
> Flexibility is okay unless it's flexibility for the sake of flexibility.
> On Windows, icons are always square. The new API is system
> independent. So the method makes sense if and only if there are
> platforms where file icons can be non-square.
> Can file icon have different width and height on Linux or macOS? If
> not, I'm for dropping the method.
>
>
>
> You repeat checks if f is null, width and height checks in each
> and every method. I guess parameter validation could be extract
> into a separate method. You will avoid lots of cope duplication.
>
> */[Shashi] Updated/*
>
>
>
> Since it's a completely new API, I suggest throwing
> IllegalArgumentException with appropriate message in the cases
> where a parameter (file, width and height) fails validation.
>
> */[Shashi] Updated./*
>
>
> Not all documentation comments for the public methods include throws
> clauses for the thrown IllegalArgumentException.
>
>
>
> 210 int size;
> 211 if(width > height) {
> 212 size = width;
> 213 } else {
> 214 size = height;
> 215 }
> This code can be simplified to
> int size = Math.max(width, height);
> Concise and clear.
> A helper method which validates the parameters could also return
> this value. Thus, again, avoiding code duplication among many
> methods in this class.
>
> */[Shashi] Updated./*
>
>
> I don't think getSize is a good name for the helper method.
> It could be validateParameters, thus its purpose is clearer.
> Documentation comment could clarify it returns the size of icon based
> on width and height.
> The method should be static as it does not use any fields.
>
>
>
>
> There are lots of tabs in this file. Tabs must be replaced with
> spaces.
> if's are inconsistent throughout the code: some are with space,
> some are without. Please add the space everyone to align with Java
> Code Conventions.
> Please also sort the imports and remove unused ones.
>
> */[Shashi] Updated/*
>
>
> The imports are not sorted.
> There are three unused imports.
>
>
> 259 return (Image)scaledImage;
> The cast is redundant.
>
>
>
>
>
> FileSystemView.java
> 259 * Icon for a file, directory, or folder as it would
> be displayed in
> 260 * a system file browser for the requested size.
> For getXXX, it's better to start description with “Returns…”
> so it aligns to other similar methods.
> However, I see the new method follows description of
> getIcon(boolean).
>
> */[Shashi] /*Because as you said rightly it follows the
> getIcon(boolean)
>
>
> Okay.
> Is it possible to update documentation to the existing
> getSystemIcon(File)?
> Should I file a separate bug to update the documentation?
>
> Documentation also references a non-public class ShellFolder.
> Should this reference be removed from documentation as the access
> to non-public classes is restricted? It does not add much value.
>
> */[Shashi] Updated./*
>
>
> The documentation for the newly added method hasn't been updated.
>
>
> 265 * @param size width and height of the icon in pixels to
> be scaled(valid range: 1 to 256)
> Why is it “to be scaled”? I would expect to get the icon of
> the requested size. At the same time, the icon can be scaled
> to the requested size if the requested size is not available.
>
> */[Shashi] /*User has no restriction of mentioning any size
> but the platform may have a limitation of size. Since we are
> supporting a set of different versions of platforms, platform
> may limit the size of the icon to a particular size, in which
> case it will be scaled to the user requested size.
>
>
> I understand that. However, I think the suggested description does
> not convey the meaning correctly.
> The method will return the icon of the requested size, won't it?
> So the correct description is:
> @param size width and height of the icon in pixels (valid range: 1
> to 256)
>
> The fact the returned icon may be scaled if the requested size is
> not available must be described in the method documentation as
> well as in @return line:
> @return an icon of the requested size (possibly scaled) as it
> would be displayed by a native file chooser
>
> */[Shashi] Updated/*
>
>
> I can't see any change here.
>
>
>
>
>
> 270 protected Icon getSystemIcon(File f, int size) {
> Can't the method be public? It was in Semyon's review.
>
> */[Shashi] /*Because of the indirection, this method can stay
> as protected. I think it is always good to be of using
> protected than making everything public. Also that is the
> advantage of adding the SystemIcon class.
>
>
> Sorry I don't see any advantage of having SystemIcon class over
> making this method public as I outlined above.
>
>
>
> 266 * @return an icon as it would be displayed by a native
> file chooser
> An icon of the requested size (possibly scaled) as…
>
> 275 if(size > 256 || size < 1) {
> 276 return null;
> 277 }
> Please add space between if and the opening parenthesis.
> You can throw InvalidArgumentException in this case.
> Does size of 1 make any sense?
>
> */[Shashi] /*Done. I can only say that 0 does not make sense.
> Check is to see that it is not less than 1.
>
>
> What about throwing InvalidArgumentException when size parameter
> is invalid?
>
> */[Shashi] Updated/*
>
>
> Not updated:
> 273 if (size > 256 || size < 1) {
> 274 return null;
> 275 }
> The method silently returns null as before.
>
>
> 288 return UIManager.getIcon(f.isDirectory() ?
> "FileView.directoryIcon" : "FileView.fileIcon");
> Should the icon be returned as MultiResolutionImage if icon size is
> different from the requested size?
>
>
>
> I understand that check is to make sure size is at least 1.
> However, icon of 1 pixel size does not make any sense. Should the
> minimum be a more sensible of 4?
> It's a concern for discussion.
> */[Shashi] /*On the native side, there is no restriction so I
> think we can keep this open.
>
>
>
>
> ShellFolder.java
> 202 /**
> 203 * @param size size of the icon > 0
> 204 * @return The icon used to display this shell folder
> 205 */
> Can you add a short description of the purpose of this method?
> “Returns the icon of the specified size used to display this
> shell folder”?
> A similar description can be added to the method above it:
> 198 public Image getIcon(boolean getLargeIcon) {
>
> */[Shashi] /*Updated. Thank you.
>
>
> Thank you for updating @return clause of the Javadoc.
> My intention was to add a generic description of the method as well:
> 202 /**
> 202 * Returns the icon of the specified size used to display
> this shell folder.
> 202 *
> 203 * @param size size of the icon > 0
> 204 * @return The icon used to display this shell folder
> 205 */
>
> Such description could also be added to method above
> getIcon(boolean getLargeIcon), at line 198.
>
> Should the range of size parameter be specified? For example,
> 1–256 as in FileSystemView.
> */[Shashi] /*Updated
>
>
> 207 * @param size size of the icon > 0(Valid range: 1 to 256)
> Isn't >0 redundant now?
>
> @param size size of the icon, valid range from 1 to 256
> looks better, doesn't it?
>
>
> <SNIP>
>
> 974 const int MAX_ICON_SIZE = 128;
> I also suggest increasing MAX_ICON_SIZE to 256. Otherwise I
> see no point in allowing 256 as the maximum size at Java level
> as you'll never have icon of 256×256 even thought the system
> may have one.
>
> */[Shashi] /*Per me, the problem is that since we support
> certain older versions of the platforms, it should not cause
> an exception at the native level. If everyone agrees for the
> change then we can change that.
>
>
> This concern was raised in the previous review too:
> http://mail.openjdk.java.net/pipermail/awt-dev/2017-September/013115.html
>
> I think it's safe to update the value of MAX_ICON_SIZE to 256. The
> oldest supported version of Windows is Windows 7 which supports
> 256×256 icons.
> Windows XP used icons up to 48×48, but it does not imply the API
> does not allow loading icon of larger size. Both 128 and 256
> should be tested on Windows XP if JDK still runs on it.
> */[Shashi] /*I will raise a bug on this and work on it later. I
> have to see fn_GetIconInfo() and what it returns the values and
> based on it, it is good to update to 256 I think.
>
>
> Why can't this be done under this bug?
> We promise to return icon of size 256 from the system but we will
> never return icon larger than 128.
>
> fn_GetIconInfo() is GetIconInfo from Windows API, see
> 202 fn_GetIconInfo = (GetIconInfoType)GetProcAddress(libUser32,
> "GetIconInfo");
> https://docs.microsoft.com/en-us/windows/desktop/api/winuser/nf-winuser-geticoninfo
> https://docs.microsoft.com/en-ie/windows/desktop/api/winuser/ns-winuser-_iconinfo
>
> The function returns HBITMAP, the bitmap data for the icon. Its size
> is used to calculate the size of the icon.
> If we requested icon of size 256, the size of the bitmap would be 256×256.
>
>
>
>
>
> Win32ShellFolder2.java
>
> <SNIP>
>
> … Yet I'm for using constants in this particular piece of code.
>
> These values are used at least twice in
> Win32ShellFolder2.java: lines 1081–1085 and 1119–1123.
>
> */[Shashi] Updated./*
>
>
> These could private:
> 80 static final int FILE_ICON_ID = 1;
> 81 static final int FOLDER_ICON_ID = 4;
>
>
>
> <SNIP>
>
> 382 return
> Win32ShellFolder2.getShell32Icon(i,
> key.startsWith("shell32LargeIcon ")?
> 383 LARGE_ICON_SIZE : SMALL_ICON_SIZE);
>
> May I suggest updating formatting to:
> return Win32ShellFolder2.getShell32Icon(i,
> key.startsWith("shell32LargeIcon ") ? LARGE_ICON_SIZE :
> SMALL_ICON_SIZE);
> or even
> return Win32ShellFolder2.getShell32Icon(i,
> key.startsWith("shell32LargeIcon ") ? LARGE_ICON_SIZE
> :
> SMALL_ICON_SIZE);
> (where : aligns with ?)
> */[Shashi] /*Updated
>
>
> 383 key.startsWith("shell32LargeIcon ")?LARGE_ICON_SIZE :
> SMALL_ICON_SIZE);
> Spaces around ? please.
>
>
> <SNIP>
>
>
> SystemIconTest.java
>
>
> Could you please organize imports?
> There are only three classes used.
>
>
> The imports are usually sorted.
> Is it possible to move java.io.File to the top of the list?
> (Your IDE can do it for you.)
>
>
> Regards,
> Alexey
>
>
>
> 41 System.out.println("Windows detected: will run
> sytem icons test");
> typo: system
>
> Since the test is Windows-specific, it can be declared using
> @requires tag of JTreg:
> @requires os.family == "windows"
> */[Shashi] /*Updated.
>
> Regards,
> Alexey
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/swing-dev/attachments/20190130/b7d09465/attachment-0001.html>
More information about the swing-dev
mailing list