Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows GUI: Fix installer out path after assets #979

Merged
merged 1 commit into from
Nov 30, 2024

Conversation

cjee21
Copy link
Collaborator

@cjee21 cjee21 commented Nov 29, 2024

@JeromeMartinez
Copy link
Member

@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 30, 2024

Shouldn't it be added also to https://github.com/MediaArea/MediaInfo/blob/master/Source/Install/MediaInfo_GUI_Windows_x64.nsi#L156 ?

Not needed because there is immediately a SetOutPath at

SetOutPath "$INSTDIR\Plugin\Custom"
before any further file operations. This difference is why I missed it in the combined installer.

@JeromeMartinez
Copy link
Member

I prefer to normalize the order of lines between installers so we don't miss such issue in the future.
I force pushed the removal of your "hot fix" and the reordering of lines and also the the horizontal position of the lines depending on the arch, so it is easier to do a diff between the installer files.

@JeromeMartinez
Copy link
Member

By the way, how/where do you see the impact of the fix? In practice I don't see a difference on Win11 (I am very new to this OS, so I may miss a lot of things).

@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 30, 2024

I force pushed the removal of your "hot fix"

Can just remove me as author then won't have the unverified tag.

By the way, how/where do you see the impact of the fix? In practice I don't see a difference on Win11 (I am very new to this OS, so I may miss a lot of things).

Before this fix, some files were wrongly placed in Assets folder. The files in install folder should be as follows:

>tree /f "C:\Program Files\MediaInfo"
C:\PROGRAM FILES\MEDIAINFO
│   History.txt
│   LIBCURL.DLL
│   License.html
│   MediaInfo.dll
│   MediaInfo.exe
│   MediaInfo.url
│   MediaInfo_InfoTip.dll
│   MediaInfo_PackageHelper.dll
│   MediaInfo_SparsePackage.msix
│   MediaInfo_WindowsShellExtension.dll
│   ReadMe.txt
│   resources.pri
│   uninst.exe
│   WebView2Loader.dll
│
├───Assets
│       Square150x150Logo.scale-400.png
│       Square44x44Logo.altform-unplated_scale-400.png
│       Square44x44Logo.scale-400.png
│
└───Plugin
    ├───Custom
    │       en.Example.csv
    │       en.Example_HTML.csv
    │       Example.csv
    │       Example_HTML.csv
    │       fr.Example.csv
    │       it.Esempio.csv
    │       it.Esempio_HTML.csv
    │       Table by fields, compact (HTML).csv
    │       Table by fields, short (HTML).csv
    │       Table by fields, standard (HTML).csv
    │       Table by fields, verbose (HTML).csv
    │       Table by streams, compact (HTML).csv
    │       Table by streams, short (HTML).csv
    │       Table by streams, standard (HTML).csv
    │       Table by streams, verbose (HTML).csv
    │       XML.csv
    │       zzz_Contrib - Dusil (HTML).csv
    │
    ├───Language
    │       ar.csv
    │       be.csv
    │       bg.csv
    │       ca.csv
    │       cs.csv
    │       da.csv
    │       de.csv
    │       en.csv
    │       es.csv
    │       eu.csv
    │       fa.csv
    │       fr.csv
    │       gl.csv
    │       gr.csv
    │       hr.csv
    │       hu.csv
    │       hy.csv
    │       id.csv
    │       it.csv
    │       ja.csv
    │       ka.csv
    │       ko.csv
    │       lt.csv
    │       nl.csv
    │       pl.csv
    │       pt-BR.csv
    │       pt.csv
    │       ro.csv
    │       ru.csv
    │       sk.csv
    │       sq.csv
    │       sv.csv
    │       th.csv
    │       tr.csv
    │       uk.csv
    │       zh-CN.csv
    │       zh-HK.csv
    │       zh-TW.csv
    │
    ├───Sheet
    │       Example.csv
    │       Export example.csv
    │
    └───Tree
            Example.csv

@JeromeMartinez
Copy link
Member

Before this fix, some files were wrongly placed in Assets folder. The files in install folder should be as follows:

Sorry, I wanted to mean what is the impact on not having the asset found by the DLL?

@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 30, 2024

Before this fix, some files were wrongly placed in Assets folder. The files in install folder should be as follows:

Sorry, I wanted to mean what is the impact on not having the asset found by the DLL?

The PNG images are referenced by the MSIX. Without them, the taskbar icon will be missing.

@JeromeMartinez
Copy link
Member

The PNG images are referenced by the MSIX. Without them, the taskbar icon will be missing.

I missed something. Here it is with the NSI installer, I don't see about which MSIX you are talking about.

@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 30, 2024

The PNG images are referenced by the MSIX. Without them, the taskbar icon will be missing.

I missed something. Here it is with the NSI installer, I don't see about which MSIX you are talking about.

The XML manifest inside MediaInfo_SparsePackage.msix references the PNGs in the Assets folder.

@JeromeMartinez
Copy link
Member

The XML manifest inside MediaInfo_SparsePackage.msix references the PNGs in the Assets folder.

And what is the impact?
I use the buggy installer and MediaInfo icon is seen in the taskbar (as in the past without the sparse package).

@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 30, 2024

And what is the impact? I use the buggy installer and MediaInfo icon is seen in the taskbar (as in the past without the sparse package).

The impact is as I mentioned in #979 (comment). Some files are wrongly placed in the wrong place that's all. No change/effect in functionality. The only way to detect the buggy installer is to inspect the install directory.

edit: the wrongly placed files are not the PNGs so nothing is broken there.

@JeromeMartinez
Copy link
Member

Some files are wrongly placed in the wrong place that's all. No change/effect in functionality.

I don't get it: if no change/effect, why installing them? We can just not install them (and remove the reference from the MSIX).
I guess that you add them for a reason, I try to understand how they are used.

@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 30, 2024

I don't get it: if no change/effect, why installing them? We can just not install them (and remove the reference from the MSIX). I guess that you add them for a reason, I try to understand how they are used.

See my edit. The wrongly placed files are not the PNGs or any of the new files that are used by the Windows 11 context menu. So there is no change in functionality there. I mean the fix of installer has no effect on functionality not that the files we are installing has no effect.

Unrelated to this fix, the MSIX has to reference the PNGs to be valid. The PNGs have to be present for the taskbar icon.

@JeromeMartinez
Copy link
Member

The PNGs have to be present for the taskbar icon.

Oops, right, I was thinking the wrong way.
OK, I reformulate my question: before the sparse package, the task bar was using the MediaInfo binary icon, does it mean that now the PNG is used?
A related question: what is the actual impact of having the sparse package? My understanding of your previous comments is that it shows something as more win11 compatible, how to see it?

@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 30, 2024

before the sparse package, the task bar was using the MediaInfo binary icon, does it mean that now the PNG is used?

Yes that is what I found out. When there is a package installed, it behaves like a MSIX packaged app so the AppxManifest and PNGs are used instead of binary icon. Since it is a sparse package, the Assets are in the install directory instead of in the MSIX itself. I have seen other applications packaging the Assets in MSIX instead which results in missing taskbar icon as well as uselessly large MSIX file.

A related question: what is the actual impact of having the sparse package?

The main purpose of the sparse package here is to gain access to the new modern context menu. You can see the new DLL is referenced in the AppxManifest along with all the file extensions we want to register.

In general, the sparse package grants an application package identity. This enables an unpackaged app to use features that require package identity which are traditionally only accessibly by packaged apps. Other features that require package identity other than the new context menu can be found here: https://learn.microsoft.com/en-us/windows/apps/desktop/modernize/modernize-packaged-apps

how to see it?

If you see Task Manager or Process Explorer and enable the package name column, you will see that MediaInfo has a package name when the sparse package is installed. That is package identity. Sparse package makes the app more like a MSIX/Store packaged app.

Can also do powershell -Command "Get-AppxPackage *MediaInfo*" to see the installed package.

@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 30, 2024

Snapshot 20241130-2 looks good.

@JeromeMartinez
Copy link
Member

The main purpose of the sparse package here is to gain access to the new modern context menu.

This is what I was missing, thank you for the explanation.
(I remind now that you already explained it but I forgot it, it is so weird to have to add a PNG for having what we were having without the MSIX but we need the MSIX for acceding the context menu)

@JeromeMartinez JeromeMartinez merged commit 4ace636 into MediaArea:master Nov 30, 2024
3 checks passed
@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 30, 2024

@JeromeMartinez side note: Probably can replace MediaInfo UWP store version with a packaged VCL or Qt one so that achieve feature parity and no need to maintain so many different code. If not mistaken some of Microsoft's updated pre-installed apps from the Store are not using UWP anymore.

@cjee21 cjee21 deleted the installer-fix branch November 30, 2024 13:45
@JeromeMartinez
Copy link
Member

Probably can replace MediaInfo UWP store version with a packaged VCL or Qt one so that achieve feature parity and no need to maintain so many different code.

We created the UWP version for having a presence on the Windows App Store before they accept Win32 apps. We need to think about if the UWP version still have a value added (UI more "Windows"? Well, it seems that people don't really see a Windows style), if not we may switch to the VCL (for the moment) one.

@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 30, 2024

UWP version still have a value added (UI more "Windows"? Well, it seems that people don't really see a Windows style)

From what I see, UWP is abandoned by Microsoft themselves. Microsoft's own apps are mostly updated to use WinUI3. That is the new 'Windows style' for Windows 11 with Mica material. Microsoft now encourages use of Windows App SDK/WinUI or WPF as development platform. There is also a guide from Microsoft to migrate from UWP to Windows App SDK.

@JeromeMartinez
Copy link
Member

From what I see, UWP is abandoned by Microsoft themselves.

Not stable... But right, maybe time for us to move from UWP and push the VCL version on the Windows Store. But I guess this will not be easy to move, but having the Win11 option also on the Windows Store is needed, I'll check with @g-maxime when we have time for that.

@cjee21
Copy link
Collaborator Author

cjee21 commented Nov 30, 2024

But I guess this will not be easy to move, but having the Win11 option also on the Windows Store is needed

I never published apps to store but a rough idea I have is to modify the sparse package manifest to turn it into a fully packaged win32 app. Or modify the Qt MSIX manifest for the VCL one. Then the graph/ffmpeg plugins can probably be published to store as a dependent but optional package if not wanting to bundle everything together. Something like what I done for the Qt MSIX should be publishable to Store with working context menu. Not 100% sure it will work since I never published to Store.

@JeromeMartinez
Copy link
Member

Then the graph/ffmpeg plugins can probably be published to store as a dependent but optional package if not wanting to bundle everything together.

People using the Store don't really care about package sizes, so all in one there :).

Something like what I done for the Qt MSIX should be publishable to Store with working context menu.

We will try...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants