Re: RFR: 8258979: The image didn't show correctly with GTK LAF [v3]
On Wed, 10 Jan 2024 08:55:19 GMT, Prasanta Sadhukhan wrote: >> Should I merge the two classes into one class like instead of separate >> `ExpandedIconWrapper` and `CollapsedIconWrapper`, only one `IconWrapper` >> class? > > guess you could since they are all private.. Merged `ExpandedIconWrapper` and `CollapsedIconWrapper` class to `IconWrapper ` class. - PR Review Comment: https://git.openjdk.org/jdk/pull/17294#discussion_r1447202641
Re: RFR: 8258979: The image didn't show correctly with GTK LAF [v3]
On Wed, 10 Jan 2024 08:25:52 GMT, Abhishek Kumar wrote:
>> src/java.desktop/share/classes/javax/swing/plaf/synth/SynthTreeUI.java line
>> 837:
>>
>>> 835: }
>>> 836: else {
>>> 837: SynthGraphicsUtils.paintIcon(collapsedIcon, context,
>>> g, x, y, w, h);
>>
>> Guess it's a copy of `ExpandedIconWrapper `class, so `CollapsedIconWrapper
>> `and `ExpandedIconWrapper` can be optimised to use common method passing in
>> the "icon" argument..
>
> Should I merge the two classes into one class like instead of separate
> `ExpandedIconWrapper` and `CollapsedIconWrapper`, only one `IconWrapper`
> class?
guess you could since they are all private..
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17294#discussion_r1447064941
Re: RFR: 8258979: The image didn't show correctly with GTK LAF [v3]
On Wed, 10 Jan 2024 05:56:37 GMT, Prasanta Sadhukhan
wrote:
>> Abhishek Kumar has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove whitespace error
>
> src/java.desktop/share/classes/javax/swing/plaf/synth/SynthTreeUI.java line
> 837:
>
>> 835: }
>> 836: else {
>> 837: SynthGraphicsUtils.paintIcon(collapsedIcon, context, g,
>> x, y, w, h);
>
> Guess it's a copy of `ExpandedIconWrapper `class, so `CollapsedIconWrapper
> `and `ExpandedIconWrapper` can be optimised to use common method passing in
> the "icon" argument..
Should I merge the two classes into one class like instead of separate
`ExpandedIconWrapper` and `CollapsedIconWrapper`, only one `IconWrapper` class?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17294#discussion_r1447033548
Re: RFR: 8258979: The image didn't show correctly with GTK LAF [v3]
On Wed, 10 Jan 2024 05:00:33 GMT, Abhishek Kumar wrote:
>> The collapsed icon for JTree is not painted using `Icon.paintIcon(Component
>> c, Graphics g, int x, int y)` in GTK LAF. The collapsed icon is returned
>> from BasicTreeUI class which doesn't contain any icon image whereas the
>> expanded icon is returned from SynthTreeUI class and expanded icon is
>> rendered correctly.
>> The proposed fix is to return collapsed icon as an object of collapsed icon
>> wrapper which implements synthIcon and is similar to the expanded icon
>> implementation.
>>
>> Test mentioned in JBS is an applet based which is converted to main based
>> now and extended for all installed LAFs on the system.
>>
>> No regression caused with the fix, link is attached in JBS .
>
> Abhishek Kumar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove whitespace error
src/java.desktop/share/classes/javax/swing/plaf/synth/SynthTreeUI.java line 837:
> 835: }
> 836: else {
> 837: SynthGraphicsUtils.paintIcon(collapsedIcon, context, g,
> x, y, w, h);
Guess it's a copy of `ExpandedIconWrapper `class, so `CollapsedIconWrapper `and
`ExpandedIconWrapper` can be optimised to use common method passing in the
"icon" argument..
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17294#discussion_r1446917678
Re: RFR: 8258979: The image didn't show correctly with GTK LAF [v3]
> The collapsed icon for JTree is not painted using `Icon.paintIcon(Component > c, Graphics g, int x, int y)` in GTK LAF. The collapsed icon is returned from > BasicTreeUI class which doesn't contain any icon image whereas the expanded > icon is returned from SynthTreeUI class and expanded icon is rendered > correctly. > The proposed fix is to return collapsed icon as an object of collapsed icon > wrapper which implements synthIcon and is similar to the expanded icon > implementation. > > Test mentioned in JBS is an applet based which is converted to main based now > and extended for all installed LAFs on the system. > > No regression caused with the fix, link is attached in JBS . Abhishek Kumar has updated the pull request incrementally with one additional commit since the last revision: Remove whitespace error - Changes: - all: https://git.openjdk.org/jdk/pull/17294/files - new: https://git.openjdk.org/jdk/pull/17294/files/0702e42a..ed5caa98 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17294=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17294=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17294.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17294/head:pull/17294 PR: https://git.openjdk.org/jdk/pull/17294
