eclipse-platform / eclipse.platform.swt

Eclipse SWT
https://www.eclipse.org/swt/
Eclipse Public License 2.0
112 stars 134 forks source link

[Windows Hi-DPI] Program icons have black background #715

Closed Phillipus closed 8 months ago

Phillipus commented 1 year ago

Windows 10/11 Eclipse 4.28 Windows hi-dpi scaling at 200%.

Some program icons in Package Explorer and Project Explorer have a black background:

explorer

Sometimes, the icons render correctly.

I can't reproduce this with a stand-alone SWT snippet so it seems to suggest that it's caused by Eclipse doing things in a certain order, or something else affects it in the UI.

See also https://github.com/eclipse-platform/eclipse.platform.swt/issues/185

deepika-u commented 1 year ago

Hi @Phillipus, For me this looks to be similar issue reported in pr #409. Can you please review that issue if that might give you some clue towards this problem. Thanks

Phillipus commented 1 year ago

Hi @Phillipus, For me this looks to be similar issue reported in pr #409. Can you please review that issue if that might give you some clue towards this problem. Thanks

Hi @deepika-u I tested running a child Eclipse with the SWT PR (and the PR in Platform UI) but, unfortunately, the program icons still have a black background.

I think it's related to https://github.com/eclipse-platform/eclipse.platform.swt/issues/185

Phillipus commented 1 year ago

I think it's related to #185

Actually, it's not a recent regression. The problem exists in Eclipse 4.8 (June 2018)

merks commented 1 year ago

Yes, this is quite an old problem. BTW, I think these come from org.eclipse.swt.program.Program.getPrograms() and from there via org.eclipse.swt.program.Program.getImageData() which works it's way tout of org.eclipse.ui.internal.misc.ExternalProgramImageDescriptor.getImageData() eventually. Unfortunately org.eclipse.swt.tests.win32.snippets.SWTIssue185.createImages(Display, String...) does not not reproduce the problem.

Phillipus commented 1 year ago

....does not not reproduce the problem.

And I can't reproduce it in a stand-alone SWT snippet. And the infuriating thing is (in terms of debugging) is that sometimes the icons render OK!

Phillipus commented 1 year ago

In the org.eclipse.swt.program.Program class starting at line 363:

public ImageData getImageData () {
    if (extension != null) {
        SHFILEINFO shfi = new SHFILEINFO ();
        int flags = OS.SHGFI_ICON | OS.SHGFI_SMALLICON | OS.SHGFI_USEFILEATTRIBUTES;
...

If I change line 366 to one of the following the icon is displayed correctly:

int flags = OS.SHGFI_SMALLICON | OS.SHGFI_USEFILEATTRIBUTES;
int flags = OS.SHGFI_ICON | OS.SHGFI_SMALLICON;
int flags = OS.SHGFI_SMALLICON;

I've no idea what those flags mean or why this should be the case, but maybe a clue there?

Phillipus commented 1 year ago

In fact, if I comment out all of this, it works:

if (extension != null) {
    SHFILEINFO shfi = new SHFILEINFO ();
    int flags = OS.SHGFI_ICON | OS.SHGFI_SMALLICON | OS.SHGFI_USEFILEATTRIBUTES;
    TCHAR pszPath = new TCHAR (0, extension, true);
    OS.SHGetFileInfo (pszPath.chars, OS.FILE_ATTRIBUTE_NORMAL, shfi, SHFILEINFO.sizeof, flags);
    if (shfi.hIcon != 0) {
        Image image = Image.win32_new (null, SWT.ICON, shfi.hIcon);
        /* Fetch the ImageData at 100% zoom and return */
        ImageData imageData = image.getImageData ();
        image.dispose ();
        return imageData;
    }
}
deepika-u commented 1 year ago

Hi, I tried the below images in the snippet. Please check if you are able to recreate the problem with this snippet.

package org.eclipse.swt.bugs;
import org.eclipse.swt.*;
import org.eclipse.swt.graphics.*;
import org.eclipse.swt.layout.*;
import org.eclipse.swt.widgets.*;

public class SWTIconExample {

    public static void main(String[] args) {
        Display display = new Display();
        Shell shell = new Shell(display);
        shell.setText("Excel and Chrome Icons");
        shell.setLayout(new GridLayout(2, false));

        // Load the Excel icon
        Image excelIcon = new Image(display, "C:\\Users\\02236C744\\Desktop\\zoom_icons\\ExcelLogoSmall.scale-100.png");
        // Load the Chrome icon
        Image chromeIcon = new Image(display, "C:\\Users\\02236C744\\Desktop\\zoom_icons\\chrome.png");
        // Load the Firefox icon
        Image firefoxIcon = new Image(display, "C:\\Users\\02236C744\\Desktop\\zoom_icons\\VisualElements_70.png");
        // Load the Edge icon
        Image edgeIcon = new Image(display, "C:\\Users\\02236C744\\Desktop\\zoom_icons\\edge.png");
        // Load the MSWord icon
        Image mswordIcon = new Image(display, "C:\\Users\\02236C744\\Desktop\\zoom_icons\\WinWordLogoSmall.scale-100.png");

        // Label to display the Excel icon
        Label excelLabel = new Label(shell, SWT.NONE);
        excelLabel.setImage(excelIcon);
        excelLabel.setLayoutData(new GridData(SWT.CENTER, SWT.CENTER, true, true));

        // Label to display the Chrome icon
        Label chromeLabel = new Label(shell, SWT.NONE);
        chromeLabel.setImage(chromeIcon);
        chromeLabel.setLayoutData(new GridData(SWT.CENTER, SWT.CENTER, true, true));

        // Label to display the Firefox icon
        Label firefoxLabel = new Label(shell, SWT.NONE);
        firefoxLabel.setImage(firefoxIcon);
        firefoxLabel.setLayoutData(new GridData(SWT.CENTER, SWT.CENTER, true, true));

        // Label to display the Edge icon
        Label edgeLabel = new Label(shell, SWT.NONE);
        edgeLabel.setImage(edgeIcon);
        edgeLabel.setLayoutData(new GridData(SWT.CENTER, SWT.CENTER, true, true));

        // Label to display the MSWord icon
        Label mswordLabel = new Label(shell, SWT.NONE);
        mswordLabel.setImage(mswordIcon);
        mswordLabel.setLayoutData(new GridData(SWT.CENTER, SWT.CENTER, true, true));

        shell.pack();
        shell.open();

        while (!shell.isDisposed()) {
            if (!display.readAndDispatch()) {
                display.sleep();
            }
        }

        // Remember to dispose of the images
        excelIcon.dispose();
        chromeIcon.dispose();
        firefoxIcon.dispose();
        edgeIcon.dispose();
        mswordIcon.dispose();
        display.dispose();
    }
}

I have picked the images at that path from below locations already existing in my windows 11 instance. firefox => C:\Program Files\Mozilla Firefox\browser\VisualElements excel, word => C:\Program Files\Microsoft Office\root\Office16\LogoImages chrome, edge => C:\Program Files\Microsoft Office\root\Office16\sdxs\FA000000070\assets\src\assets\images edge => C:\Program Files (x86)\Microsoft\Edge\Application\115.0.1901.203\VisualElements

Let me know if anyone is able to recreate the issue atleast i am not successful.

Phillipus commented 1 year ago

@deepika-u I can't reproduce this with a stand-alone snippet. As mentioned before it seems to depend on the order of events.

deepika-u commented 9 months ago

@Phillipus Can you please check below snippet

package org.eclipse.swt.bugs3;
import java.util.*;
import java.util.List;

import org.eclipse.swt.*;
import org.eclipse.swt.graphics.*;
import org.eclipse.swt.program.*;
import org.eclipse.swt.widgets.*;

public class ProgramImageTest {

    public static void main(String[] args) {
        final Display display = new Display();

        final List<Image> images = createImages(display, "svg", "shtml", "html", "xlsx", "docx", "zip", "pdf", "png", "jpg", "msi", "txt", "theme");

        final Shell shell = new Shell(display);

        shell.addListener(SWT.Paint, event -> {
            int x = 0;
            for (Image image : images) {
                event.gc.drawImage(image, x, 0);
                x += image.getImageData().width;
                x += 10;
            }
        });

        shell.setSize(400, 300);
        shell.open();

        while (!shell.isDisposed()) {
            if (!display.readAndDispatch()) {
                display.sleep();
            }
        }

        for (Image image : images) {
            image.dispose();
        }

        display.dispose();
    }

    private static List<Image> createImages(Display display, String... extensions) {
        final List<Image> images = new ArrayList<>();
        for (String extension : extensions) {
            final Program program = Program.findProgram(extension);
            if (program == null) {
                continue;
            }

            final ImageData data = program.getImageData();
            if (data == null) {
                continue;
            }

            images.add(new Image(display, data));
        }
        return images;
    }
}

Hope this should help you recreate the problem.

Phillipus commented 9 months ago

@deepika-u This is the result of the Snippet. It doesn't recreate the problem.

Image 001

deepika-u commented 9 months ago

@deepika-u This is the result of the Snippet. It doesn't recreate the problem.

@Phillipus : have you got to try with 200% resolution? is it the same? sorry to ask you again.

Phillipus commented 9 months ago

Yes, that's at 200%.

HeikoKlare commented 8 months ago

I have debugged this issue and found the following:

The black background is a result of the icon's mask data being empty, even though there actually is mask data when loading the image under different conditions where the background is transparent, as expected. This is the according source code line retrieving the mask data: https://github.com/eclipse-platform/eclipse.platform.swt/blob/085376c0874dad2d6e757ceaf4d94e513b13400f/bundles/org.eclipse.swt/Eclipse%20SWT/win32/org/eclipse/swt/graphics/Image.java#L1460

In my experiments, the first time that you start an application after you have changed the primary monitor scaling, the images are rendered fine. From the second start on, the mask data will be missing. I could not yet figure out why that is the case. Anyway, the problem with missing mask data is reproducible. It occurs once a display is instantiated, so if you retrieve a program icon after a display has been instantiated, the mask data will be empty. This happens precisely because of the OLE initialization: https://github.com/eclipse-platform/eclipse.platform.swt/blob/085376c0874dad2d6e757ceaf4d94e513b13400f/bundles/org.eclipse.swt/Eclipse%20SWT/win32/org/eclipse/swt/widgets/Display.java#L2837 If you comment out that line and retrieve the icon after display instantiation, it will have proper mask data.

To reproduce the behavior, you can use the following snippet and just put a breakpoint into the Image class in the line retrieving the mask data (as referenced above) and inspect the maskData:

new Display();
Program program = new Program();
program.extension = ".xlsx"; // or any other extension for which a program with a proper icon is registered
program.getImageData();

I currently have no clue why the mask data is missing, but actually the images contain the required data as alpha values in the usual data array. In #998, I have proposed a fix that considers the alpha values when present instead of the mask data. At least for the program icons, that fix seems to work fine and have not experienced any unintended side effects so far.

HeikoKlare commented 8 months ago

If I change line 366 to one of the following the icon is displayed correctly:

int flags = OS.SHGFI_SMALLICON | OS.SHGFI_USEFILEATTRIBUTES;
int flags = OS.SHGFI_ICON | OS.SHGFI_SMALLICON;
int flags = OS.SHGFI_SMALLICON;

I've no idea what those flags mean or why this should be the case, but maybe a clue there?

fyi: all these flags are necessary, see https://learn.microsoft.com/en-US/windows/win32/api/shellapi/nf-shellapi-shgetfileinfoa The icon is just displayed correctly when changing them because the icon is not properly resolved using the file extension anymore, such that the code uses the "fallback" to resolve the icon via the program icon path rather than the file extension.

Phillipus commented 8 months ago

Hi @HeikoKlare thanks for taking a look at this.

I tested PR #998 on my Windows machine with hi-dpi monitor at 200% scaling and it fixes the problem!

As I can't reproduce the problem with a stand-alone SWT snippet I launched a child Eclipse instance and created a project with some empty files.

Before the patch:

before

After the patch:

after

I don't know if there are any side-effects from this, I didn't test beyond this.

HeikoKlare commented 8 months ago

Thanks for testing, @Phillipus!

Just for the record: I debugged a bit more and found that the retrival of the icon handle for the file extension is what makes the access to the icon on the next application start have an empty mask. More precisely, if you start a program that only executes the icon handle retrieval, i.e., the following line, https://github.com/eclipse-platform/eclipse.platform.swt/blob/085376c0874dad2d6e757ceaf4d94e513b13400f/bundles/org.eclipse.swt/Eclipse%20SWT%20Program/win32/org/eclipse/swt/program/Program.java#L368 then subsequent programs will load this icon without mask data.

I could not figure out why this is the case. From my understanding of the Windows API documentation, a DestroyIcon call is missing here, but even with that call the issue remains.

Anyway, the usage of the file extension to get the icon seems seems to be a kind of unnecessary "shortcut (introduced more than 20 years ago, so maybe a performance optimization at that point in time), as @Phillipus has already found in https://github.com/eclipse-platform/eclipse.platform.swt/issues/715#issuecomment-1616584635. Since I am in favor of reducing code instead of adding another tweak for uncomprehensible behavior, I would propose to simply remove this shortcut logic. It does not exist in the other OS's Program implementation and seems to only add (currently) buggy code without giving a benefit. So I have propose this removal as an alternative to #998 in #999.

Phillipus commented 8 months ago

@HeikoKlare Testing #999 and that works. As you say, it's probably better to change the code in Program rather than Image.

In Program line 183 no longer needs the extension parameter:

static Program getProgram (String key, String extension)

Could that be removed?

deepika-u commented 8 months ago

At 200% i am seeing below Before patch 998, image

After patch 998, image

I see similar behavior in your zip icon as in 999. But let me also try with pr 999.

Phillipus commented 8 months ago

I don't understand why we can't reproduce the black backgrounds in a stand-alone test snippet.

deepika-u commented 8 months ago

At 200% i am seeing below Before patch 999, image

After patch 999, image

From view perspective i dont see much difference between 998 and 999 from a user perspective.

HeikoKlare commented 8 months ago

In Program line 183 no longer needs the extension parameter:

static Program getProgram (String key, String extension)

Could that be removed?

Yes, thanks for the hint! I've updated the PR.

I don't understand why we can't reproduce the black backgrounds in a stand-alone test snippet.

I am also not sure about that, but if you debug into the ImageData initialization in a standalone snippet, you can see that the mask data is erroneous there as well (after second start of the snippet with the same monitor scale value).

From view perspective i dont see much difference between 998 and 999 from a user perspective.

That's great. Actually the result of both solutions should be same (and in case there are different, probably the alpha value extraction in #998 is not completely correct).

HeikoKlare commented 8 months ago

I don't understand why we can't reproduce the black backgrounds in a stand-alone test snippet.

I guess the reason is that "ordinary" image drawring on a GC does the same as my patch in #998: If no valid mask data is present, it just uses the alpha values. However, in the package explorer (and at several other places) a special JFace draw functionality is used that treats mask and alpha data differently. In particular, if mask data is present (even if empty), it is written into an alpha mask used for drawing (see https://github.com/eclipse-platform/eclipse.platform.ui/blob/8c94f2a3a896a278d0cadebc7d06702e255e3a46/bundles/org.eclipse.jface/src/org/eclipse/jface/resource/CompositeImageDescriptor.java#L265):

if (src.maskData != null) {
    srcMask = src.getTransparencyMask ();
    if (src.depth == 32) {
        alphaMask = ~(srcPalette.redMask | srcPalette.greenMask | srcPalette.blueMask);
        while (alphaMask != 0 && ((alphaMask >>> alphaShift) & 1) == 0) alphaShift++;
    }
}

Even though that code could be changed as well, simplifying the icon loading in Program has less risk of causing side effect and is "correct" anyway, because the loaded mask data is inconsistent (as explained above).

Phillipus commented 8 months ago

@HeikoKlare Yes, I agree that the changes in Program class only are better than changing Image. Given that this works, what'e the next stage? More testing?

HeikoKlare commented 8 months ago

I am not sure in which direction tests could/should be performed. The only regression I could image is that for some unknown reason an icon can be resolved via the file extension but not via the icon name (i.e., its path). I have verified that in case this situation occurs, at least nothing breaks. In particular, when I change the icon name to something unresolvable (I did program.iconName = iconName.replace("C:", "D:") in findProgram(String)), the default icon is simply used: image

So I expect that the worst that could happen are program icons missing that are currently present. Since this is not the case for several icons I have tested so far and since there is no systematic way of identifying in which cases that might happen, I would propose to simply merge the removal and see if someone reports missing icons, so that we can precisely analyze that case. The change provides a clear benefit and is unlikely to produce a regression (and in case it does, it will not be severe).

Phillipus commented 8 months ago

@HeikoKlare Agree!

deepika-u commented 8 months ago

I have retested with pr 999

When no patch is applied -> image

When pr 999 is applied -> image

I am good to go for this fix. As a next step in the same context, we might have to check #409 once this is merged.

HeikoKlare commented 8 months ago

Thanks, @deepika-u! I'm already taking a look at #409: there only seems to be a simple change required to get that working properly.

Phillipus commented 8 months ago

Merged now. Many thanks @HeikoKlare !

tmssngr commented 6 months ago

The commit 97ca656d broke the generic file icon on Windows for .exe files. Here is the snippet to reproduce:

import org.eclipse.swt.*;
import org.eclipse.swt.graphics.*;
import org.eclipse.swt.program.*;
import org.eclipse.swt.widgets.*;

public class ProgramIcons {

    public static void main(String[] args) {
        final Display display = new Display();

        final Program exeProgram = Program.findProgram(".exe");
        final ImageData imageData = exeProgram.getImageData();
        final Image image = new Image(display, imageData);

        final Shell shell = new Shell(display);
        shell.addListener(SWT.Paint, event -> {
            event.gc.drawImage(image, 0, 0);
        });

        shell.setSize(400, 300);
        shell.open();

        while (!shell.isDisposed()) {
            if (!display.readAndDispatch()) {
                display.sleep();
            }
        }

        image.dispose();

        display.dispose();
    }
}

Reported as bug #1130.