goatcorp / Dalamud

FFXIV plugin framework and API
GNU Affero General Public License v3.0
1.07k stars 254 forks source link

Notification SetIconTexture is not ergonomic #1738

Open anna-is-cute opened 3 months ago

anna-is-cute commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.