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.

Sign up for GitHub

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

Switch INotificationManager and ITitleScreenMenu to use ISharedImmediateTexture #1879

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Switch INotificationManager and ITitleScreenMenu to use ISharedImmediateTexture #1879

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
50e0560
Update ITitleScreenMenu
Critical-Impact Jul 1, 2024
afcc920
Make ForwardingSharedImmediateTexture public, remove invalid entries …
Critical-Impact Jul 1, 2024
4e545c4
Use IImmediateTexture for notifications
Critical-Impact Jul 2, 2024
c7d91e0
Clear icon texture when not owned
Critical-Impact Jul 3, 2024
25565b4
Restore missing interface methods/properties as wrappers
Critical-Impact Jul 4, 2024
878d4df
Catch task exception
Critical-Impact Jul 4, 2024
778f48d
Add missing properties into TitleScreenMenu
Critical-Impact Jul 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions Dalamud/Interface/ImGuiNotification/IActiveNotification.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

namespace Dalamud.Interface.ImGuiNotification;

using Textures;

/// <summary>Represents an active notification.</summary>
/// <remarks>Not to be implemented by plugins.</remarks>
public interface IActiveNotification : INotification
Expand Down Expand Up @@ -52,6 +54,14 @@ public interface IActiveNotification : INotification
/// <remarks>This does not override <see cref="INotification.HardExpiry"/>.</remarks>
void ExtendBy(TimeSpan extension);

/// <summary>Sets the icon from <see cref="ISharedImmediateTexture"/>, overriding the icon.</summary>
/// <param name="sharedImmediateTexture">The new shared immediate texture to use, or null to clear and revert back to the icon specified
/// from <see cref="INotification.Icon"/>.</param>
/// <remarks>
/// <para>If you need to provide a IDalamudTextureWrap that you will be responsible for disposing of, wrap it with <see cref="ForwardingSharedImmediateTexture"/>.</para>
/// </remarks>
void SetIconTexture(ISharedImmediateTexture? sharedImmediateTexture);

/// <summary>Sets the icon from <see cref="IDalamudTextureWrap"/>, overriding the icon.</summary>
/// <param name="textureWrap">The new texture wrap to use, or null to clear and revert back to the icon specified
/// from <see cref="INotification.Icon"/>.</param>
Expand All @@ -61,6 +71,7 @@ public interface IActiveNotification : INotification
/// <para>If <see cref="DismissReason"/> is not <c>null</c>, then calling this function will simply dispose the
/// passed <paramref name="textureWrap"/> without actually updating the icon.</para>
/// </remarks>
[Obsolete("Will be removed in API11")]
void SetIconTexture(IDalamudTextureWrap? textureWrap);

/// <summary>Sets the icon from <see cref="IDalamudTextureWrap"/>, overriding the icon, once the given task
Expand All @@ -76,6 +87,7 @@ public interface IActiveNotification : INotification
/// <para>If <see cref="DismissReason"/> is not <c>null</c>, then calling this function will simply dispose the
/// result of the passed <paramref name="textureWrapTask"/> without actually updating the icon.</para>
/// </remarks>
[Obsolete("Will be removed in API11")]
void SetIconTexture(Task<IDalamudTextureWrap?>? textureWrapTask);

/// <summary>Sets the icon from <see cref="IDalamudTextureWrap"/>, overriding the icon.</summary>
Expand All @@ -90,6 +102,7 @@ public interface IActiveNotification : INotification
/// calling this function will simply dispose the passed <paramref name="textureWrap"/> without actually updating
/// the icon.</para>
/// </remarks>
[Obsolete("Will be removed in API11")]
void SetIconTexture(IDalamudTextureWrap? textureWrap, bool leaveOpen);

/// <summary>Sets the icon from <see cref="IDalamudTextureWrap"/>, overriding the icon, once the given task
Expand All @@ -108,6 +121,7 @@ public interface IActiveNotification : INotification
/// <para>If <see cref="DismissReason"/> is not <c>null</c>, then calling this function will simply dispose the
/// result of the passed <paramref name="textureWrapTask"/> without actually updating the icon.</para>
/// </remarks>
[Obsolete("Will be removed in API11")]
void SetIconTexture(Task<IDalamudTextureWrap?>? textureWrapTask, bool leaveOpen);

/// <summary>Generates a new value to use for <see cref="Id"/>.</summary>
Expand Down
18 changes: 18 additions & 0 deletions Dalamud/Interface/ImGuiNotification/INotification.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

namespace Dalamud.Interface.ImGuiNotification;

using Textures;

/// <summary>Represents a notification.</summary>
/// <remarks>Not to be implemented by plugins.</remarks>
public interface INotification
Expand All @@ -26,6 +28,20 @@ public interface INotification
/// </summary>
INotificationIcon? Icon { get; set; }

/// <summary>Gets or sets a texture wrap that will be used in place of <see cref="Icon"/> if set.</summary>
/// <remarks>
/// <para>A texture wrap set via this property will <b>NOT</b> be disposed when the notification is dismissed.
/// Use <see cref="IActiveNotification.SetIconTexture(ISharedImmediateTexture?)"/> or
/// <see cref="IActiveNotification.SetIconTexture(Task{ISharedImmediateTexture?}?)"/> to use a texture, after calling
/// <see cref="INotificationManager.AddNotification"/>. Call either of those functions with <c>null</c> to revert
/// the effective icon back to this property.</para>
/// <para>This property and <see cref="IconTextureTask"/> are bound together. If the task is not <c>null</c> but
/// <see cref="Task.IsCompletedSuccessfully"/> is <c>false</c> (because the task is still in progress or faulted,)
/// the property will return <c>null</c>. Setting this property will set <see cref="IconTextureTask"/> to a new
/// completed <see cref="Task{TResult}"/> with the new value as its result.</para>
/// </remarks>
public ISharedImmediateTexture? ImmediateIconTexture { get; set; }

/// <summary>Gets or sets a texture wrap that will be used in place of <see cref="Icon"/> if set.</summary>
/// <remarks>
/// <para>A texture wrap set via this property will <b>NOT</b> be disposed when the notification is dismissed.
Expand All @@ -38,6 +54,7 @@ public interface INotification
/// the property will return <c>null</c>. Setting this property will set <see cref="IconTextureTask"/> to a new
/// completed <see cref="Task{TResult}"/> with the new value as its result.</para>
/// </remarks>
[Obsolete("Will be removed in API11")]
public IDalamudTextureWrap? IconTexture { get; set; }

/// <summary>Gets or sets a task that results in a texture wrap that will be used in place of <see cref="Icon"/> if
Expand All @@ -50,6 +67,7 @@ public interface INotification
/// the effective icon back to this property.</para>
/// <para>This property and <see cref="IconTexture"/> are bound together.</para>
/// </remarks>
[Obsolete("Will be removed in API11")]
Task<IDalamudTextureWrap?>? IconTextureTask { get; set; }

/// <summary>Gets or sets the hard expiry.</summary>
Expand Down
2 changes: 1 addition & 1 deletion Dalamud/Interface/ImGuiNotification/Internal/ActiveNotification.ImGui.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ private void DrawIcon(Vector2 minCoord, Vector2 size)
var maxCoord = minCoord + size;
var iconColor = this.Type.ToColor();

if (NotificationUtilities.DrawIconFrom(minCoord, maxCoord, this.IconTextureTask))
if (NotificationUtilities.DrawIconFrom(minCoord, maxCoord, this.ImmediateIconTexture))
return;

if (this.Icon?.DrawIcon(minCoord, maxCoord, iconColor) is true)
Expand Down
92 changes: 40 additions & 52 deletions Dalamud/Interface/ImGuiNotification/Internal/ActiveNotification.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

namespace Dalamud.Interface.ImGuiNotification.Internal;

using Textures;

/// <summary>Represents an active notification.</summary>
internal sealed partial class ActiveNotification : IActiveNotification
{
Expand All @@ -23,9 +25,6 @@ internal sealed partial class ActiveNotification : IActiveNotification
private readonly Easing progressEasing;
private readonly Easing expandoEasing;

/// <summary>Whether to call <see cref="IDisposable.Dispose"/> on <see cref="DisposeInternal"/>.</summary>
private bool hasIconTextureOwnership;

/// <summary>Gets the time of starting to count the timer for the expiration.</summary>
private DateTime lastInterestTime;

Expand Down Expand Up @@ -118,32 +117,25 @@ public INotificationIcon? Icon
set => this.underlyingNotification.Icon = value;
}

/// <inheritdoc/>
public ISharedImmediateTexture? ImmediateIconTexture
{
get => this.underlyingNotification.ImmediateIconTexture;
set => this.underlyingNotification.ImmediateIconTexture = value;
}

/// <inheritdoc/>
public IDalamudTextureWrap? IconTexture
{
get => this.underlyingNotification.IconTexture;
set => this.IconTextureTask = value is null ? null : Task.FromResult(value);
set => this.underlyingNotification.IconTexture = value;
}

/// <inheritdoc/>
public Task<IDalamudTextureWrap?>? IconTextureTask
{
get => this.underlyingNotification.IconTextureTask;
set
{
// Do nothing if the value did not change.
if (this.underlyingNotification.IconTextureTask == value)
return;

if (this.hasIconTextureOwnership)
{
_ = this.underlyingNotification.IconTextureTask?.ToContentDisposedTask(true);
this.underlyingNotification.IconTextureTask = null;
this.hasIconTextureOwnership = false;
}

this.underlyingNotification.IconTextureTask = value;
}
set => this.underlyingNotification.IconTextureTask = value;
}

/// <inheritdoc/>
Expand Down Expand Up @@ -266,36 +258,40 @@ public void ExtendBy(TimeSpan extension)
}

/// <inheritdoc/>
public void SetIconTexture(IDalamudTextureWrap? textureWrap) =>
this.SetIconTexture(textureWrap, false);
public void SetIconTexture(ISharedImmediateTexture? sharedImmediateTexture)
{
this.underlyingNotification.ImmediateIconTexture = sharedImmediateTexture;
}

/// <inheritdoc/>
public void SetIconTexture(IDalamudTextureWrap? textureWrap, bool leaveOpen) =>
this.SetIconTexture(textureWrap is null ? null : Task.FromResult(textureWrap), leaveOpen);

/// <inheritdoc/>
public void SetIconTexture(Task<IDalamudTextureWrap?>? textureWrapTask) =>
this.SetIconTexture(textureWrapTask, false);
[Obsolete("Will be removed in API11")]
public void SetIconTexture(IDalamudTextureWrap? textureWrap)
{
this.SetIconTexture(textureWrap != null ? new ForwardingSharedImmediateTexture(textureWrap) : null);
}

/// <inheritdoc/>
public void SetIconTexture(Task<IDalamudTextureWrap?>? textureWrapTask, bool leaveOpen)
[Obsolete("Will be removed in API11")]
public void SetIconTexture(Task<IDalamudTextureWrap?>? textureWrapTask)
{
// If we're requested to replace the texture with the same texture, do nothing.
if (this.underlyingNotification.IconTextureTask == textureWrapTask)
return;

if (this.DismissReason is not null)
{
if (!leaveOpen)
textureWrapTask?.ToContentDisposedTask(true);
return;
}
var result = textureWrapTask?.Result;
this.SetIconTexture(result != null ? new ForwardingSharedImmediateTexture(result) : null);
}

if (this.hasIconTextureOwnership)
_ = this.underlyingNotification.IconTextureTask?.ToContentDisposedTask(true);
/// <inheritdoc/>
[Obsolete("Will be removed in API11")]
public void SetIconTexture(IDalamudTextureWrap? textureWrap, bool leaveOpen)
{
this.SetIconTexture(textureWrap != null ? new ForwardingSharedImmediateTexture(textureWrap) : null);
}

this.hasIconTextureOwnership = !leaveOpen;
this.underlyingNotification.IconTextureTask = textureWrapTask;
/// <inheritdoc/>
[Obsolete("Will be removed in API11")]
public void SetIconTexture(Task<IDalamudTextureWrap?>? textureWrapTask, bool leaveOpen)
{
var result = textureWrapTask?.Result;
this.SetIconTexture(result != null ? new ForwardingSharedImmediateTexture(result) : null);
}

/// <summary>Removes non-Dalamud invocation targets from events.</summary>
Expand All @@ -317,10 +313,9 @@ internal void RemoveNonDalamudInvocations()
if (this.Icon is { } previousIcon && !IsOwnedByDalamud(previousIcon.GetType()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Change this line so that IsOwnedByDalamud check is also done for the IDTW stored inside ForwardingSharedImmediateTexture.

this.Icon = null;

// Clear the texture if we don't have the ownership.
// The texture probably was owned by the plugin being unloaded in such case.
if (!this.hasIconTextureOwnership)
this.IconTextureTask = null;

if (this.ImmediateIconTexture is { } iconTexture && (!IsOwnedByDalamud(iconTexture.GetType()) || (iconTexture.TryGetWrap(out var wrap, out _) && !IsOwnedByDalamud(wrap.GetType()))))
this.ImmediateIconTexture = null;

this.isInitiatorUnloaded = true;
this.UserDismissable = true;
Expand Down Expand Up @@ -400,13 +395,6 @@ internal bool UpdateOrDisposeInternal()
/// <summary>Clears the resources associated with this instance of <see cref="ActiveNotification"/>.</summary>
internal void DisposeInternal()
{
if (this.hasIconTextureOwnership)
{
_ = this.underlyingNotification.IconTextureTask?.ToContentDisposedTask(true);
this.underlyingNotification.IconTextureTask = null;
this.hasIconTextureOwnership = false;
}

this.Dismiss = null;
this.Click = null;
this.DrawActions = null;
Expand Down
27 changes: 24 additions & 3 deletions Dalamud/Interface/ImGuiNotification/Notification.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

namespace Dalamud.Interface.ImGuiNotification;

using Textures;

/// <summary>Represents a blueprint for a notification.</summary>
public sealed record Notification : INotification
{
Expand All @@ -29,15 +31,34 @@ public sealed record Notification : INotification
/// <inheritdoc/>
public INotificationIcon? Icon { get; set; }

/// <inheritdoc/>
public ISharedImmediateTexture? ImmediateIconTexture { get; set; }

/// <inheritdoc/>
public IDalamudTextureWrap? IconTexture
{
get => this.IconTextureTask?.IsCompletedSuccessfully is true ? this.IconTextureTask.Result : null;
set => this.IconTextureTask = value is null ? null : Task.FromResult(value);
get => this.ImmediateIconTexture?.GetWrapOrDefault();
set => this.ImmediateIconTexture = value != null ? new ForwardingSharedImmediateTexture(value) : null;
}

/// <inheritdoc/>
public Task<IDalamudTextureWrap?>? IconTextureTask { get; set; }
public Task<IDalamudTextureWrap?>? IconTextureTask
{
get => Task.FromResult(this.ImmediateIconTexture?.GetWrapOrDefault());

set
{
if (value == null)
{
this.ImmediateIconTexture = null;
}
else
{
var dalamudTextureWrap = value.Result;
Copy link
Contributor

Choose a reason for hiding this comment

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

Assigning failed tasks should not throw. IMO it's better to just leave previous properties/methods alone and add ISIT on its own, and only on drawing refer to ISIT at first and then check the rest to figure out the correct thing to draw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd basically need to throw out everything I've already done if that's the route you want to go, as it'd be faster for me to start on a new branch and implement a ImmediateIconTexture there rather than trying to put back in what I've already removed

Have added in a try/catch, not pretty but does aleviate that above concern

Copy link
Contributor

Choose a reason for hiding this comment

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

Hopefully nobody refers to the state of the task after assigning it to the notification, too. Not sure why would one do that, so I'd guess it's be safe for this property to be "inconsistent".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only thing I could think of is creating a new Task.FromException<IDalamudTextureWrap?>(exception), storing it in a field and returning that when IconTextureTask.get is called

Then when a set resolves correctly, it sets the failed task field to null.

Not sure why would want to do it either, if I don't do something like above, it shouldn't cause a crash as they'll still have to null check, just a bit incosnsistent as you said

Copy link
Contributor

Choose a reason for hiding this comment

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

Another possible way would be making ForwardingSIT store Task instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've considered it however it'd require the GetWrapOrEmpty function to call Service<DalamudAssetManager>.Get().Empty4X4 which adds a dependency on an internal service.

Now if we made it so you could only get a ForwardingSharedImmediateTexture's from the ITextureProvider and it injected the 4x4 into the ForwardingSharedImmediateTexture that'd be fine but it adds a hard dependency to an object that could concievably be constructed outside of Dalamud

this.ImmediateIconTexture = dalamudTextureWrap == null ? null : new ForwardingSharedImmediateTexture(dalamudTextureWrap);
}
}
}

/// <inheritdoc/>
public DateTime HardExpiry { get; set; } = DateTime.MaxValue;
Expand Down
25 changes: 15 additions & 10 deletions Dalamud/Interface/ImGuiNotification/NotificationUtilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

namespace Dalamud.Interface.ImGuiNotification;

using Textures;

/// <summary>Utilities for implementing stuff under <see cref="ImGuiNotification"/>.</summary>
public static class NotificationUtilities
{
Expand Down Expand Up @@ -78,6 +80,19 @@ internal static unsafe bool DrawIconFrom(
return true;
}

/// <summary>Draws an icon from an instance of <see cref="ISharedImmediateTexture"/>.</summary>
/// <param name="minCoord">The coordinates of the top left of the icon area.</param>
/// <param name="maxCoord">The coordinates of the bottom right of the icon area.</param>
/// <param name="texture">The texture.</param>
/// <returns><c>true</c> if anything has been drawn.</returns>
internal static bool DrawIconFrom(Vector2 minCoord, Vector2 maxCoord, ISharedImmediateTexture? texture)
{
if (texture is null)
return false;

return DrawIconFrom(minCoord, maxCoord, texture.GetWrapOrEmpty());
}

/// <summary>Draws an icon from an instance of <see cref="IDalamudTextureWrap"/>.</summary>
/// <param name="minCoord">The coordinates of the top left of the icon area.</param>
/// <param name="maxCoord">The coordinates of the bottom right of the icon area.</param>
Expand Down Expand Up @@ -105,16 +120,6 @@ internal static bool DrawIconFrom(Vector2 minCoord, Vector2 maxCoord, IDalamudTe
}
}

/// <summary>Draws an icon from an instance of <see cref="Task{TResult}"/> that results in an
/// <see cref="IDalamudTextureWrap"/>.</summary>
/// <param name="minCoord">The coordinates of the top left of the icon area.</param>
/// <param name="maxCoord">The coordinates of the bottom right of the icon area.</param>
/// <param name="textureTask">The task that results in a texture.</param>
/// <returns><c>true</c> if anything has been drawn.</returns>
/// <remarks>Exceptions from the task will be treated as if no texture is provided.</remarks>
internal static bool DrawIconFrom(Vector2 minCoord, Vector2 maxCoord, Task<IDalamudTextureWrap?>? textureTask) =>
textureTask?.IsCompletedSuccessfully is true && DrawIconFrom(minCoord, maxCoord, textureTask.Result);

/// <summary>Draws an icon from an instance of <see cref="LocalPlugin"/>.</summary>
/// <param name="minCoord">The coordinates of the top left of the icon area.</param>
/// <param name="maxCoord">The coordinates of the bottom right of the icon area.</param>
Expand Down
9 changes: 5 additions & 4 deletions Dalamud/Interface/Internal/DalamudInterface.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
using Dalamud.Interface.Internal.Windows.StyleEditor;
using Dalamud.Interface.ManagedFontAtlas.Internals;
using Dalamud.Interface.Style;
using Dalamud.Interface.Textures;
using Dalamud.Interface.Utility;
using Dalamud.Interface.Utility.Raii;
using Dalamud.Interface.Windowing;
Expand Down Expand Up @@ -166,24 +167,24 @@ private DalamudInterface(
{
titleScreenMenu.AddEntryCore(
Loc.Localize("TSMDalamudPlugins", "Plugin Installer"),
dalamudAssetManager.GetDalamudTextureWrap(DalamudAsset.LogoSmall),
new ForwardingSharedImmediateTexture(dalamudAssetManager.GetDalamudTextureWrap(DalamudAsset.LogoSmall)),
this.OpenPluginInstaller);
titleScreenMenu.AddEntryCore(
Loc.Localize("TSMDalamudSettings", "Dalamud Settings"),
dalamudAssetManager.GetDalamudTextureWrap(DalamudAsset.LogoSmall),
new ForwardingSharedImmediateTexture(dalamudAssetManager.GetDalamudTextureWrap(DalamudAsset.LogoSmall)),
this.OpenSettings);

titleScreenMenu.AddEntryCore(
"Toggle Dev Menu",
dalamudAssetManager.GetDalamudTextureWrap(DalamudAsset.LogoSmall),
new ForwardingSharedImmediateTexture(dalamudAssetManager.GetDalamudTextureWrap(DalamudAsset.LogoSmall)),
() => Service<DalamudInterface>.GetNullable()?.ToggleDevMenu(),
VirtualKey.SHIFT);

if (!configuration.DalamudBetaKind.IsNullOrEmpty())
{
titleScreenMenu.AddEntryCore(
Loc.Localize("TSMDalamudDevMenu", "Developer Menu"),
dalamudAssetManager.GetDalamudTextureWrap(DalamudAsset.LogoSmall),
new ForwardingSharedImmediateTexture(dalamudAssetManager.GetDalamudTextureWrap(DalamudAsset.LogoSmall)),
() => this.isImGuiDrawDevMenu = true);
}
});
Expand Down
Loading
Loading