Feature/add complex notifications - #261
Conversation
Add overload to SetIconFile for loading icon from embedded resource
feat: add docs symbols to nuget package
Implement disabling user zoom.
… file name. Fixed bug passing boolean parameters on startup.
…oolean startup parameters.
There was a problem hiding this comment.
Pull request overview
This PR introduces a richer notification API (backed by new native interop) and adds a few new window features/options, along with updates to packaging/publishing configuration.
Changes:
- Add
ZoomEnabledstartup/runtime control and expose it via fluent API. - Replace the simple
SendNotification(title, body)flow with aPhotinoNotification-based API and related P/Invokes/types. - Add
defaultFileNamesupport for Save File dialogs and add embedded-resource icon support; update packaging/pipelines for symbol publishing.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| Photino.NET/PhotinoWindow.NET.cs | Adds zoom enablement, embedded-resource icon helper, SaveFile default filename, and new CreateNotification entry point. |
| Photino.NET/PhotinoNotification.cs | Adds managed notification wrapper (actions/callback wiring + fluent methods). |
| Photino.NET/PhotinoNotificationDllImports.cs | Adds native interop declarations for complex notifications. |
| Photino.NET/PhotinoNotificationType.cs | Adds enum for toast template selection. |
| Photino.NET/PhotinoNotificationDismissalReason.cs | Adds dismissal reason enum used by callbacks. |
| Photino.NET/PhotinoNotificationButton.cs | Adds a button model (currently not wired to PhotinoNotification). |
| Photino.NET/PhotinoNotificationBuilder.cs | Adds a builder abstraction for notifications (currently out of sync with PhotinoNotification). |
| Photino.NET/PhotinoNativeParameters.cs | Adds ZoomEnabled to startup parameter struct. |
| Photino.NET/PhotinoNativeDelegates.cs | Adds notification callback delegate types. |
| Photino.NET/PhotinoDllImports.cs | Adds zoom-enabled P/Invokes; updates SaveFile P/Invoke signature. |
| Photino.NET/Photino.NET.csproj | Enables symbol/doc generation settings and removes Photino.Native package reference. |
| azure-pipelines-photino.net-prod.yml | Changes published artifact pattern to *.symbols.nupkg. |
| azure-pipelines-photino.net-dev.yml | Changes published artifact pattern to *.symbols.nupkg. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public PhotinoNotification CreateNotification(PhotinoNotificationType type) | ||
| { | ||
| Log($".SendNotification({title}, {body})"); | ||
| Log($".Createnotification({type})"); |
There was a problem hiding this comment.
The log message has a typo/inconsistent casing: .Createnotification({type}) should match the public API name CreateNotification for easier filtering/diagnostics.
| Log($".Createnotification({type})"); | |
| Log($".CreateNotification({type})"); |
| public string ShowSaveFile(string title = "Save file", string defaultPath = null, (string Name, string[] Extensions)[] filters = null, string defaultFileName = null) | ||
| { | ||
| defaultPath ??= Environment.GetFolderPath(Environment.SpecialFolder.MyDocuments); | ||
| filters ??= Array.Empty<(string, string[])>(); | ||
| defaultFileName ??= string.Empty; |
There was a problem hiding this comment.
ShowSaveFile adds a new defaultFileName parameter but the XML doc comment above the method doesn’t document it. With doc generation enabled in the csproj, this can produce XML doc warnings/incomplete docs for consumers; add a <param name="defaultFileName">…</param> entry (and keep the synchronous/async overloads consistent).
| public async Task<string> ShowSaveFileAsync(string title = "Choose file", string defaultPath = null, (string Name, string[] Extensions)[] filters = null, string defaultFileName = null) | ||
| { | ||
| return await Task.Run(() => ShowSaveFile(title, defaultPath, filters)); | ||
| return await Task.Run(() => ShowSaveFile(title, defaultPath, filters, defaultFileName)); |
There was a problem hiding this comment.
ShowSaveFileAsync adds a new defaultFileName parameter but the XML doc comment above the method doesn’t document it. Add a matching <param name="defaultFileName">…</param> to avoid incomplete generated docs (and potential CS1573 warnings).
| string tempFile = Path.Combine(Path.GetTempPath(), fileName); | ||
|
|
||
| using (FileStream fileStream = new FileStream(tempFile, FileMode.Create, FileAccess.Write)) | ||
| { |
There was a problem hiding this comment.
ExtractEmbeddedResourceToTempFile uses fileName directly when constructing the temp path. If callers pass a path (e.g., ../... or subdir/file.ico), this can write outside the temp directory or overwrite unintended files. Consider sanitizing with Path.GetFileName(fileName) and generating a unique temp filename (e.g., GUID) to avoid collisions/overwrites.
| { | ||
| string resourceName = $"{resourceNamespace}.wwwroot.{fileName}"; | ||
|
|
||
| Assembly assembly = Assembly.GetExecutingAssembly(); |
There was a problem hiding this comment.
This method uses Assembly.GetExecutingAssembly() to locate embedded resources. For library usage, the embedded icon is more likely to live in the entry/calling assembly (the app), so this will often fail to find resources even when the namespace is correct. Consider accepting an Assembly parameter, or using Assembly.GetEntryAssembly() (with a fallback) so resources can be resolved reliably.
| Assembly assembly = Assembly.GetExecutingAssembly(); | |
| Assembly assembly = Assembly.GetEntryAssembly() ?? Assembly.GetExecutingAssembly(); |
| @@ -25,15 +33,11 @@ | |||
| <AllowUnsafeBlocks>true</AllowUnsafeBlocks> | |||
| <Copyright>Copyright (C) 2025</Copyright> | |||
| </PropertyGroup> | |||
There was a problem hiding this comment.
The Photino.Native package dependency was removed. Since this library P/Invokes into Photino.Native, dropping the NuGet dependency likely breaks consumers (no native runtimes/content files pulled transitively) and is a breaking packaging change. If intentional, it should be explicitly handled (e.g., documented breaking change or alternative delivery); otherwise re-add the PackageReference so the dependency remains transitively available.
| <SymbolPackageFormat>symbols.nupkg</SymbolPackageFormat> | ||
| <GenerateDocumentationFile>True</GenerateDocumentationFile> | ||
| <DebugType>embedded</DebugType> |
There was a problem hiding this comment.
SymbolPackageFormat is set to symbols.nupkg (legacy) and DebugType is embedded. NuGet.org has long standardized on .snupkg for symbols; legacy symbols packages are typically rejected. Consider switching to snupkg and using a compatible PDB format (e.g., portable) so symbol publishing works reliably.
| <SymbolPackageFormat>symbols.nupkg</SymbolPackageFormat> | |
| <GenerateDocumentationFile>True</GenerateDocumentationFile> | |
| <DebugType>embedded</DebugType> | |
| <SymbolPackageFormat>snupkg</SymbolPackageFormat> | |
| <GenerateDocumentationFile>True</GenerateDocumentationFile> | |
| <DebugType>portable</DebugType> |
| inputs: | ||
| command: push | ||
| packagesToPush: "Photino.NET/bin/$(buildConfiguration)/*.nupkg" | ||
| packagesToPush: "Photino.NET/bin/$(buildConfiguration)/*.symbols.nupkg" |
There was a problem hiding this comment.
This pipeline now pushes only *.symbols.nupkg. That will skip publishing the main package (*.nupkg), and legacy symbols.nupkg is typically not accepted by NuGet.org. Update the pipeline to push the main .nupkg (and optionally .snupkg if you switch to SymbolPackageFormat=snupkg).
| packagesToPush: "Photino.NET/bin/$(buildConfiguration)/*.symbols.nupkg" | |
| packagesToPush: "Photino.NET/bin/$(buildConfiguration)/*.nupkg;!Photino.NET/bin/$(buildConfiguration)/*.symbols.nupkg" |
| command: push | ||
| feedsToUse: "select" | ||
| publishVstsFeed: "Photino.Native/PhotinoPackages" | ||
| packagesToPush: "Photino.NET/bin/$(buildConfiguration)/*.nupkg" | ||
| packagesToPush: "Photino.NET/bin/$(buildConfiguration)/*.symbols.nupkg" | ||
| allowPackageConflicts: true |
There was a problem hiding this comment.
This pipeline now pushes only *.symbols.nupkg. That likely prevents the actual package (*.nupkg) from being published to the feed, and legacy symbols.nupkg may not be supported by the feed. Push the main package and (if desired) the .snupkg symbols package separately.
| public PhotinoNotification CreateNotification(PhotinoNotificationType type) | ||
| { | ||
| Log($".SendNotification({title}, {body})"); | ||
| Log($".Createnotification({type})"); | ||
| if (_nativeInstance == IntPtr.Zero) | ||
| throw new ApplicationException("SendNotification cannot be called until after the Photino window is initialized."); | ||
| Invoke(() => Photino_ShowNotification(_nativeInstance, title, body)); | ||
| throw new ApplicationException("CreateNotification cannot be called until after the Photino window is initialized."); | ||
| return new PhotinoNotification(_nativeInstance) | ||
| .SetType(type); |
There was a problem hiding this comment.
SendNotification(string title, string body) was removed and replaced with CreateNotification(...). This is a breaking public API change for existing consumers; consider keeping the old overload as a convenience wrapper (possibly [Obsolete]) that builds/shows a simple notification using the new notification API.
|
@pathartl We'd like to merge your changes on this PR (and the native one associated with this one) |
See tryphotino/photino.Native#166 for more details