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

Notification SetIconTexture is not ergonomic #1738

Open
anna-is-cute opened this issue Mar 21, 2024 · 2 comments
Open

Notification SetIconTexture is not ergonomic #1738

anna-is-cute opened this issue Mar 21, 2024 · 2 comments

Comments

@anna-is-cute
Copy link
Contributor

The documentation for IActiveNotification.SetIconTexture states that the texture will be disposed on notification dismiss automatically.

I have a collection of a few icons I intend to set on a notification. Rather than parse the icons every single time I make a notification, I load them all at once at startup and cache them. On plugin dispose, I also dispose of the icons.

SetIconTexture makes this scheme impossible. Ideally, I would like to be able to set an icon multiple times throughout a notification's lifetime, so using CreateWrapSharingLowLevelResource is unwieldy. A SetIconTexture-derivative that marks the texture as not-to-be-disposed would be ideal.

Also unwieldy is the inability to compare the current icon texture to another texture. I see in the source that Dalamud has the ability to do so, so it would be nice if API consumers could do the same. Consider, for example, this code:

if (this.Icons.TryGetValue(state, out var texture)) {
    if (notif.Icon.Texture != texture) { // would love to be able to do this
        notif.SetIconTextureWithoutAutoDispose(texture);
    }
}

As a final aside, the documentation for HardExpiry is pretty heavy on the double-negatives:

If neither INotification.HardExpiry nor INotification.InitialDuration is not MaxValue, then the notification will not expire after a set time.

Consider rewriting this to be more-easily parsed:

If both INotification.HardExpiry and INotification.InitialDuration are MaxValue, then the notification will not expire after a set time.

@Soreepeong
Copy link
Contributor

Soreepeong commented Mar 21, 2024

The primary motivation for only supporting ownership transferal of textures was that the lifetime of texture would get confusing, especially when a notification may outlive a plugin. But now that I think about it, removing the textures for the notifications invoked by the plugin on plugin unload should work; I'll add SetIconTexture( Task<IDTW?>|IDTW? texture, bool leaveOpen ) variant so that it will not dispose the texture upon the notification dismiss. Instead, if leaveOpen is set, the icon will be cleared if a notification is still visible after plugin is unloaded.

I'm thinking that texture comparison should be done by introducing a function instead of operators, as even for a single texture wrap there are various ways of consuming it. Does adding static bool ResourceEquals(this IDTW? left, IDTW? right); as an extension method sound acceptable, which compares the underlying D3D11 shader resource view?

Last one I'll fix that along with changing the rest.

@anna-is-cute
Copy link
Contributor Author

Does adding static bool ResourceEquals(this IDTW? left, IDTW? right); as an extension method sound acceptable, which compares the underlying D3D11 shader resource view?

Yes, but IActiveNotification needs a way to expose the IDalamudTextureWrap to do the comparison. Currently there's only INotificationIcon.

Soreepeong added a commit to Soreepeong/Dalamud that referenced this issue Mar 21, 2024
Notification record and IActiveNotification interface now supports
setting or updating the texture wraps being used, and SetIconTexture has
gotten more overloads to support leaveOpen mechanism that can commonly
be found with Stream wrappers.

ImGui widget is updated to support testing setting "leaveOpen" and
updating "IconTexture" property via setter, making it possible to check
whether IDTW.Dispose is being called under given conditions.

Some changes to doccomments are made.
goaaats pushed a commit that referenced this issue Mar 22, 2024
* Add IconTexture/Wrap to INotification (#1738)

Notification record and IActiveNotification interface now supports
setting or updating the texture wraps being used, and SetIconTexture has
gotten more overloads to support leaveOpen mechanism that can commonly
be found with Stream wrappers.

ImGui widget is updated to support testing setting "leaveOpen" and
updating "IconTexture" property via setter, making it possible to check
whether IDTW.Dispose is being called under given conditions.

Some changes to doccomments are made.

* typo
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

No branches or pull requests

2 participants