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

Add _ScintillaManagedDragDrop & improve error messages #114

Merged
merged 2 commits into from
May 6, 2024

Conversation

ahmetsait
Copy link
Collaborator

@ahmetsait ahmetsait commented Apr 17, 2024

  • Add _ScintillaManagedDragDrop for making RevokeDragDrop() optional
  • Generate better error message when satellite libs are not found

@desjarlais
Copy link
Owner

I like the drag/drop addition, it will take some time to review the satellite library changes.

@desjarlais
Copy link
Owner

I'm getting errors at design time with these changes and unless I'm missing something, the current way of doing it will throw with the basepath that failed and using what you have suggested here will just show a list of paths. How would this help improve the errors?

@ahmetsait
Copy link
Collaborator Author

There are no changes to how satellite libraries are resolved in this PR. It improves the error message by showing the list of paths that were searched instead of just showing the last one.

I suspect the designer error might be caused by you changing the version from v5.3.3.18 to something else (e.g. v5.3.3.19) which made the designer unable to find the folder %USERPROFILE%\.nuget\packages\scintilla5.net\5.3.3.19\build\x64. This is something I've also experienced when I added Scintilla.NET as a source project.

@desjarlais
Copy link
Owner

The path checks are fairly linear logically, so it keeps checking until a good path is able to be returned.

What is the value in knowing the full list, don't you just need the last one that failed? Knowing that other paths were not checked seems to not have a lot of value, but I might be missing something.

I do like the idea of better logging, but I would like to flesh out this idea a little more before checking anything in.

@ahmetsait
Copy link
Collaborator Author

ahmetsait commented Apr 21, 2024

There is no point in arguing in favor of the current error message whatsoever. It makes the user think that ScintillaNET checks a single path.

You see, when the designer fails with:
System.InvalidOperationException: Unable to locate the Scintilla.NET satellite assemblies : directory '%LOCALAPPDATA%\Microsoft\VisualStudio\packages\Scintilla.NET.5.3.3.18\build\x64' not found and the %LOCALAPPDATA%\Microsoft\VisualStudio\packages directory only has _ChannelFeeds, _Channels, _Instances, _LatestInstaller, _RemoteChannels folders, it gives no clue about why ScintillaNET even looks at this path.

If it instead gave the following error:

System.InvalidOperationException: Scintilla.NET satellite assemblies not found in any of the following paths:
%LOCALAPPDATA%\Microsoft\VisualStudio\17.0_e67b7181\ProjectAssemblies\tzktqa41.cdy01\x64
%USERPROFILE%\.nuget\packages\scintilla5.net\5.3.3.18\build\x64
%LOCALAPPDATA%\Microsoft\VisualStudio\packages\Scintilla.NET.5.3.3.18\build\x64

I would be able understand the issue immediately because %USERPROFILE%\.nuget\packages actually contains all the downloaded packages including the scintilla5.net folder.

@ahmetsait
Copy link
Collaborator Author

Ping @desjarlais

@desjarlais
Copy link
Owner

I'm still looking into adding the logging and figuring out why it is failing on my machine.

@desjarlais
Copy link
Owner

.NET seems to work fine, but .NET Framework is giving me problems. When I try to add the control, it gives the error with no searchedPaths.

I'm currently testing a StringBuilder field and each path gets added to the StringBuilder and is thrown instead of the last basePath. I think it is a similar idea to what you are saying here. So far it is working in .NET and .NET Framework. I'm going to try one more time getting this particular PR to work with .NET Framework. Have you tested what you have against .NET Framework apps?

@ahmetsait
Copy link
Collaborator Author

I don't understand why or how you're running into issues, I'm using this exact code in production in a .Net Framework 4.8.1 app. VS WinForms designer works just fine if I put the native DLLs under %USERPROFILE%\.nuget\packages\scintilla5.net\<ScintillaNET-Package-Version>\build\x64.

I was hoping you would merge this and publish a package soon.

@desjarlais
Copy link
Owner

I can skip the testing and debugging I'm doing to see what causes the issue in the .NET Framework if you are willing to let me assign any new bugs with loading the satellite dll's to you after this is checked in as is?

@ahmetsait
Copy link
Collaborator Author

How about publishing a beta package with these changes first? I can test it as a nuget pkg to see if there is some obvious issue.

@desjarlais
Copy link
Owner

I'm attaching version 21 with your changes, test it out and let me know how things go on your end.

Scintilla5.NET.5.3.3.21.zip

@ahmetsait
Copy link
Collaborator Author

ahmetsait commented May 2, 2024

I tested my changes using a local NuGet source with both fresh .NET Framework 4.8 & .NET 6 WinForms projects and everything seems to be in order.

Additional findings on my tests:

  • SDK style projects use <PackageReference> by default, in which case NuGet packages are placed into user-global NuGet package folder (~/.nuget/packages).
  • Legacy project format use packages.config by default, in which case NuGet packages are placed into a packages folder alongside the solution file. This in turn makes it impossible to get VS Designer to work without manually placing DLLs. Thankfully it is possible to use <PackageReference> in legacy project format as well so we can just tell users to migrate.
  • Legacy project format doesn't respect *.targets from NuGet packages so native DLLs are not automatically included, thus not copied to the output directory without manual user intervention. Looks like it does actually copy satellite DLLs despite not showing up inside the Solution Explorer.
  • VS Designer silently ignores exceptions if the project targets .NET 5+, and just shows controls that don't throw an exception. This is the primary cause of Scintilla control not visible on form designer #95.

@desjarlais
Copy link
Owner

I don't see the issue with .NetFramework 4.8 either, but it looks like I was seeing it because my default was 4.7.2 and those projects seem to cause the problem:

scintillaerrordragdrop

@ahmetsait
Copy link
Collaborator Author

@desjarlais can you attach a small repro project?

@desjarlais
Copy link
Owner

I'll attach one soon, but there is nothing in it. I did a new .net framework winform project set to 4.7.2 and added the local nuget for scintilla and tried to drag the control and got the error message.

I'm going to check a different machine first and see if it's the same behavior.

@ahmetsait
Copy link
Collaborator Author

.NET Framework projects are created in legacy project format and use packages.config by default so they don't put DLLs under ~/.nuget as I've said above. It's not possible to make designer work out of the box because package contents are not placed in a predictable path. Only reason they might have worked before is leftovers from some other project that used <PackageReference> with the same ScintillaNET version.

@desjarlais desjarlais merged commit f07c188 into desjarlais:master May 6, 2024
2 checks passed
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