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

String marshalling fix #3

Merged
merged 8 commits into from
Dec 25, 2023

Conversation

ds5678
Copy link
Contributor

@ds5678 ds5678 commented Dec 9, 2023

Hello again. I solved the issue I reported in #2. It was caused by native not being Utf8 on all platforms. I bumped CppSharp to the latest version and ran it on the header for Native File Dialog Extended v1.1.1.

@Speykious
Copy link
Owner

Awesome! Gonna check that everything still works on my machine and stuff

Copy link
Owner

@Speykious Speykious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd gladly accept this change, especially if it means making sure we're using utf-8.
But unfortunately, that change is not gonna work on Linux (and fwiw, on Mac too). :(
On non-Windows platforms, since the native charset is already utf-8, in nativefiledialog-extended, they're defined as a C macro:
https://github.com/Speykious/nativefiledialog-extended/blob/ab1e78eb5462f153d247bad309760d0bd67a6d57/src/include/nfd.h#L266-L273

I think I (or you?)'ll need to slightly modify the CppSharpGenerator so that it changes the names of the functions on non-Windows platforms.

@ds5678
Copy link
Contributor Author

ds5678 commented Dec 9, 2023

If you know how to fix it, go right ahead. You should have push access to my branch.

Otherwise, I'll take a look tomorrow.

@Speykious
Copy link
Owner

Speykious commented Dec 9, 2023

So far I've tried a somewhat naive approach of adding function name replacements (N to U8) in the PostProcess method when not on Windows, and I think it seemed to work up until it was time to find the nfdu8filteritem_t type. I think CppSharp can't find it yet again because it's a #define, but I can't find a function to rename that type...

@ds5678
Copy link
Contributor Author

ds5678 commented Dec 9, 2023

I think the best solution might be to ask the native library author to make the API universal. A few small tweaks upstream and generating interop bindings becomes easier for all frameworks.

@ds5678
Copy link
Contributor Author

ds5678 commented Dec 9, 2023

If we pr that change upstream, the utf8 methods will work out of the box on all 3 platforms.

However, the native methods will have cross platform differences. Currently, the CppSharp output code aligns with Windows wide characters. I propose we add SupportedOSPlatform attributes to the native methods. That would warn developers that they should only use the native methods on Windows.

@ds5678
Copy link
Contributor Author

ds5678 commented Dec 9, 2023

@ds5678
Copy link
Contributor Author

ds5678 commented Dec 9, 2023

@Speykious
Copy link
Owner

Well that was fast :v

@ds5678
Copy link
Contributor Author

ds5678 commented Dec 9, 2023

I also want to pr doxygen support to the native library because that will let CppSharp pull the documentation for our library.

@ds5678
Copy link
Contributor Author

ds5678 commented Dec 9, 2023

I also asked about injecting attributes.

mono/CppSharp#1804

@ds5678
Copy link
Contributor Author

ds5678 commented Dec 12, 2023

I also asked about injecting attributes.

mono/CppSharp#1804

I was able to inject the attributes!

    public void Postprocess(Driver driver, ASTContext ctx)
    {
        foreach (var function in ctx.TranslationUnits.SelectMany(t => t.Functions).Where(f => f.Name.EndsWith('N')))
        {
            AddWindowsOnlyAttribute(function);
        }
        AddWindowsOnlyAttribute(ctx.FindClass("NfdnfilteritemT").First());
    }

    private static void AddWindowsOnlyAttribute(Declaration declaration)
    {
        declaration.Attributes.Add(new Attribute() { Type = typeof(SupportedOSPlatformAttribute), Value = "\"windows\"" });
    }

@ds5678
Copy link
Contributor Author

ds5678 commented Dec 23, 2023

A few updates:

@Speykious
Copy link
Owner

Oh my god... I didn't have any Readme file all this time?? This is bad 💀

@Speykious
Copy link
Owner

I'll update NativeFileDialogs.Net.Runtime now

@ds5678
Copy link
Contributor Author

ds5678 commented Dec 25, 2023

Much appreciated

@Speykious
Copy link
Owner

If you know how to fix it, go right ahead. You should have push access to my branch.

Otherwise, I'll take a look tomorrow.

Well... apparently I don't! D:

❯ git push AssetRipper HEAD:string-marshalling-fix
ERROR: Permission to AssetRipper/NativeFileDialogs.NET.git denied to Speykious.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

@ds5678
Copy link
Contributor Author

ds5678 commented Dec 25, 2023

What were you trying to change? Just bumping the NuGet version?

@Speykious
Copy link
Owner

Speykious commented Dec 25, 2023

Yes, on all packages for NativeFileDialogs.Net.Runtime, and also change the submodule since we don't need my fork anymore.
image

@ds5678
Copy link
Contributor Author

ds5678 commented Dec 25, 2023

I don't think you can push to my branch, probably because I made this from an organization repository.

brave_2023-12-25_10-08-00 brave_2023-12-25_10-09-27

@Speykious
Copy link
Owner

I see. Is there anything you can do in the organization itself then?

@ds5678
Copy link
Contributor Author

ds5678 commented Dec 25, 2023

I added permissions for you to push to that repository, so you should be good now.

@Speykious
Copy link
Owner

Thanks, it worked :D

@Speykious
Copy link
Owner

The examples work on my end with the new versions! I'll do one more test before merging.

@ds5678
Copy link
Contributor Author

ds5678 commented Dec 25, 2023

I noticed that you were referencing the Autogen NuGet package. It's only necessary to reference the Runtime NuGet package due to how native libraries are used.

@Speykious
Copy link
Owner

Oh damn, it has actually never worked on Mac. I'm getting a DllNotFoundException. It can't find the libnfd.dylib in the right place no matter what name I give the runtimes/osx-... directory. I guess that's a separate issue I'll have to pull up.

@Speykious
Copy link
Owner

I noticed that you were referencing the Autogen NuGet package. It's only necessary to reference the Runtime NuGet package due to how native libraries are used.

What do you mean? As in I should turn AutoGen into a project reference instead?

@ds5678
Copy link
Contributor Author

ds5678 commented Dec 25, 2023

613358e

@ds5678
Copy link
Contributor Author

ds5678 commented Dec 25, 2023

Whenever you compile, all the NuGet references will be correct in the generated packages.

@Speykious
Copy link
Owner

Ah right. Yeah that's what you meant. That's fine

@Speykious Speykious merged commit 242034e into Speykious:main Dec 25, 2023
1 check passed
@Speykious Speykious mentioned this pull request Dec 25, 2023
@ds5678 ds5678 deleted the string-marshalling-fix branch June 26, 2024 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants